Extreme Enthusiasm

This is another thing I often see people do. Consider this code fragment:

<div id="sidebar">
  <%= render partial: "sidebar" %>
</div>

This basically says that in a portion of a layout that is called “sidebar” we want to render a partial template that is also called “sidebar”. Hmmmm. Duplication here! Why are these two things called the same? Let’s look into the “sidebar” template; we discover that it contains code that produces a navigation menu. Aha! Why not call the partial “navigation_menu” then?

The mistake in the above code was that the partial template was named after where it is used, not after what it is. When you correct that problem, what you get is

<div id="sidebar">
  <%= render partial: "navigation_menu" %>
</div>

and the duplication problem also disappears.

Another example I’ve seen in production code:

class Money {
  public int getValueForAuthorizationSystem() {
    return value.scaleByPowerOfTen(2).intValue();
  }   
}

Class Money should not really care what the Authorization System wants… it should be concerned with returning the amount in cents!

class Money {
  public int inCents() {
    return value.scaleByPowerOfTen(2).intValue();
  }   
}

Why is it that programmers name things after where they are used and not after what the things really are? I think that one major cause is the desire to break up a monolithic thing, because this is what design should be about, but doing it poorly. Suppose I have a 100 lines long method.

def long_method
  # ... ... ...
end

I use extract method a number of times and I end up with something like this:

def long_method
  # ...
  step_one(foo, bar, baz)
  step_two(foo, bar, zork)
  step_three(agh, foo, bar, zork)
  step_four(agh, bar, baz, zork)
end

what happened here? We took a procedure and cut it in arbitrary pieces. The result is that we have a number of methods that we can’t give meaningful names to, other than “step_X” and that have an excessive number of arguments. This means that we cut the procedure poorly. Note that each of the methods can only be used in the place where it is called; it depends on the context and can’t be used anywhere else.

What to do then? What I would do is to get back to the 100-lines method and try to cut it another way. Perhaps we will arrive at something like

def formerly_long_method
  do_something(foo)
  do_something(bar)
  do_something(baz, do_something(zork))
end

where the same helper method has been used in more than one way.

In conclusion: names like foobarForX should always be challenged. They might be fixed by looking again at what “foobarForX” really is; or they might be an indication that the design is cut the wrong way. In that case you try the cut again and see if you can improve the situation. Look for things that are meaningful concepts in the problem domain!

The number one design problem I see people do is writing code that is overspecific. By that I mean code that can only be used in one place and can never be reused. Programmers should always try to write code that is generic, reusable and independent from the context where it is used.

Design is (mainly) about breaking the solution in small, manageable pieces. However, how you do the cut is important; if you cut poorly, you end up with a worse mess than when you had a single monolithic piece.

This is an example I saw last week. It is Rails-specific but I think non-Rails readers will be able to follow the logic. The application had a general HTML layout that contained something like this:

<table>
  <tr>
    <!-- main application content -->
    <td valign="top"><%= yield %></td>

    <!-- right sidebar -->
    <%= yield :right_sidebar %>
  </tr>
</table>

Ignore for a second the use of a table for layout (that requires a separate post!) Can you see the design flaw?

Yes, the flaw is that all pages that want to add something to the right sidebar must remember to wrap it in <td valign="top">...</td>. Like this:

<% content_for :right_sidebar do %>
<td valign="top">
  BANNER GOES HERE
</td>
<% end %>

In practice the code that goes in the right sidebar is dependent on the context where it is used. The solution in this case is simple: keep the context in the calling code, not in the called code. Like this:

<table>
  <tr>
    <!-- main application content -->
    <td valign="top"><%= yield %></td>

    <!-- right sidebar -->
    <td valign="top">
      <%= yield :right_sidebar %>
    </td>
  </tr>
</table>

This has the problem that if the page does not want to put anything in the right banner, it will still have an empty td element. Fortunately, Rails has a clean solution for this:

<table>
  <tr>
    <!-- main application content -->
    <td valign="top"><%= yield %></td>

    <!-- right sidebar -->
    <% if content_for? :right_sidebar %>
    <td valign="top">
      <%= yield :right_sidebar %>
    </td>
    <% end %>
  </tr>
</table>

The code that uses the right sidebar is now simplified to

<% content_for :right_sidebar do %>
  BANNER GOES HERE
<% end %>

And this code will not have to change when the application layout is changed to something that does not use tables.

Watch how you cut!