Let’s discuss an example of small refactoring that we recently did while reviewing one of the pull requests.

The advantages of refactoring can be easily explained using the example of Easter. When you have a larger family and need to peel 30 eggs, it can be a pain in the ass. But it doesn’t have to. It depends on how they were cooked. If the water was salted and the eggs were soaked in cold water – the whole job will not take more than 15 minutes, because the shell will come off easily. However, if you have to deal with small pieces of eggshell that stick to the egg whites, it will take an hour.

Before

Let’s think about a simple data model. We have document which has one or more agreement. In order not to cram everything into the model file we create a service class which deals with removing unsigned documents.

class AgreementsDestroyer
  def initialize(document:, agreements:)
    @document = document
    @agreements = agreements
  end

  def destroy
    destroy_agreements
  end

  private

  def destroy_agreements
    MultiJson.load(@agreements.to_json).each do |key, value|
      next if value.present?

      @document.granted_form_agreements.where(form_agreement_id: form_agreement_ids(key.to_s)).destroy_all
    end
  end

  def form_agreement_ids(agreement_type)
    FormAgreement.where(agreement_type: agreement_type).pluck(:id)
  end
end

What’s wrong with this code?

1. Unnecessary JSON processing: In the first code example, the @agreements is already an array. This unnecessary processing may introduce additional performance costs. However, there is no need to serialize and deserialize the data in this case.
2. Unnecessary auxiliary methods: An auxiliary method form_agreement_ids helps us name what the key variable is but it does not make the task any easier. Additionally, there are a lot of methods crammed into line 17, which makes the code harder to read and debug.
3. No database query optimization: Each call to the destroy_all method causes a separate database query. This can lead to excessive database load, especially when a delete operation must be performed on many items. In addition, we have a query that is executed repeatedly on FormAgreement.
4. Clueless code: The code is a bit unreadable in the loop that iterates over @agreements. The next if value.present? condition is misleading. It is also unclear what value is because it is not named as an appropriate variable.
5. One line is too long: Notice that it is not visible even in the preview. Lines can almost always be shortened.
6. Redundant naming: the main method is called destroy (when the class is a Destroyer). Sometimes it happens when making builders that you write build. It’s better to just do call.

First refactoring

class AgreementsDestroyer
  def initialize(document:, agreements:)
    @document = document
    @agreements = agreements
  end

  def call
    destroy_agreements
  end

  private

  def destroy_agreements
    @agreements.each do |agreement_type, is_checked|
      next if is_checked

      @document
        .granted_form_agreements
        .joins(:form_agreement)
        .where(form_agreements: { agreement_type: agreement_type })
        .destroy_all
    end
  end
end

What do we have here?

1. Code simplification: Refactoring simplifies the structure of the code, leading to greater readability and understanding. After refactoring, the code is more concise and clear, making it easier to maintain and develop in the future.
2. Remove unnecessary processing: The original code used the MultiJson.load method to serialize and deserialize the object @agreements. In the refactored code, you can directly iterate over the passed array @agreements, thus eliminating unnecessary processing.
3. Database query optimization: The refactored code uses joins (.joins) and condition (where) directly in the database query, instead of calling an additional method form_agreement_ids. This allows you to reduce the number of database queries and process data more efficiently.

Second refactoring

Could you want more? Of course.

class AgreementsDestroyer
  def initialize(document:, agreements:)
    @document = document
    @agreements = agreements
  end

  def call
    document
      .granted_form_agreements
      .joins(:form_agreement)
      .where(form_agreements: { agreement_type: agreement_types_to_delete })
      .destroy_all
  end

  private
  
  attr_reader :document, :agreements
  
  def agreement_types_to_delete
    agreements.reject { |_type, is_checked| is_checked }.keys
  end
end

Here are some advantages of this version:

1. Simpler logic: In this version of the code, instead of iterating through @agreements and checking the value of is_checked for each contract type , the reject method was used to reject contracts that are not selected. This leads to more concise and readable logic.
2. Direct key retrieval: Instead of converting @agreements to an array of keys as in the second example, this version of the code directly retrieves the keys by using the keys on the returned result from the reject method. This leads to more concise code.
3. Database query optimization: This version of the code also uses database query optimization, combining granted_form_agreements with form_agreements and executing one query with the where condition. This ensures that contracts are deleted more efficiently, especially if you need to delete them frequently and in large quantities.
4. Using methods that return instance attributes: Referring to instance variables via @ is readable in the constructor of a class. However, doing this later on is problematic because a typo may return nil. Anyway, you could go a step further here and use the attr_accessor method to set instance variables like this: self.document = document. However, it is customary to use @ in the constructor
5. Call does the job: It often happens in our code that a call is almost an alias to some private method that contains the service logic. Calling individual methods in call makes sense if the algorithm of a given class is more extensive. In such a case, there is no point in multiplying entities. In turn, the private method agreement_types_to_delete makes sense and would unnecessarily lengthen the call method if its value was assigned to a variable.

Summary

Code refactoring of AgreementsDestroyer is a good example of attention to detail and developing programming skills. While it may seem like small changes don’t matter, it’s details like these that make the difference between good and great code.

Attention to detail in your code is crucial to its readability, scalability, and performance. Small changes can significantly affect the final result and software quality.

When you are a beginner, paying attention to such details is extremely important. It’s not about pettiness, but about developing the skills of analytical thinking, code refactoring and optimization. Improving the structure of the code, removing unnecessary operations and optimizing queries are skills that will accompany you throughout your programming career.

Remember that as a programmer you create software that will be used by other people. Attention to detail is proof of your professionalism. Big things start with small details, so it’s worth paying attention to every aspect of the code and striving for perfection. Especially on Saturday mornings, when coffee tastes best.

(I love how ChatGPT sums up the refactoring message in its enthusiastic AI Spirit).