How DRY is too DRY?

rubyonrails-talk
1

I'm working on fixing up the code in my first Rails application and have come around to DRYing up the views and partials. For the most part, I'm satisfied with the DRYness of the application, but I have 5-6 partials that display nearly identical XHTML, just with different content, link_to paths, and images.

I know this question is entirely subjective, but would finding a way to further DRY the 5-6 partials into one partial with plugs for different fillers be too much? If so, how would I go about doing this?

2 examples...

http://pastie.org/402754 http://pastie.org/402756

2

Here's how it could look like -> http://pastie.org/402767

And having five pages with almost the same markup is always wrong, you don't even need to be looking for DRYing up your code.

3

Thanks, I'll work through your example.

I'm not sure that I follow your second comment. I understand that having duplicate markup is not desirable, but is there another way I should be approaching this?

4

And what about cases where I need the model attribute names to be different? For instance, in one case I might print item.name, but in the other, item.title. And since this markup will be similar to the front-end display (non-admin), is there a good way to set the link_to to be either :admin or :root?

5

And what about cases where I need the model attribute names to be different? For instance, in one case I might print item.name, but in the other, item.title.

Make all models that are going to be used by that template respond to the same message. If at your template, you're calling "item.title", all models that are going to be used in there should have a "title" method. In your case, you can just alias the "name" method to "title". A better approach would be to implement the "to_s" method in your active record models by returning these "name" properties.

If you have a user model, you could implement it's to_s like the following:

class User < ActiveRecord::Base

  def to_s     login   end

end

So you don't even care about what that model is, you just expect it's to_s method to yield a correct "label".

And since this markup will be similar to the front-end display (non-admin), is there a good way to set the link_to to be either :admin or :root?

You can just add another parameter at the render :partial call.

6

Ok, I follow having all models respond to "title", but why would implementing the model's "to_s" method be a better approach? Is it just for consistency, always expecting to_s to return what the partial should display? How is that any different that expecting all models to answer to title?

Thanks. This has been really helpful. I used your example to create the following. Thoughts?

http://pastie.org/402843

7

ericindc wrote:

I'm working on fixing up the code in my first Rails application and have come around to DRYing up the views and partials. For the most part, I'm satisfied with the DRYness of the application, but I have 5-6 partials that display nearly identical XHTML, just with different content, link_to paths, and images.

This supplements your other answers, which are doubtless technically correct.

