A few weeks ago at our Maine meet-up, David was telling a story about preparing for his latest RailsConf keynote. His plan was to talk about bad code from our own apps, and we were cracking up when he explained his hunt for all that “crusty gold” buried in our repos. I ran into my own nugget of crusty gold today while taking a peek at Basecamp’s footers.
The footer of each page in Basecamp may show a badge that says “Managed with Basecamp.” I wanted to figure out when and why that badge is displayed. So I found my way to the relevant template and saw this:
<div class="Footer">
<% if @account.free? || @account.show_footer_badge?%>
<a href="<%= @account.free? ? "http://www.basecamphq.com/?ref=free" : affiliate_link %>" target=_blank>
<img align="right" width="112" height="30" src="/images/poweredbybasecamp.gif" alt="Powered by Basecamp" border="0" align="absmiddle" style="margin-right: 8px;" />
</a>
<% end %>
</div>
The first order of business was the question: Under what conditions do we show the badge? The first condition, @account.free? is clear enough (it means the account is on a Free plan). But what’s up with the second condition: @account.show_footer_badge?
Since the method is on @account, the first place I checked was our Account model. But it turned out that show_footer_badge? isn’t a method. It’s a column on the database. That meant I had to find where that value was being set by the user and saved into the DB to figure out what it really ‘means’.
Some grepping revealed that show_footer_badge? is actually a relic from our first BC affiliate program. People on this old program had the choice to check a box to hide or show the footer badge to all their employees and clients using their Basecamp account. What’s crusty about this? The problem is that @account.show_footer_badge? gave me no indication that this is really about affiliate settings. A simple change to the name of the method would have really clarified that:
<% if @account.free? || @account.affiliate_wants_to_show_badge? %>
This comes back to a favorite design pattern: Intention Revealing Interfaces. @account.show_footer_badge? says what to do, but it doesn’t reveal the original author’s reason or intention.
To round out this nugget, let’s improve that nasty anchor and image tag from the first excerpt above. We’ll use a Rails helper and some CSS to clean things up:
<div class="Footer">
<% if @account.free? || @account.affiliate_wants_to_show_badge? %>
<%= link_to basecamp_url_with_affiliate_code, image_tag('poweredbybasecamp.gif'), :class => 'affiliate_badge' %>
<% end %>
</div>
The next time we go digging into Basecamp’s footers, this code will be a lot easier to understand.
Related: Behind the scenes: Redesigning and coding the Highrise sidebar modules, What belongs in a helper method?
Tim Connor
on 22 Oct 08Why not a method, similar to the originally misnamed “show_footer_badge?” that just returns the || of those two? It seems cleaner to just have a method in the Account that determines that, rather than more view logic.
Jason
on 22 Oct 08Wouldn’t Account#show_footer_badge? actually confirm if there is an attribute on the Account model, called “show_footer_badge”, (thus always returning true), rather than the actual value of the “show_footer_badge” column, which would be accessed by Account#show_footer_badge (without the question mark)?
I’m sure I’ve been caught out by this somewhere….
RS
on 22 Oct 08Tim, in this case “more view logic” is what we prefer. We could combine those two conditions and move them to a method on the model, but then we would be treating that combination of conditions as business logic instead of view logic. And that’s debatable.
There’s also a cost of indirection if we move the OR condition to the model. The person reading the view would have to dig one level deeper to discover the two very different conditions that cause the badge to be displayed or hidden. I’d rather keep that logic as close to the view as possible for the benefit of the person reading the template.
RS
on 22 Oct 08By ActiveRecord’s conventions, the question mark at the end of the method reflects the fact that the column is a boolean rather than a string or integer etc. It returns the actual value of the column as a boolean.
tosh
on 22 Oct 08I think Tim really has a point,
if you had written the business logic in the model, you could have documented the logic on a central place if it is not immediately clear what happens by only looking at the code.
IMHO for the template logic it is only interesting to know that there is a badge which is shown sometimes according to the self awareness of the account instance.
When I read your example before I read your solution I was expecting that you would move the logic to the model to get rid of some “php without templating system”-ism.
It is great that rails is MVC, and I agree that it is pragmatic to put some logic into the view/template but you have to draw the line somewhere and I would have drawn it differently.
Still I am interested in more code reviews and decisions, there is certainly an opportunity to learn something here, so thank you for posting this.
Ismael Celis
on 22 Oct 08Since both methods are called on the account object (yes one is actually a reader for a DB column but a method nontheless) and both tell the view wether or not to show the badge, I would have put them in a single model method as well. From the point of view of the designer, he/she only needs to know wether that account has a badge to display or not, regardless of the internal (and possibly changing) logic behind that decision.
RS
on 22 Oct 08We don’t see it that way at all. We think it’s the designers responsibility to know declaratively which conditions change the state of the view. By moving view logic into the model, you lose separation of concerns, and you take that logic out of the designer’s hands.
StartBreakingFree.com
on 22 Oct 08Haha, if that’s the crustiest piece of code you’ve got in there then I think you guys are all set!
You should see the relics I’ve got in mine. Thanks! Brian
TJS
on 23 Oct 08Hi all,
Very slightly OT, but I arrived at this post via a Google query: “turn off managed with Basecamp logo.”
I just started seeing the logo shown above in our Basecamp install. Don’t recall ever having been a 37S affilliate, but I’ve been a paid subscriber for several years.
Any guidance as to how I can turn it off? I’ve tried looking in all of the obvious Account Settings to no avail.
Thanks in advance for any guidance.
Anon
on 23 Oct 08Clean Code by Bob C. Martin is a recommended read btw. :-)
Mark
on 23 Oct 08I agree with Tim, tosh and Ismael. The designer should only be concerned with wether or not the model says a badge should appear.
Exposing the designer to the conditions determining badge display is not a separation of concerns. It’s the opposite. If you switch designers you’ll need to teach her your business model before she can be effective.
You also lose orthogonality. What if you eliminate free accounts (shudder)? You’ll have to change your model and your view logic.
I think you are overstating the cost of having to dig through a few files of source code.
If the badge was displaying at the wrong time, and the view template hasn’t changed it’s unlikely the problem lies in the view logic. You’ll call in a programmer first. You don’t want a designer digging around your models (no offense intended toward designers at all).
RS
on 23 Oct 08I agree. Here’s the thing: If you don’t want the designer in the models, and you also think the conditions for showing/hiding view states belong in the model, then you necessarily prevent the designer from deciding which things should be visible at what time on the screen. In other words, you prevent the designer from doing their job. We think it’s the designers job to decide what should display on the screen and when and where.
As tosh said above: “you have to draw the line somewhere and I would have drawn it differently.” That’s a great way to sum it up. We draw the line where we do in order to give designers as much control over the state of the view as possible.
aswl
on 23 Oct 08The most important point for me now is – why is the “managed with basecamp” badge suddenly showing up in my account now. This is particularly annoying if I have to print a page to take to a client. No disrespect but I would like basecamp to be invisible. I am on the Plus plan.
Alisey
on 25 Oct 08“If you think the conditions for showing/hiding view states belong in the model, then you necessarily prevent the designer from deciding which things should be visible at what time on the screen.”
Do you also think that hiding/showing functionality for paid members is a designer’s responsibility and should belong to View? Designer don’t really need to hack with Model to affect the way it works, he can ask, you know.
This discussion is closed.