After graduating from TECH :: CAMP, I started working under professional engineers for three and a half months ... I am keenly aware that I have devoted a lot of resources to my seniors, No. 1, yes, ** code review **.
I will write this article with the hope that it will be useful for someone to learn + because I have been devoting a lot of resources.
The language is Ruby on Rails.
First of all, the basics of the basics are the naming of variables and method names that cannot be done at first. This has a significant impact on readability when making changes and when others read the code. (I can't read English, so it's hard to benefit from it)
Look at the code below.
a = "strawberry"
b = 100
As an extreme example, if you use this variable with such a variable name, you will not be able to guess what is in it, and it will be stressful to read the code. Obviously, you should try to name the variables so that you can clearly see the contents of the variables.
product_name = "strawberry"
price = 100
This way, when you use a variable elsewhere, you can speculate on what's inside the variable, and you don't have to go back to the assignment.
Until the time I joined the company, I myself put all the necessary data into an array and routed it around. I didn't care about readability at all, so I used the following element calls without hesitation.
data = [["soccer", "baseboll"], ["taro", "ichiro"]]
#I want to take the name of the child who is doing baseboll
puts data[0][1] #=>baseboll
puts data[1][1] #=>ichiro
If the above description is messed up in the code, only the writer will know it without a lot of effort to understand what the 0th and 2nd elements are. No, after a while, even the writer will not know what it is.
Be sure to use hashes when using this kind of data to make it easier to maintain, to make the code easier for readers and for yourself to understand.
data = {soccer: "taro", baseboll: "ichiro"}
puts data.key("ichiro") #=>baseboll
puts data[:baseboll] #=>ichiro
By writing as above, the reader will be able to clearly understand what the calling element is. Getting a Value based on a Key is easy and vice versa.
This is a review I received when connecting the current iOS app with the Rails backend. The implementation was to send the primary key of one table from iOS, perform a search that spans multiple tables, and then return the resulting one record to the iOS side. At first I thought it would be ** show ** if I wanted to return one record, but I gave up because I couldn't know any information about that table from Swift side ...
So, using the index action, the code I wrote is here (It has been modified + the part that is separated into the model is also aggregated without separating)
def index
render json: Item.includes(item_stores: :item_store_customer)
.find_by(item_stores:{consignment_item_store_customers: {customer_id: (params[:customer_id]}}))
end
At first glance, it doesn't seem to be a problem, but if you think about it carefully, the index action returns a list, so it's strange to write a one-record exploration from the beginning. Even if it is separated as a scope, it will be less reusable, and above all, it will be confusing because the behavior of the index action is strange when seen by others.
As a result, I changed it to the following code.
def index
render json: Item.includes(item_stores: :item_store_customer)
.where(item_stores:{consignment_item_store_customers: {customer_id: (params[:customer_id]}}))
end
I just changed find_by to where, but this looks more correct as the behavior of the index action. When looking at the application as a whole, I want to make a habit of checking whether each behavior conforms to the manners. The behavior on the Rails side is correct, and on the Swift side, you can get it just by extracting the 0th of the array.
This description often seen in Rails controllers
def update
item = Item.find(params[:id])
render json: item.update(item_params)
end
The above description is fine (I'll explain why later), but the find method throws an ActiveRecord :: RecordNotFound exception in the unlikely event that a record is not found.
"It is strange that data is lost at the timing of this update in the first place." "It is not permissible to skip the update request from here and the contents do not exist."
In that case, it may be necessary to raise an exception and grasp the error properly, but if it is possible that the data does not exist, the following description is preferable. think.
def update
item = Item.find_by(params[:id])
render json: item&.update(item_params)
end
First, change find to find_by. If find_by, even if the target record does not exist, nil will be returned instead of raising an exception. And by writing ** item & .update **, if the content of the item is nil, the code after & will not be read.
If the fact that the record does not exist is possible & allowed in the application, it is necessary to properly grasp the movement when it does not exist.
The second thing is not to make the method too bloated.
For example, the following method that calculates the daily wage based on the hours worked and the hourly wage. If the working hours exceed 8 hours, the overtime pay for the excess will be calculated to be 1.25 times.
def calculate_salary(working_time, hourly_wage)
if working_time <= 8
hourly_wage * working_time
else
hourly_wage * 8 + ((working_time - 8) * hourly_wage * 1.25)
end
end
p calculate_salary(8, 100) #=>800
p calculate_salary(9, 100) #=>925.0
p calculate_salary(10, 100) #=>1050.0
Of course, the above code has a problem with Wansaka, but here we will solve the first one. The above is the method to calculate salary. From that point of view, it seems better to throw the processing of the part that asks for the excess overtime pay to another method, and by throwing it to another method and abstracting it, "I wanted to use it elsewhere. You can easily reuse it when you say "!".
So let's better isolate the method ✊
def calculate_salary(working_time, hourly_wage)
if working_time <= 8
hourly_wage * working_time
else
hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
end
end
def excess_payroll(overtime, hourly_wage)
overtime * hourly_wage * 1.25
end
p calculate_salary(8, 100) #=>800
p calculate_salary(9, 100) #=>925.0
p calculate_salary(10, 100) #=>1050.0
As mentioned above, if you separate only the excess salary from the salary, the processing flow will be clearer and the code for calculation will be easier to read.
Next is the argument. When a method receives an argument and performs processing, it is said that conditional branching should be stopped based on the received argument information.
Then what do you do? It is preferable to divide each branching process into methods and confirm the process at the time of calling.
Let's take a look at the payroll code above.
def calculate_salary(working_time, hourly_wage)
if working_time <= 8
hourly_wage * working_time
else
hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
end
end
def excess_payroll(overtime, hourly_wage)
overtime * hourly_wage * 1.25
end
p calculate_salary(8, 100) #=>800
p calculate_salary(9, 100) #=>925.0
p calculate_salary(10, 100) #=>1050.0
If you look at it, conditional branching is done based on the information of working hours (working_time) passed as an argument in the calculate_salary method. Let's subdivide this and distinguish whether it is overtime or not at the stage of calling the method.
def calculate_salary(working_time, hourly_wage)
hourly_wage * working_time
end
def calculation_of_overtime(working_time, hourly_wage)
hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
end
def excess_payroll(overtime, hourly_wage)
overtime * hourly_wage * 1.25
end
working_time = gets.to_i
if working_time <= 8
p calculate_salary(working_time, 100) #=>working_time = 8 A.800
else
p calculation_of_overtime(working_time, 100) #=>working_time = 9 A.925.0
end
In the first place, it was possible to branch whether the time was exceeded or not before calling the method, and to confirm the processing of one method before calling.
Next is the magic number. A magic number is a number that suddenly appears in a program and you do not know what you intend to do. I personally think it's close to the index number, and if this also gets messed up in the code, the next reader will say "I don't understand."
Let's take the payroll program we used earlier as an example.
def calculate_salary(working_time, hourly_wage)
hourly_wage * working_time
end
def calculation_of_overtime(working_time, hourly_wage)
hourly_wage * 8 + excess_payroll(working_time - 8, hourly_wage)
end
def excess_payroll(overtime, hourly_wage)
overtime * hourly_wage * 1.25
end
working_time = gets.to_i
if working_time <= 8
p calculate_salary(working_time, 100) #=>working_time = 8 A.800
else
p calculation_of_overtime(working_time, 100) #=>working_time = 9 A.925.0
end
Isn't the mysterious numbers in the program that stand out suddenly "8", "1.25", and "100"? The proliferation of these things in the program makes it difficult to understand the extent of the impact, and changes can be laborious and risky.
Then what should we do? Wrap it in a method and give your magic number a name.
def regular_working_hours
8
end
def hourly_wage_increase_for_overtime
1.25
end
def hourly_wage_price
100
end
def calculate_salary(working_time, hourly_wage)
hourly_wage * working_time
end
def calculation_of_overtime(working_time, hourly_wage)
hourly_wage * regular_working_hours + excess_payroll(working_time - regular_working_hours, hourly_wage)
end
def excess_payroll(overtime, hourly_wage)
overtime * hourly_wage * hourly_wage_increase_for_overtime
end
working_time = gets.to_i
if working_time <= regular_working_hours
p calculate_salary(working_time, hourly_wage_price) #=>working_time = 8 A.800
else
p calculation_of_overtime(working_time, hourly_wage_price) #=>working_time = 9 A.925.0
end
By doing this, what number will it be when another person reads it later? You can look at the variables and say, "Well, hourly wages!" Without asking. Also, if you want to change the hourly wage, you only need to change the number in "hourly_wage_price", so you can reduce the risk of omission of change at the same time.
I often do this myself, and I have been pointed out many times. For example, the code below.
def index
item_list = Item.all
render json: item_list
end
Somehow it looks good, but it just returns the data returned to the DB and the data as it is. If you want to output the acquired data or the assigned item without using it in the subsequent processing, use it as it is without making unnecessary assignments.
def index
render json: Item.all
end
This is pretty refreshing.
This was also pointed out once. I don't know if the reviewers are in an environment where they can execute the implemented functions. In an environment where it can't be executed, you need to look at the code and think in your head whether the process will work. In order not to overwhelm the reviewer and not let them think about unnecessary things, be sure to include a test to set the index of "movement" in the review.
I think this depends on the time and the situation, but if the project is mature to some extent (test standards, policies, etc. are set), "If this test passes, the standards are met. It will be a judgment material for "I'm not there".
Anyway, the review request is a fundamental thing to make it a place to see the quality of the code.
If you look at the part that uses each method and it seems to be replaced by map, use map as much as possible. Maps are better than each in terms of performance and readability.
#each
int_list = (1..3).to_a
int_to_double = []
int_list.each do |i|
int_to_double << i * 2
end
p int_to_double
#=>[2, 4, 6]
#map
int_list = (1..3)
int_to_double = int_list.map do |i|
i * 2
end
p int_to_double
#=>[2, 4, 6]
You don't need to prepare an empty array to store it in the array, so you can draw it quite neatly.
When reviewing the code, be sure to incorporate the points you receive into yourself. Those who review it bother to stop and read the code. Take any suggestions seriously and respond in good faith.
Recommended Posts