[RUBY] Approximately three and a half months after joining the company, an output article to feed on the reviews

Why write

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.

Part 1 Name it exactly

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.

Part 2 Do not use index numbers as much as possible

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.

Part 3 When using the index method on the Rails controller, do not write code that assumes that only one record will be acquired from the beginning.

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.

Part 4 When using the update action, it is better to consider the possibility that the target is nil if necessary.

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.

Part 5: Do not do many things with one method

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.

Part 6 Do not make a conditional branch based on the argument information

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.

Part 7: Do not use magic numbers

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.

Part 8: Do not make unnecessary substitutions

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.

Part 9: When requesting a code review, the reviewer is not always in the execution environment, so the test and function implementation are submitted as a set review request.

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.

Part 10 When receiving the result as an array, if the behavior is the same, use map instead of each

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.

Finally

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

Approximately three and a half months after joining the company, an output article to feed on the reviews
How a liberal arts engineer passed Java Silver in half a year after joining the company
A story about three and a half months inexperienced programming who created an "engineer checker" and eradicated influencers
My site was thrown in a week after joining the company
[For those who want to do their best in iOS from now on] Summary of the moment when I was surprised after developing an iOS application for two and a half years