Using Null Object Pattern in Rails to improve code quality

If you have not read my previous post I wrote about Service classes, I would suggest to read that first as it is in continuation to the previous post. This is the link to previous post.

In this post we are going to look at another pattern that helps us in refactoring code, especially helps us in removing conditions. It is called Null Object Pattern.

This code is used to download the data dump based on some rules as always. But here is my 2 cents, do not even try to understand the code.

   def download_measurements
    specification = @inspection.specification
    serial_numbers = @inspection.get_serial_numbers
    inspection_results = @inspection.inspection_results.includes(:inspection_serial_number)
    row_list = []

    row_list << ['unit number', 'serial number', 'gage id', 
                 'inspected by', 'inspected on yyyy-mm-dd', 
                 'place', 'measurement', 'bonus tolerance', 
                 'actual measurement', 'text', 'remark', 
                 'non-conformance number']

    serial_numbers.each_with_index do |serial_number, index|
      specification.feature_repeats.times do |place_index|
        place_number = place_index + 1
        ir = inspection_results.collect{|i| 
             i if i.serial_number.number == serial_number}.
             uniq.compact.first

        specification.readings_required.times do |measurement_index|
          measurement_number = measurement_index + 1
          rm = ir.nil? ? nil : 
            ir.result_measurements.where(:place_number => place_number, 
            :measurement_number => measurement_number).first
          
          inspected_on_date = ir.nil? ? nil : ir.inspected_on.nil? ?
          nil : ir.inspected_on.strftime('%Y-%m-%d') 
          
          measurement_value = rm.try(:actual_measurement).nil? ? 
          nil : rm.try(:actual_measurement)
         
          measurement_value = rm.try(:measurement_result) == 'P' ? 'PASS' : rm.try(:measurement_result) == 'F' ? 'FAIL' : nil if specification.result_type == 'Attr'
          row_list << [index+1, serial_number, ir.try(:gage_serial_number), 
                       ir.try(:inspected_by), inspected_on_date, place_number, 
                       measurement_number, nil, measurement_value, rm.try(:information),
                       ir.try(:remarks), ir.try(:non_conformance_number)]
        end
      end
    end
    file_name = "Template" + "-" + @inspection.balloon + "_#{Time.now.to_i}.xlsx"
    file_path = "#{Rails.root}/tmp/#{file_name}"
    XlsxHelper.generate(row_list, file_path)
     send_file(File.open(file_path), :type => 'application/vnd.ms-excel', :filename => file_name)
   end

We clearly need to refactor because this code is difficult to understand even for the person who has written the code, forget the team who is working on the same project. And since this code is difficult to understand, it is difficult to debug and it is difficult to change as changes will come as they always do.

As I can see that this whole code first of all should not be a part of a controller method. Since the purpose of this code is to generate a sort of data dump in a csv file, I would make it a service class and call the methods of that class as explained in the previous post.

By just glancing at the code, I can understand that there is a header added somewhere in there and then there is some logic to add the data. So I will start by making a service class. And make these empty methods.

class ExportIdeData
  def initialize(inspection)
    @inspection = inspection
  end

  def run!
  end

  private

  def header
  end

  def add_data
  end
end

And my aim is to make controller code look like this:


  def download_measurements
    ide_data_runner = ExportIdeData.new(@inspection)
    ide_data_runner.run!
    generate_file(ide_data_runner)
  end

Now that we have defined a service class and has added methods to it, lets focus on this line of the old code:

  inspected_on_date = ir.nil? ? nil : ir.inspected_on.nil? 
    ? nil : ir.inspected_on.strftime('%Y-%m-%d') 

In this code, we see a complex logic of conditions which decide what value to return. If ir object does not exist, we want the value nil, if ir object exists but no value for attribute inspected_on exists in db, then we want nil. And if the value exists in db, we want to change the format of the date and then return that value.

If I have to apply such conditions to lets say only 1 attribute of the model, I wont mind adding all the conditions for once. But in our use case, when we have to apply such logic to multiple attributes of a model, it won’t just make sense to compromise on the code quality. And thus, Null Object Pattern helps us in cleaning our code.

