Refactor code doesn't look like Ruby

The first code

The code I've reviewed is like this. I got many questions.

class CompanyController < ApplicationController
  def my_fields
    data = [] # 🤔 1. why it's initialized?
    my_fields = MyFieldMaster.all
    my_fields.each do |field| # 🤔 2. Unclear the intention of the author
      parent_id = if field[:parent_field_id] == 'DEFAULT'
                    nil
                  else
                    field[:parent_field_id]
                  end
      # 🤔 3. this each block has two of responsibility. The first one is if/else.
      # 🤔 4. The second is to make an array data includes hashes.(This could be the main purpose in this each block.
      data << { 
        id: field[:field_id], name: field[:field_name], parentId: parent_id
      } # 🤔5. And Why field[:field_id]? Isn't it attribute?
    end
    render json: data, status: :ok
  end
end

The first refactoring

• 🤔1. Stop initialize
  • Use .map instead of .each
• 🤔5. Use method instead of field[:field_id]

class CompanyController < ApplicationController
  def my_fields
    my_fields = MyFieldMaster.all
    data = my_fields.map do |field|
            parent_id = if field.parent_field_id == 'DEFAULT'
                          nil
                        else
                          field.parent_field_id
                        end
            { id: field.field_id, name: field.field_name, parentId: parent_id }
          end
    render json: data, status: :ok
  end
end

The second refactoring

• 🤔 2. make it clear the intention of the author by extract not the main purpose.
  • 🤔 3. it should be extracted to a method
  • 🤔 4. only this process remains.

# 🦄 Extracted code
class MyFieldMaster
  def masked_parent_field_id
    field.parent_field_id == 'DEFAULT' ? nil : field.parent_field_id
  end
end

# 🦄 Original code which is refactored
class CompanyController < ApplicationController
  def my_fields
    data = MyFieldMaster.all.map do |field|
            { id: field.field_id, name: field.field_name, parentId: field.masked_parent_field_id }
          end
    render json: data, status: :ok
  end
end

Comments

[Godfrey C]

Thanks for explaining and sharing your thought process.

[n350071🇯🇵]

I'm happy to see your comment. Thank you too 🙏