A PR has been submitted

Kudos to you, person who contributed to a Ruby repository. I am actually thankful that you did not just create an issue but you actually tried to solve the problem by creating a Pull Request. Now my job, is 10x easier since I don’t have to stop the other things that I am trying to do.

But actually, a reviewer has a big job. A reviewer has to care, a lot, about the repository and the health of the code base. Not saying that I care about the repository the most and I am being too picky of how Ruby code should be written. But in general, yes, I do care about the code that you want me to merge to fix your problem. And here’s why:

You love instance variables

Yes, I love instance variables too, but it’s more like “I love that I can use instance variables but if I can avoid it I would”. In most programming languages, instance variables are a good way to pass down an object around. With the right usage, it can definitely make a class smaller. But here is my problem

@car.name # => NoMethodError: undefined method `name` for nil:NilClass
car.name # => NameError: undefined local variable or method `car` for main:Object

Did that give you the hint? Yes, an instance variable returns nil! So what would happen if it was interpolated inside a string?

"That #{car} is so fast! I should get one of those" # => NameError: undefined local variable or method `car` for main:Object
"That #{@car} is so fast! I should get one of those" # => "That  is so fast! I should get one of those"

This would be a big problem wouldn’t it? Silent failures are hard to find. Inside a string, ruby calls to_s on the object. And nil.to_s is just an empty string.

You access hashes with Hash[key]

Yes, I know, it works as well. And yes, you avoid a lot of errors in doing so. But actually, it is better to know where the error started. Think about a scenario where there is something that you want to access and its a key inside a hash inside another hash (oh the nightmare of hash-ception).

# nightmare hash from hell
nhfh = {a: {b: {c: {d: "canada"} } } }

# worst way to do it
nhfh[:a][:b][:c][:d] # => "canada"
# slightly better if you are using ruby 2.3
nhfh.dig(:a, :b, :c, :d)

API’s change. Everything changes all the time. Our apps tend to grow up to be nasty children who don’t tell us what’s wrong if we don’t teach them to tell us what’s wrong when something aweful happens. If for example I changed the API of the hash to something more meaningful nhfh = {company: {person: {address: {country: "canada"} } } }, can you guess what happens to the other accessors I have used? Yes, it returns nil.

So you want to know how to solve this eh? Silver medal solution will be to use a better hash accessor fetch.

nhfh = {a: {b: {c: {d: "canada"} } } }
nhfh.fetch(:a)
    .fetch(:b)
    .fetch(:c)
    .fetch(:d) # => "canada"

# API change
nhfh = {company: {person: {address: {country: "canada"} } } }
nhfh.fetch(:a)
    .fetch(:b)
    .fetch(:c)
    .fetch(:d)  #=> KeyError: key not found: :a

Notice how this stops from the first accessor that failed. It fails loudly and asks its creator to change the accessors. But I did say this is a silver medal solution. It’s great and it is far better than silent failures, but it is still not the best solution. If this hash is a JSON returned by an outside API, it would be better to create a wrapper class to be consumed by your app

class CompanyWrapper
  def initialize(params)
    @company = params
  end

  def address
    person.fetch(:address)
  end

  def person
    company.fetch(:person)
  end

  def company
    @company
  end
end

Notice the order of my methods. Define the method below its callee which brings me to my next point

Avoid spaghetti code

Every class should read like a story where the story that should finish on the first method, and as you go further, the less you should care about the following story. If done right, the methods clear and succinct, ordered with intent to be understood, then the reader would only need to read the first method of the class.

class BadSendEmail
  include Virtus.model

  attribute :user_id, Integer

  def call
    send_email! && increment_email_service!
  end

  private
  def user
    User.find(user_id)
  end

  def company
    user.company
  end

  def send_email!
    EmailService.new_email(user_email).deliver_later
  end

  def user_email
    user.email
  end

  def increment_email_service!
    company.sent_emails.increment!
  end
end

How was your experience reading and understanding through the code? To be honest, when reading through this code, I’m not sure if it is working since everything is jumbled together. There is no direction on how the class tells the story. Once there is a bug in it, it is harder to find it as well because the code is harder to reason about

class GoodSendEmail
  include Virtus.model

  attribute :user_id, Integer

  def call
    send_email! && increment_email_service!
  end

  private
  def send_email!
    EmailService.new_email(user_email).deliver_later
  end

  def increment_email_service!
    company.sent_emails.increment!
  end

  def user_email
    user.email
  end

  def company
    user.company
  end

  def user
    User.find(user_id)
  end
end

Although it is valuable to read the whole service object, you can just stop at the call method to know what the service does. But if you read further, we see the definition of the methods that was called directly above it. The reader would understand more and more of the code as they read a long.

As a collaborator, understand that when someone reviews your code, it is not just because it looks better but also because you are not the one full-time maintaining it. The decisions you make towards your solution will be harder to remove once it is implemented. It is much easier to add code than to remove code without breaking any change. But when something breaks, our code should be vigilant. We should be alarmed that a breaking change happened and our errors guide us on to where we should apply our fixes.