2

I know autmatic getters and setters are considers bad as they tend to break object's encapsulation. They also move the work that should have been done within the object outside.

Allen Holub is a big advocate of avoiding them. In one of his presentations he gives this example as bad design:

enter image description here

Here currency conversion happens outside the Money class and is spread across the application wherever conversion is needed.

As a fix he introduces this:

enter image description here

It makes sense to me that a Money object now uses Currency object and Money arithmetics are done within the Money class, but this line m=currency.convertToYourCurrency(m) I don't understand.

How will the Currency method public Money convertToYourCurrency(Money money); know what's the argument's currency? Is it going to use a getter? If it delegates to Money for conversion why bother calling it from within Money to begin with?

Did Mr. Holub make a typo on his slide or am I missing something obvious?

Link to the class: https://vimeo.com/user22258446/review/79095046/9c0ffa90c8 this is from ~44:22

  • 2
    It seems you mix up "getters" with "default getters". "default getters" are indeed evil because they are created automatically as they are "maybe usefull later". Getters on itself that are chosen to be implemented in the class are nothing more than any other method except that there has no good name been found. E.g. you can name it customer.tellMeYourBalance() or customer.getBalance(). It both has the same meaning and "getter" is how this kind of normally simple logic methods is called. Btw, I actually think, this Money class is not that well designed. – valenterry Dec 6 '14 at 16:36
4

The example is kinda messed up, and looks to me like it just hasn't been thought through.

Currencies aren't like scientific measurements, you shouldn't implicitly convert between them on demand. The value of a currency will change on the day. You should really only convert between currencies with the end-users explicit permission, certainly not implicitly all over the place.

The class holds money as a double. DON'T EVER DO THAT. See What every computer scientist should know about floating point. Money should always be held in an integral or decimal type. If you use double you'll get rounding errors and people get really unhappy if you round their money wrong.

As it stands, there is a circular dependency between Money and Currency. Money contains a currency, but currency also works with money. That's not necessarily wrong, but is suspect.

The line in question:

m=currency.convertToYourCurrency(m)

You are correct. As written, it would require a getter or something similiar. Instead, it should be

m = m.convertToCurrency(currency)

Where convertToCurrency is defined

Money convertToCurrency(Currency currency) {
   return new Money(currency, this.currency.convertTo(currency, this.value));
}
1

How will the Currency method public Money convertToYourCurrency(Money money); know what's the argument's currency? Is it going to use a getter?

One could say that, loosely speaking, the fix is a very simplistic visitor pattern. The Currency object is the visitor that, by the pattern's definition, performs operations on the Money objects. Of necessity the visitor must know\have access to the Money class structure in order to its thing.

If it delegates to Money for conversion why bother calling it from within Money to begin with?

  • The larger point is encapsulating the currency conversion - not making the Money client code do it.
  • currency conversion code is now re-usable, every Money client does not have to write its own conversion code.
  • Single Responsibility Principle. One can argue that for the design intent conversion belongs in the Money class.
1
  • I think his first example is a little contrived. If you've really gotten yourself into that pickle, you probably didn't think hard enough about the problem in the first place.

  • The "largerThan" and "addTo" methods make sense because rather than carry two values & imposing some semantics outside the class on the separate values, he's defining the operations on value / currency pair through the class operations. And that's a good thing, because the meaning of "largerThan" or "addTo" are encapsulated in the class. Which is (probably) better than leaving the definition (and possible misuse) to the client.

  • But an alternate approach would be to express all money values in some canonical currency, do all your operations in that common currency, and then convert it to the desired currency at the last minute.

  • The part where he dispenses with the getter in favor of a write to method is ... something. One could argue that introduces a dependency on the Writer interface rather than allowing the client to get the value and do with it what it wants.

I've done that before, but not as a general practice.

For example, if you were going to serialize Money to JSON, would you introduce a "toJSON" method that accepts a "serializer" object? Now that class has another dependency & an more semantics to manage. There's potential for blowing up the interface with all the different integration points.

...e.g. to forge on, how would you save a Money to a database as a part of some transaction?

So he's making an interesting point, and I think his example of rolling up logical operations into the class is a good one. But the idea of completely dispensing with getters is an interesting refactoring, but not one that I commonly see.

...

I think the core idea here is that he has an example where the operations are spread across 4 classes: Money, Currency, CurrencyConverter, and whatever client is pulling it all together.

But really, the Currency class has tighter usage if you eliminate CurrencyConverter and move it's logic inside Currency so that the currency type and operations on that type are bundled together & hidden from the outside world.

Then he pulls the operations on the value / currency pair and bundles those together inside the Money class, and hides them from the outside world.

So IMHO this looks more to me like an example of improving poor or weak encapsulation more than a condemnation of getters -- the getters here are more like accessories to the crime rather than the masterminds. ;)

  • Do you understand the m=currency.convertToYourCurrency(m) line? – Weltschmerz Dec 6 '14 at 4:57
  • sure; it's simply taking the (externalized) logic from CurrencyConverter and encapsulating it in the Currency class. Sometimes externalizing logic is a really nice refactoring, but in this case it just spreads logic across 2 classes that ought to be managed by one. His example doesn't show the details of convertToYourCurrency, but I'd bet 100 Yen, or 5.9 Krone, that the code inside convertYourCorrency is the same as CurrencyConverter. He's just taking logic that's external to the Currency class and encapsulating it in that class. (If I'm right) – Rob Dec 6 '14 at 6:20
  • see, I would describe what he's doing as "managing state by encapsulating semantics" ... rather than passing around raw values through getters, he's pulling logic into the class so he doesn't have to reveal the raw values. Which is a fair point, and one way to do things. – Rob Dec 6 '14 at 6:22
  • So if I understand you correctly you think there is a Money getter returning Currency. In the video he repeats 10 times how getters and setters are evil, they should not be used. They destroy this and mess up that, he is very clear. And this refactor example he specifically showed as a way of eliminating getters. That is why i dont understand that function. It needs 2 currencies to do the work, one is the object itself. How does it get the second currency? – Weltschmerz Dec 6 '14 at 16:01
  • Hm. I think you're right. "convertToYourCurrency" should be in the Money class -- that way it could see the (private) m.currency field as well as this.currency. The way it is, it looks like the code won't compile. – Rob Dec 6 '14 at 22:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.