Through my years of working with Ruby on Rails, I've seen some common practices that I believe are not required and add complexity to the code base.
This really simple practices look like they do no harm, but can make code less readable, add verbosity and insert unnecessary code.
One expression methods
Methods are supposed to encapsulate a set of processes or expressions, but sometimes they are wrongly used to define a single logic unit. Most of the time, developers create this methods to make the code more readable.
Consider the next example:
class User < ApplicationRecord
before_save: setup_user
def setup_user
update_complexity
end
def update_complexity
self.complex = true
end
end
After looking at the example above, think about why should we separate the logic in two different methods when we can just move the assignation inside of the setup_user
method.
def setup_user
self.complex = true
end
The first example is easier to read for the most of us, but it adds complexity to the model by adding one more method that is just redirecting to another one without doing anything else.
The second example is readable enough to quickly understand the code intentions and keeps the model clean.
Devs should always account for readability but also keep complexity in mind, it is easier to read a one line raw statement instead of dealing with fat models.
Prefixing the model name when naming methods, variables and constants
Naming variables, constants and methods can be such a basic but painful thing to do, the name you choose is not always understandable enough for others. It is not great also when names mean more that they should.
See the next example:
class User < ApplicationRecord
USER_DEFAULT_ROLE = 'admin'
...
end
In the example above we define the user's default role as a constant, we are being very specific in the name so we know that the default role belongs to the user model, but... why?
If we call this constant from outside the scope of the model, we would call it like this User::USER_DEFAULT_ROLE
, and as you can see, we are repeating the word "User". It may not look like it can affect the readability of the code, but imagine having several constants in the same model or just having long model and constant names...
class CompanyConciliationFee < ApplicationRecord
COMPANY_CONCILIATION_FEE_DEVELOPMENT_COST_RATIO = 0.59
...
end
Look at how long it would look if we call this invented constant from outside the model scope!
MaintenanceFee::COMPANY_CONCILIATION_FEE_DEVELOPMENT_COST_RATIO
Yeah, the example might look like I wrote the first thing that came to my mind... but that's basically how naming works.
Another reason people do this is because they are thinking to use the constant within the scope of the model, where it wouldn't need to prefix the class name. This may be more straightforward but remember that all constants are always associated to the model they are defined into.
Prefixing the model or class can happen with constants, variables and methods, and even when they feel necessary, keep in mind that they will clutter your code.
Endless class reference
Endless class reference refers to something similar to one expression methods but through classes.
Sometimes we break down the code structure to what we think are the necessary classes to get the job done, I.E. services, presenters, PORO's or whatever type of code encapsulation we can use, but sometimes we lengthen the logic by using more than we need, and therefore increase complexity.
Look at the next example:
class UserGenerator
def initialize(user)
...
end
def call
UserAvatarUploader.new(user, avatar).call
end
def avatar
...
end
end
class UserAvatarUploader
...
def call
AvatarUploader.new(:user, user, avatar)
end
end
class AvatarUploader
...
def call
user.update(avatar: avatar)
end
end
If you follow the process to create a user and upload an avatar, you can observe that we are actually using 3 different services to do one simple thing, upload an avatar.
This example is a very basic and simplified version of what happens in production code bases that are used by thousands of people every day. User's don't notice, but devs suffer when debugging only to find out that they have to follow an endless class reference to figure out where in the code the avatar is uploaded.
We don't need all those classes. Keep it simple... if you only job is to upload an avatar, follow the KISS principle and keep it in only one service, you don't need the others! It may feel to you that you are decoupling service responsibilities, but what you are actually doing is making your jr. teammates life very unpleasant one.
And there it was... a few cases that I often see in Ruby on Rails codebases that can be avoided. What do you think? Do you know about another common simple practice that can be avoided to simplify our code?
Top comments (2)
This is a great article. The larger the RoR app gets you're working on, the more pronounced this issue becomes. Oncall shifts / 3:00am debugging over the years has completely colored my underling coding philosophy. When I'm doing PRs, one of the questions I always ask myself is, "Can I debug this at 3:00am on a weekend while partially drunk?" lol! If the answer is no, then maybe a more KISS approach is needed.
Hahaha, I like that approach! Definitely while debugging you can identify big chunks of code that can use a refactor and follow the KISS approach.
I personally check the complexity of the bug first, and reverse engineer the process to identify the source... If by the time I found the source of the bug I already forgot how and where in the code I started, then I propose a refactor or at least add some comments to guide the process.