Getters and Setters unclear example

Question

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:

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

As a fix he introduces this:

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

Answer 1

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));
}

Answer 2

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.

Answer 3

• 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. ;)