avatar

Vít Meloun

Posted on 13th August 2019

Rails PRO tips: #2 DRY?

news-paper News | Software Development |

In the first article, we discussed why the “Rails Way” can be considered harmful and what to do about that. Within this text, I would like to inspect another concept often being mentioned when you begin to study Rails: DRY (meaning “Don’t Repeat Yourself”).

4 ananas
Photo by Pineapple Supply Co. on Unsplash

Let’s dive straight into the code and imagine you have added this method into your codebase:

def write_to_google_sheet(sheet, rows)
  sheet.write(rows)
end

Works great and about as you would expect: you can have some rows being written to a Google Sheet (the sheet object comes from a Ruby gem, taking care of communicating with Gdocs); simple as that. Everybody in your company loves you, and life is great.

After a few days, your manager comes to you and asks to add support for writing to a Google Document, too. “No problem,” you say, and add this method:

def write_to_google_document(doc, text)
  doc.write(text)
end

Which, again, does what it should and is pretty easy to understand. But when you see those two methods alongside, you notice they have a lot in common: both of them communicate with Google Docs; both accept some data and an object, representing where the data should be written to; and both of them send the write message to this object. It seems repetitive – DRY to the rescue!

def write_to_google(target, data)
  target.write(data)
end

A week after, another request comes in: when writing to a Sheet, sometimes users need to specify the cell writing should begin at. Any instance of the Sheet class accepts an optional argument coordination, a document object doesn’t.

Now, how would you implement this new functionality? Within the existing general write_to_google method, of course:

def write_to_google(target, data, coordination: nil)
  if coordination
    target.write(data, coordination: coordination)
  else
    target.write(data)
  end
end

Then you think about it and realize that what the if statement really does is it distinguishes whether the target is a Document or a Sheet. So why not ask straight for that? Also, let’s move the repetitive target.write to the bottom:

def write_to_google(target, data, coordination: nil)
  arguments =
    if target.is_a?(Sheet)
      {coordination: coordination}
    else
      {}
    end

  target.write(data, arguments)
end

This way, you are prepared for the situation in the future, when the manager comes with a request to write into a Google Slide (with optional layout name); it can be easily done this way:

def write_to_google(target, data, coordination: nil, slide_layout: nil)
  arguments =
    if target.is_a?(Sheet)
      {coordination: coordination}
    elsif target.is_a?(Slide)
      {slide_layout: slide_layout}
    else
      {}
    end

  target.write(data, arguments)
end

Now you have all the logic in one place, non-repetitive, and in the future, when they will want to implement Google Forms writing, you will just add a line and argument for that. Great, right?

Well, not exactly.

Wrong abstraction problem

What we are witnessing here is a deep and very common problem: namely the wrong abstraction (and maybe some object screaming to be discovered, as Sandi Metz would say).

Every time you see ifs like this (checking object type and changing method behavior according to that), you need to stop, take a step back, rethink the whole situation and ensure there is a really good reason for that.

Imagine the mess testing this method. It does so many things; how would you even do that properly?

Imagine further maintaining and extending. Google has 4 document types now, but what if there were 20 of them? Would you add a line for every one? And argument, too? (Yes, you could cheat with a Hash there, but it doesn’t really solve anything.)

And what if you wanted to add a second argument to the Sheet communication? Plus it is not composable in any possible way.

In this example problem we are solving, having a method for each Google document type with some repetitive code would definitely be an improvement over that huge, all-in-one method. For testing, for reasoning about the code, and… it just feels better.

It could stay like that forever, or you could always extract the right common logic – but later. And later means you having more information. Extracting common code into a new place is very easy, but when you create a wrong abstraction by that, the way back is not.

Avoid premature DRYing

Your initial instinct was right: you thought you saw a common abstraction in two methods, so you’ve moved it into one place. What was wrong was that it was premature; there were only two of the solved problem examples, so you didn’t get right what the common abstraction really is. You didn’t have enough information yet, so you didn’t see all the things that are different – and dealing with those made the method a terrible mess.

Instead of immediate, almost automatically DRYing every piece of code you write, I would recommend to suppress the need and tolerate some duplicity in your codebase. It may (and probably will) be temporary: after the 3rd or 4th occurrence, you’ll have a much deeper awareness of what the things they have in common are (and are not), and then you can do it right.

Because code duplicity is much cheaper than a wrong abstraction being introduced to the codebase.

Read the Rails PRO tip #1: The Rails Way