How do I smell Ruby code?

Understanding the worst of code

This guest post is by Timon Vonk, who is a self-employed Ruby enthusiast and standard nerd with an edge. He has worked with Ruby for several years, but is well-known with many other (programming) languages. Also likes martial arts, loud music, varying quantities of booze and a good scotch.

Introduction

Writing bad code isn’t a bad thing. Not understanding the problem you’re trying to solve any better after having written that piece of code is. Fortunately, that happens far less often. In this article I hope to give a better understanding of Ruby code by going into Ruby specific code smells. We’ll start with some simple examples that are common in all programming languages – they just need to be covered – and then dive into some Ruby specific smells.

So what is that smell?

The term was coined in the 90s by Kent Beck on WardsWiki (one of the first wikis around) and has been popularized ever since. A code smell is a hunch, not necessarily measurable, that the code you’re looking at can be improved in some way. This process of improvement is called refactoring, as you might know. And as far as refactoring is concerned, there is no time like now, don’t leave open ends; it’s a bad habit.

The Basics

Let us give a quick rundown on the more basic code smells:

• Duplicated code, if you see any, is almost always a bad thing. We’ll get into this part a little later.
• Multiple method / class responsibility is always a bad thing. Try factoring out your solution in multiple methods. It will make you’re code more readable and a lot better maintainable. Large methods and classes are a dead giveaway for this as well.
• A class should never use more methods from other classes than from itself. Why is it even there?
• A child class should always honor the contract of the parent class, i.e., be a kind of that class. Check out the Liskov substitution principle for more information.
• If a class hardly does anything, why is it there?
• Does your solution have a more simple approach? Complexity can be a reason of pride for some – if not most – coders, but too much makes it terrible to understand, especially later on.
• Non-descriptive or too long identifiers or names are a good sign that either you can’t define the responsibility of the code, or you have a hard time with naming conventions.

Simple, easy smells should give a good start on fine tuning your code. However, every language has its own specific quirks. Let us take a look at Ruby.

Calling eval on user input or unchecked code

input = "'rm -rf /'"
klass =  eval input

Of course this is bad. I hope it doesn’t need any explanation. Try not to use eval, but instance_eval instead. And if you use either, make sure that the code you eval is secure — never eval user input directly.

Nested blocks without added value

array = [["banana", "apple"],["pineapple","beer"]]
# And I want to call reverse on each element, I could do
array.collect { |sub_array| sub_array.collect { |subsub_array| subsub_array.reverse }}
# But this is much nicer
array.collect { |sa| sa.collect(&:reverse) }
# => [["ananab", "elppa"],["elppaenip","reeb"]]

The reason is simple enough. Your code is more readable, and that’s what we all want. So what about nested multi-line blocks? Check it out, big chance your solution is the root of this particular evil.

Code similarity

def post_to_site(data)
  url = build_url(data)
  response = RestClient.post(url)
end

def get_from_site(data)
  url = build_url(data)
  response = RestClient.get(url)
end

def delete_from_site(data)
  url = build_url(data)
  response = RestClient.delete(url)
end

You can easily solve this lump of code by introducing some meta-programming:

def  response_from_site(data, method = :get)
  url = build_url(data)
  response = RestClient.public_send(method, url)
end

This gives you a clean, nicer method. And it’s readable too! Isn’t that nice?

Long, repetitive and cluttering statements

Often enough you have similar parameters that call similar methods. For instance, you might need to check on some parameter and call the associated method. Or even simpler, you might need to check if a certain user input matches your criteria. I prefer a simple rule of thumb, if you’re working with any sort of collection or set, the functional approach is always cleaner, more simple and most definitely faster. The point is not to dictate when and whether you should prefer the functional approach, just that you understand that long lines of repetitive clutter screw up your code.

Take the following example:

input = "english"
case input
when "english"
  puts "English, ***, do you speak it?"
when "french"
  puts "Baguette!"
when "dutch"
  puts "I only smoke *** when it's free."
else
  puts "Dunno"
end

You can imagine in complex applications that this goes on and on. I actually see it happen a lot and it’s not necessarily bad. However, it’s hard to maintain and in more complex situations it can get really hard to read through. I’ve seen loads of Rails applications where they use just this to check on a particular parameter. Really ugly!

Since it seems like you’re white listing, one way to solve it would be to use a hash with input:result.

whitelist = {
  "english" => "English, ***, do you speak it?",
  "french" => "Baguette!",
  "dutch" => "I only smoke *** when it's free.",
  "other" => "Dunno."
}

if whitelist.has_key?(input)
  puts whitelist[input]
else
  puts whitelist[other]
end

It’s always important to be proud of the code you write. It really helps if it doesn’t smell. And I hope this article helped you do that.

Feel free to ask questions and give feedback in the comments section of this post. Thanks!

You might want to read a related article:

• How does your code smell?