So these are some of the changes that we make to our new service class.

  def header
    @rows = [['unit number', 'serial number', 'gage id',
              'inspected by', 'inspected on yyyy-mm-dd',
              'place',  'measurement', 'bonus tolerance',
              'actual measurement', 'text', 'remark', 
              'non-conformance number']]
  end

  def add_data(sheet)
    @serial_numbers.each_with_index do |sn, index|
      @place_measurements.each do |pc|
        ir = find_inspection_result(sn)
        rm = find_result_measurement(ir, pc[0], pc[1])
        data = [index+1, sn.number, 
                ir.serial_number, ir.inspected_by, 
                ir.inspected_on_rep, pc[0], pc[1], nil, 
                rm.measurement_value, ir.remarks, 
                ir.non_conformance_number]

        add_row_with(sheet, data, @header_values, 
                     height=27.571428571428573)
      end
    end
  end

Now do not get too much into the code. Just have a quick look at it, this code looks clean.The whole code is not important for you to understand for what we are trying to achieve here, pay attention to these lines:

  ir = find_inspection_result(sn)
  rm = find_result_measurement(ir, pc[0], pc[1])

These are some of the methods that that we need to define:

  def find_inspection_result(serial_number)
    ir = @inspection.results.where(:serial_number_id => serial_number.id).first
         || NilInspectionResult.new
    ir
  end

In the above code, I find the inspection result object and if it fails to find such object in database, then we initialize a new object of the class NilInspectionResult.
Keep in mind these are classes that do have a corresponding database table. It is just a plain ruby class which happen to be created under models folder.

This is how it looks:

class NilInspectionResult
    
  def gage_serial_number
    nil
  end
    
  def inspected_by
    nil
  end 
    
  def inspected_on_rep
    nil
  end
  
  def remarks
    nil
  end
              
  def non_conformance_number
    nil
  end 
end 

So, when we do not find an result object in database, a new object of our newly defined class NilInspectionResult has access to these methods defined above which a valid result object would have.
What I mean here is that remarks, non_conformance_number etc are attributes or methods defined in InspectionResult model and so we define the same methods in our new class also with values we expect them to return.

This also helps us in keeping our code clean but also easy to change. For ex: In future if client wants that instead of returning nil as the date for the case when result object does not exist, return the current date. So I just have to change the method in NilInspectionResult class to return current date and not nil.

Similarly look at this code:

measurement_value = rm.try(:actual_measurement).nil? 
  ? nil : rm.try(:actual_measurement)

‘try’ is just a fancy way to write if else conditions. And it is suggested to avoid such conditions whenever possible. So I would change the code to something like this:

def find_result_measurement(ir, pn, pm)
  rm = ir.result_measurements.where(:place_number => pn, :measurement_number => mn).first 
       rescue NilResultMeasurement.new
  rm
end

Here we find the result measurement object which also depends on the ir object. And in our previous method we have found the result object ir, and it could contain the actual inspection result object or a nil template of it(NilInspectionResult.new).

So in code to find the result measurement object, I use rescue to initialize a NilResultMeasurement object. And I add a class like this:

class NilResultMeasurement
  def measurement_value
    nil 
  end

  def pass_fail_result
    'N/A'
  end 
end

Now I think I do not need to explain why this above code is what it is. But one of the most important thing to remember when using Null Object Pattern is not to overuse it.

So I hope this helps you in avoiding conditional blocks whenever and wherever feasible to use this pattern. And I will say this again, we have not reduced the number of lines of code, we have infact increased the number of lines of code. But we have made our code looks much cleaner, easier to change and understand.

Using Service Classes to improve code-quality

ServiceClasses are nothing but plain old ruby classes. But they go a long way to keep your controllers thin and models thin. To show what I mean by this, lets look at some code. This is the code in controller to download data following some set of rules.

