Wednesday, June 18, 2008

Learning encapsulation should come before learning Rails

Do you see anything wrong in this one line of code?

puts 'Drink up your milk!' if @milk.expiration_date < 2.days.from_now


I think there is. In particular, the programmer can do better. It is about a programming concept that we have all heard and should be familiar with: encapsulation. If you are into testing, which you ought to be, you can probably identify such encapsulation violations by how smelly your tests are, as I believe in Test Driven Design. Going back to the line of code. What's wrong with it?

The pattern of this code is, you have code somewhere that yanks states (expiration_state) out of an object (@milk) and then interrogate against those states (< 2.days.from_now). By no means this is a rule, as exceptions do exist. But, when this happens, your programming siren in your head should go off as if the CI build is broken: can this be a method on the object @milk itself?

Ruby is a powerful programming language. It allows you to do all sorts of fancy programming: dynamically altering classes, add methods to only the selected few object instances, while being duck-typed and object-oriented all at the same time. There are examples galore in Rails itself. But to enable all such magic, all of your ActiveRecord domain models have getters and setters on all of their states (i.e. data). While that is convenient to access their states, sometimes you have to be careful. With great power comes great responsibility, and that responsibility comes down to you.

You might not think this applies to you. But have you ever written code like this in Ruby?

MyMailer.deliver_product_returned_notification if @product.state == State::Returned

total = @line_items.sum { |line_item| line_item.price * line_item.quantity }

if [:admin, :superuser].include?(@user.role)
...
end


Now, let's look at these rewritten examples:

puts 'Drink up your milk!' if @milk.expiring?

MyMailer.deliver_product_returned_notification if @product.returned?

total = @line_items.sum(&:subtotal)

if @user.superuser?
...
end


Forget about the loops, the blocks, the hype, and everything about Ruby for a sec. Code is code. Not only does the code become more readable, when you try to enrich your domain by naming things correctly, you could also very well be opening up new business concepts that wasn't previously clear or accurate. If you have a domain model, you are modeling it against domain concepts so that your app can interact with. You'd better be sure it is right, or else lots of time will go wasted on code that solves only half of the business problems all the time.

8 comments:

Clint Bishop said...

Preach it, Chu.

Brian Yamabe said...

Thank you. Your post has finally codified in my head why I've always had a problem with "tell, don't ask." In your milk example, you have now enforced a standard that I may or may not want in that "milk" now tells me if it is expired and I no longer know if this means it's 2 days before expiration which might mean I need to go get some milk or if it's past the expiration date and I shouldn't drink it.

I understand the point of "tell, don't ask" is for better separation of concerns, but in certain cases information in a class may be used in ways that don't concern it.

Stephen Chu said...

@brian:

I agree with you when you say "in certain cases information in a class may be used in ways that don't concern it." But I am not convinced by the example that you gave.

If my @milk object can answer expiring? and expired?, then you will still maintain the differentiation. If you have cases where you truly don't care, you can have another method discount_item?, which will become a new business concept.

One of the advantages of properly encapsulating states is that I can change the implementation of my Milk class without code changes of calling code. I can re-implement expiring? using the code like my blog entry hard-coded, or a string column, or talking to an inventory catalog web service, anything.

Unknown said...

This is beside the point, but I don't think the example does what one might think it does.

Stephen Chu said...

@fred:

I'm not sure what you mean. Could you elaborate?

Unknown said...

Well, presumably it's intended to mean "if the milk is going to expire in two days or less, drink it." But what it actually says is "If the expiration date is earlier than the day after tomorrow, drink it." So it could have expired a year ago, and you'll drink it and die.

Unknown said...

...which I suppose is really another argument in favor of encapsulation. It's easy to make a mistake in a quick one-liner, but the encapsulated version will probably be more thorough.
Or at least if it's not, then after a few people get sick you can fix all the code in one place. :-)

P.S. Date and Time arithmetic drive me nuts. I've been programming for more years than I can count (heh) and I still have to carefully think it through every time.

Anonymous said...

One sign that you're breaking encapsulation is when you violate the guideline of demeter. It may not be easy to spot in your milk example, but when you rewrite it like:

@milk.expiration_date.<(2.days.from_now)

it becomes obvious.

So while demeter is just a guideline, understanding that guideline gives you immediate insight into situations where your code is coupled to some particular object structure.