Refactoring: Writing code for clarity

What makes good code is a contentious topic. It's hard to say for sure what exactly good code is without putting it into context. Good code for a spacecraft is very different than good code for your side project. One common theme we can pull from both is clarity. It should be pretty easy to understand what is happening from reading the code.

We focus significantly on code as programmers. This makes sense since we are programmers after all. But this misses the point. Focusing on code is like an author focusing on grammar. Who wants to read a perfectly grammatically correct book? It would be bland and boring. Authors know how to blend and twist the rules. They are there to convey a story. As programmers we need to build a product.

I often code in a way that many programmers would say is pointless or not needed or maybe excessive. And I agree when looked through the lens of "good programming". Most problems we run into aren't programming problems, they are product problems that we try to gloss over with dozens of exception cases.

But let's look at an example. In Rails, the community will often say to avoid default scopes. This recommendation makes sense. They can introduce some unwanted behavior to your app.

Option 1:

class Car < ApplicationRecord
  has_many :wheels -> { where(deleted_at: nil) }
end

We have avoided the default scope here by adding it into the accessor method. This achieves what we probably expect as intended behavior in most instances. The Car is only going to return us the wheels that are not yet deleted.

However when we call car.wheels it obscures behavior. It does not indicate to me from the API of this class that wheels is only undeleted wheels. We can perhaps solve this fairly easily.

class Car
  has_many :wheels
  
  def attached_wheels
    wheels.where(deleted_at: nil)
  end
end

Now we have two methods which better indicate behavior. We could call car.wheels to get all the wheels ever or car.attached_wheels to get the currently active wheels. This is a nice improvement. However, I argue this is still confusing. At some point, some programmer is going to come along and make an assumption. Assumptions lead to bugs. I don't know if there is a saying around that but if not someone should make one. Let's try again.

class Car
  def attached_and_deleted_wheels
    Wheel.where(car: self)
  end
  
  def attached_wheels
    Wheel.where(car: self, deleted_at: nil)
  end
end

At this point the programmers are going to start to push back typically. I've done something "not normal" which was to remove the built in relation. The built in relation introduces a method which could be easily confused as to the functionality. I'd prefer to make the functionality clear even if it might not be "normal". If we look at this class interface we now have two methods car.attached_and_deleted_wheels and car.attached_wheels. To finish up the interface and options you could add in a car.deleted_wheels. At this point, I have taken away all the ambiguity of the interface and left no methods to be "misused".

There could be one more step to go depending on the case. If this was going to be a critical model in my app, I may elect to extract this logic out into its own class.

class WheelFinder
   def self.for(object)
     new(object)
   end
   
   def intialize(object)
     @object = object
   end
   
   def attached
     Wheel.where(car: cars, deleted_at: nil)
   end
   
   def deleted
     Wheel.where(car: cars).where("deleted_at is not null")
   end
   
   def attached_and_deleted
     Wheel.where(car: cars)
   end
   
   private
   
   attr_reader :object
   
   def cars
     if object.is_a?(Car)
       [object]
     elsif object.is_a?(Person)
       person.cars
     end
   end
end

In this example, we expanded the finding logic. We can consolidate the finding of different logic for different models into one testable class. This class can take in two models, a Car and a Person . It can then find all the wheels that are owned by that respective object. We can utilize this like WheelFinder.for(person).attached.

There is one small side effect of this refactor. The method names could drop the noun in the method. Instead of WheelFinder.for(person).attached_wheel , I dropped wheel off the method.

When methods utilize nouns combined with verbs and adjectives I tend to look at that is a code smell. Now, we can't just can't call code that looks bad smelly. If you say something is smelly, also provide the name of that smell. In this case I put this under excessively long identifiers. With this smell, you find yourself adding data to method or variable names which could be solved by restructuring the application to remove the ambiguity.

In this case, we had car.attached_wheels. Because we were operating on a car model we needed to have two words to describe what we wanted. By removing that into the WheelFinder class we introduced clarity in structure. We now are operating on a class which is clearly about finding wheels so we don't need to duplicate that noun in multiple spots.

Now, I get the arguments. I don't need all that! It's more lines! It's way more complex than what the requirements need! And all those arguments are absolutely correct.. when viewing the code through a programmers eyes. Many car enthusiasts out there will say that you don't need a radio in a car, which is true. Some will say you don't need AC, also true. Old cars rarely had roofs so why bother with that? It just adds weight. Do we need seatbelts? How about padded comfy chairs? All of that is true. But the result is no one wants to buy your car. As programmers we often think in bare minimums and then end up with products that no one wants to buy. These products barely run, and leave behind a trail of metaphoric parts falling off. The users get annoyed with bugs and missing small extra features but hey, at least we didn't need that one method and few extra lines of code.. right?