class ImportExportsController < ApplicationController

  def multi_column_download_template
    rows = []
    header = ['Spec Number', 'place', 'measurement',
              'measurement number']

    file_name = "multi-column-template" + "_#{Time.now.to_i}.xlsx"
    file_path = "#{Rails.root}/tmp/#{file_name}"

    specs = @inspection_master.row_no_wise_specs
    specs.each do |spec|
      header << spec.id.to_s + "#" + spec.characteristic
    end
    rows << header

    feature_repeat = specs.collect(&:feature_repeats).max
    measurement    = specs.collect(&:minimum_readings_required).max

    inspection = Inspections::Inspection.new(
                   :feature_repeats => feature_repeat, 
                   :minimum_readings_required => measurement)
    place_cols = inspection.calculate_records

    @inspection_master.inspection_serial_numbers.each_with_index do |d, index|
      rows << [index+1, d.place, d.measurement, d.serial_number]      
    end
  
    generate_and_download_file(rows, file_path, file_name)
  end

  def generate_and_download_file(rows, file_path, file_name)
    #code which generate a xls file and send it as response
  end

end

This is bad. The controller should not look like this. It is not thin. It is difficult to understand. And the bigger problem which is difficult to see the moment we write code is that things will change. Believe me change is the only constant. And the cost of those changes will be much more higher. So comes in Service classes.

The ideology behind service classes is that it should have only one purpose to serve and nothing else.

And I can clearly see a purpose which our new service class should serve and that is to provide the data for our template download and nothing else. I keep the service classes under app/services folder. Also I do not think I need to iterate much on the importance of the name of the service class you choose. It goes without saying that the name should tell the purpose it is going to serve. I will name my service class MultiColumnDownload and I put it under app/services folder. This is how the service class looks like:


class MultiColumnDownload

  def initialize(inspection_master_id)
    @inspection_master = InspectionMaster.find inspection_master_id
    @file_name = "Multi-column-template-" + "_#{Time.now.to_i}.xlsx"
  end

  def file_name
    @file_name
  end

  def file_path
    "#{Rails.root}/tmp/#{@file_name}"
  end

  def run!
    initialize_header
    find_specs
    insert_data
    @rows
  end

  private

  def initialize_header
    header = ['Spec Number', 'place', 'measurement', 'measurement number']
    specs = @inspection_master.row_no_wise_specs
    specs.each do |spec|
      header << spec.id.to_s + "#" + spec.characteristic
    end
    @rows = [header]
  end

  def find_specs
    @specs = @inspection_master.specifications
  end

  def find_feature_repeats
    @specs.collect(&:feature_repeats).max
  end

  def find_minimum_readings_required
    @specs.collect(&:minimum_readings_required).max
  end

  def insert_data
    @inspection_master.inspection_serial_numbers.each_with_index do |d, index|
      @rows << [index+1, d.place, 
                d.measurement, d.serial_number]
    end
  end

end

And now our controller looks like this:


class ImportExportsController < ApplicationController

  def multi_column_download_template
    download_object = MultiColumnDownload.new(@inspection_master.id)
    rows = download_object.run!
    generate_and_download_file(rows, download_object.file_path, download_object.file_name)
  end

  def generate_and_download_file(rows, file_path, file_name)
    #code which generate a xls file and send it as response
  end
end

In the controller, I just have to instantiate an object for our newly created service class and we call this object service object. And call the run! method. Keeping our controller code very thin just the way it should be.

Now this is the code I like and proud of. Not the earlier one. Now in future if I want to change the formatting of this template or add or remove some data, I know where to look for the code and where to make changes. Also I do not have to change anything in my controller code as long as the service class returns data.

Have we reduced the number of lines of code? NO. In fact we have increased the lines of code by at least one and half times.

Have we reduced the complexity of code? YES. Now the code is easier to understand and maintain over long term.

I can only hope that you find this useful and make service objects without any fear of increasing code complexity or number of lines of code. I certainly do..!!!

(p.s. I have reduced the complexity of the controller to not deviate away from what we are trying to understand here.)