Ruby/Rails Code Smell Basics 04

Code smells and their refactorings can be very daunting and intimidating to newbies. So in this series, I’ve tried to make them easy to understand, both for slightly experienced Ruby developers and starters alike.

This final article mentions a few more smells you should look out for and sums up what this small series wanted to achieve. A final whiff, if you like…

Topics

• Comments


• Callbacks


• Bad Names


• Mixins


• Data Clumps

A Final Whiff

The last article in this series is something like a bonus round. I wanted to introduce you to a few more smells that can be addressed quickly and without much fuss. One for the road, so to speak. I think with the knowledge you’ve gathered from the previous articles, most of them won’t even need code examples to wrap your head around.

When you open a book about refactoring, you will easily find more smells than we have discussed. However, with these major ones under your belt, you will be well prepared to deal with any of them.

Comments

Generously applied comments are rarely a good idea—probably never. Why not? Because it might suggest that your design is not speaking for itself. That means your code is probably so complicated to understand that it needs literal explanations.

First of all, who wants to go through hoards of text in your code—or worse, through code that is hard to understand. Jackpot if both are a common occurrence. That’s just bad form and not very considerate of people who come after you—no offence, masochists, torture your future self all you want.

You want to write code that is expressive enough in itself. Create classes and methods that speak for themselves. In the best scenario, they tell a story that is easy to follow. That is probably one of the reasons conventions over configurations became so influential. Reinventing the wheel is certainly sometimes a good practice to sharpen your understanding and to explore new territory, but in fast-paced development environments, your colleagues are looking for clarity and quick navigation—not only within your files but also within the mental map you create in your code.

I don’t want to drift off into a whole new topic, but naming plays a big role in all of that. And excessive commenting within your code slightly contradicts good naming practices and conventions. Don’t get me wrong, it’s fine to add comments—just stay on the path that “illuminates” your code rather than distracting from it. Comments should certainly not be instructions for clever code that mostly you can decipher because you wanted to show off. If you keep your methods simple—as you should—and name everything with consideration, then you have little need to write whole novels in between your code.

Stay away from the following:

• Todo lists


• Dead code commented out


• Comments in method bodies


• More than one comment per method

It’s also useful to break out parts of methods via extract method and giving this part of a method a name that tells us about its responsibility—rather than have all the details clutter up a high-level understanding of what’s going on within the method’s body.

def create_new_agent
...
end

...
# create new agent
visit root_path
click_on     'Create Agent'
fill_in      'Agent Name', with: 'Jinx'
fill_in      'Email',      with: '[email protected]'
fill_in      'Password',   with: 'secretphrase'
click_button 'Submit'
...

What is easier to read? A no brainer of course! Use the free mileage you get by naming things properly via extracted methods. It makes your code so much smarter and easier to digest—plus the benefits of refactoring in one place if reused, of course. I bet this will help trim down your comments by a very significant amount.

Callbacks

This is a simple one. Don’t use callbacks that are not related to persistence logic! Your objects have a persistence life cycle—creating, saving and deleting objects, so to speak—and you don’t want to “pollute” that logic with other behaviour like the business logic of your classes.

Keep it simple, remember? Typical examples of what to avoid are sending emails, processing payments and stuff. Why? Because debugging and refactoring your code should be as easy as possible, and messy callbacks have a reputation of interfering with these plans. Callbacks make it a bit too easy to muddy the waters and to shoot yourself in the foot multiple times.

Another problematic point about callbacks is that they can hide the implementation of business logic in methods like #save or #create. So don’t be lazy and abuse them just because it seems convenient!

The biggest concern is coupling of concerns, of course. Why let the create method of SpectreAgent, for example, deal with the delivery of a #mission_assignment or something? As so often, just because we can do it—easily—doesn’t mean we should. It’s a guaranteed bite in the ass waiting to happen. The solution is actually pretty straightforward. If a callback’s behaviour has nothing to do with persistence, simply create another method for it and you’re done.

Bad Names

Bad naming choices have serious consequences. In effect, you are wasting other people’s time—or even better your own, if you have to revisit that piece of code in the future. The code you write is a set of instructions to be read by you and other people, so a purely logical, super prosaic, overly clever, or worse, a plain lazy approach to naming things is one of the worst things you can leave behind. Aim to make your code easier to understand by providing better names.

Clarity trumps false cleverness or unnecessary conciseness any day of the week! Work hard on naming methods, variables, and classes that make it easy to follow some sort of thread.

I don’t want to go as far as to say that you should aim for trying to tell a story, but if you can, go for it! Machines are not the ones who need to “read” your code—it’s run by them, of course. Maybe that’s one reason why the term “Software Writer” has been growing on me a bit lately. I’m not saying that the engineering aspect should be diminished, but writing software is more than writing soulless instructions for machines—at least software that is elegant and sparks joy to work with.

Don’t freak out if this turns out to be a lot more difficult than you thought. Naming is notoriously hard!

Mixins

Mixins are a smell? Well, let’s say they can be smelly. Multiple inheritance through Mixins can be useful, but there are a couple of things that make them less useful than you might have thought when you started out with OOP:

• They are trickier to test.


• They can’t have their own state.


• They “pollute” the namespace a bit.


• It’s not always super clear where functionality comes from—since it’s mixed in.


• They can inflate the size of classes or the number of methods drastically. Small classes rule, remember?

I suggest you read up a bit on “Composition Over Inheritance”. The gist of it is that you should rely more on reuse of your own, separately composed classes than on inheritance or subclassing. Mixins are a form of inheritance that can be put to good use but also something you should be a bit suspicious about.

Data Clumps

Watch out for repeatedly passing the same multiple arguments into your methods. That often suggests that they have a relationship that can be extracted into a class of its own—which can in turn often drastically simplify feeding these methods with data by reducing the size of arguments. Whether it’s worth introducing a new dependency is the thing you have to weigh, though.

This smell is another form of subtle duplication that we can handle better. A good example is passing a long list of arguments that make up an address and credit card info. Why not package all of this in an existing class, or extract a new class first and pass in the address and credit card objects instead? Another way to think about it is having a range object instead of a start and an end. If you have instance variables that fall for that smell, then extracting a class is worth considering. In other cases, a parameter object might offer the same quality of abstraction.

You’ll know that you have achieved a small win if your system is easier to understand and you found a new concept—like credit card—that you could encapsulate into an object.

Final Thoughts

Congratulations! You have leveled up your OOP skills significantly! Boss level status is approaching. No, seriously, great job if this whole topic was rather new to you!

As a final recommendation, I want you to take away one thing. Please remember that there is no recipe that will always work. You will need to weigh every problem differently and often mix different techniques to fit your needs. Also, for the rest of your career, this is most likely something you’ll never stop struggling with—I guess a good struggle, though, a creative and challenging one.

This is a little guess, but I feel that if you understood most of the topics we covered, you’ll be well on your way to writing code other developers like to discover. Thanks for your time reading this little series, and good luck becoming a happy hacker!