The DRY is primarily about the behaviors of methods in an OO design. Having two methods that do the exact same thing, but with different labels or a different algorithm, is the greatest violation of DRY. (Start by seeing if you can make the two methods look more similar, before merging them. If you can't, at least find a way to put them right next to each other!)

Similarly, your metadata should be DRY. This is why (and how) Rails obeys "convention before configuration". Don't write an XML file saying "our unit tests are in the test/ folder". That repeats the information stored in the name of the folder.

Now we come to the heart of the matter. HTML, by itself, is not behavior, and it's almost not structure. It is mostly art. DRYing two similar stretches of HTML is the moral equivalent of DRYing all your #fff and "white" CSS directives. It's okay if they say the same thing, when they don't use logical statements to say them, and when you might not frequently need to change them all at the same time.

So that is the answer to "too DRY". Keep DRYing your project until you reach a local maximum of convenience, and then don't DRY any more.

8

* ericindc <ericmilford@gmail.com> [2009-02-27 18:20:27 -0800]:

I'm working on fixing up the code in my first Rails application and have come around to DRYing up the views and partials. For the most part, I'm satisfied with the DRYness of the application, but I have 5-6 partials that display nearly identical XHTML, just with different content, link_to paths, and images.

I know this question is entirely subjective, but would finding a way to further DRY the 5-6 partials into one partial with plugs for different fillers be too much? If so, how would I go about doing this?

My criteria for *repeat* detection is, what you want when you need to change them. When you DRY up two similar parts, you lose the ability to modify one without affect the other one. So I think if the similar parts are likely to change in the same way in future, they should be DRYed; otherwise I prefer to keep them seperate although they looks simliar to each other.

my $0.02 Jan

9

Ok, I follow having all models respond to "title", but why would implementing the model's "to_s" method be a better approach? Is it just for consistency, always expecting to_s to return what the partial should display? How is that any different that expecting all models to answer to title?

First it's for the sake of consistency, but it's also 'cos the to_s method is defined to return a "string representation of the object", so if you already have a method that does what you want, why create another one or why care about creating another method that "returns a string representation of the object"?

This way you don't need to add extra virtual methods at your models, you just expect them to have a good to_s implementation.

Thanks. This has been really helpful. I used your example to create the following. Thoughts?

http://pastie.org/402843

It's good, but that's how I'd do it -> http://pastie.org/403009

The more conventions you have, the easier it is to keep things consistent in your pages.

10

Thanks for both posts regarding DRY and too DRY. Some very good points.

> Ok, I follow having all models respond to "title", but why would > implementing the model's "to_s" method be a better approach? Is it > just for consistency, always expecting to_s to return what the partial > should display? How is that any different that expecting all models > to answer to title?

First it's for the sake of consistency, but it's also 'cos the to_s method is defined to return a "string representation of the object", so if you already have a method that does what you want, why create another one or why care about creating another method that "returns a string representation of the object"?

Thanks, you've been a huge help.

Makes sense for consistency's sake. My initial concern is about overriding a core method. What if I needed to_s elsewhere? Plus, wouldn't I be writing more code given that every model must now have a custom defined to_s method? Unless adding extra virtual attributes is bad, would it be less work (given that I'd have to change two models at this point) to just add a "title" method for those that doesn't respond to title?

This way you don't need to add extra virtual methods at your models, you just expect them to have a good to_s implementation.

> Thanks. This has been really helpful. I used your example to create > the following. Thoughts?

>http://pastie.org/402843

It's good, but that's how I'd do it ->http://pastie.org/403009

The more conventions you have, the easier it is to keep things consistent in your pages.

I picked up a few good points, but I decided on my implementation because there are other types of those sidebar boxes. For instance, they won't all say "Create New XYZ". Some will say "My Comments", or "Add Member". The images, etc. will change too.

It made sense to me to keep the structure of the box DRY since that will never change and just plug in my values as needed. And the only way I found to do that was what I pastied too. I'll keep looking at it though.

11

Makes sense for consistency's sake. My initial concern is about overriding a core method. What if I needed to_s elsewhere?

Usually, you shouldn't need it unless what you need is a "string representation of your object" (yes, i'm sounding like a broken vinyl now ;D), and receiving a user_name or a title would probably be what you're looking for.

Also, if it's something for debugging purposes in Ruby we use "inspect".

Plus, wouldn't I be writing more code given that every model must now have a custom defined to_s method? Unless adding extra virtual attributes is bad, would it be less work (given that I'd have to change two models at this point) to just add a "title" method for those that doesn't respond to title?

Yes and no. When you say that it's "title", it has to be a method called title, so you're stuck with this "title" thing, if a model has a title and you don't want to use it, you're about to have a problem. Using to_s or a to_label (this one feels clumsy) means your view code don't need to know which method is going to be called, which simplifies changes in the future. Imagine that today, when presenting a user, you show it's full name, but later on there's a request to, instead of showing the full name, show only the login, if you had a full_name call at every user show, you'd have to change them all to login, but if you had oveeriden the to_s method you would just have to change it in one place.

Also, when you override the to_s method you don't need to call it at the view, as you need to call the "title" method:

<%= link_to user, user %>

The link_to helper will automatically call "to_s" at your object, so you don't have to call it by yourself.

12

Ok, I'm sold. I'll update each model's to_s method to respond with the "title" that I want displayed in the partial. Thanks again for your input.