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
- Use
- 🤔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
Top comments (2)
Thanks for explaining and sharing your thought process.
I'm happy to see your comment. Thank you too 🙏