When I reviewed the code, I always pointed out something similar, so I summarized the perspectives I care about. There is a point of view that does not depend on the language used, but since I mainly use Rails at work, there is also a point of view that is specific to Ruby and Rails.
For the time being, I briefly wrote what I can think of at the moment, but I will add it if I remember the viewpoint I care about in the future. It is good to continue delivering (CD) both development and Qiita articles without aiming for perfection from the beginning.
Use static analysis tools to check static parts of your code, such as indentation and coding rules. It is a waste of time for humans to visually check, and leakage and blurring occur. In addition, you will be distracted by the details and overlook the points that should be pointed out.
Rubocop is a well-known static analysis tool in Ruby. The introduction of functions that support such development tends to be postponed in the initial development, but it is very difficult to insert static analysis tools in the middle. If you try to add it later, you will get code that relaxes or exempts the rules, so be sure to include it at the initial development stage. Personally, I think it's best to set it tight and relax as needed.
It's a contradictory theme because I just wrote that static code check should be used one before, but static code check is relaxed or an exception if it is not enough to make confusing corrections to correct the indication of static code check. It is better to set. A common method is overlined methods. When the method exceeds a certain number of lines, it is said to divide it, but if it is appropriate that the processing is a single unit, it will only be difficult to understand, so I think that it is not necessary to force it.
In Rails, it often works without problems even if the status code is not consistent with the method and response, so I try to check whether the status code is used properly. I think that the detailed usage is different depending on the person, but I will write my interpretation about the main status code.
2XX This is the status code when the request is processed normally.
201 Created It is used when a resource is generated by POST or PUT request. When using 201, the generated resource is often returned in the response body. If you have nothing to return, I think you should use 204 No Content.
204 No Content It is often used when a resource is deleted by a DELETE request, but it is also used when there is no response body even in POST or PUT. When using this, do not put a value in the response body.
200 OK If you want to return a normal response other than the above, you should basically use 200 OK.
This is the status code when you want to redirect to another page.
301 Moved Permanently Use 301 if you want to be permanently redirected to another page. For example, when migrating a web page, it is used to redirect from the URL of the old page to the new page. It is also useful as an SEO measure. If you specify 301, the search engine will index the new URL for you.
303 See Other I wasn't aware of it until recently, but if you want to redirect to the detail page of the resource after registering / updating the resource, use 303. The pattern is as follows. Register new resources with POST / resources and redirect to the page (GET / resources /: id) that displays resource information.
302 Found
Unlike 301, use 302 if you want to temporarily redirect to another page.
If you use redirect_to in Rails, it will be 302 by default, but considering the purpose of redirecting, 302 has few uses, and I think that it is often more appropriate to distribute it to 301 or 303.
This is the status code when a client-induced error occurs.
404 Not Found The status code used when the page cannot be found. Since "404" is often displayed on the screen, I think that many people know it.
It may be a different idea for some people, but I try to use 404s and 200s depending on whether the missing resource is abnormal or normal for the client. Here are some concrete examples to make it easier to imagine.
If resources cannot be found with GET / resources /: id
Return 404 because the client specifies an id that does not exist
Get the TODO list When GET / todos has 0 TODOs
Since 0 TODO is normal operation, 200 is returned.
When I specified the narrowing conditions in the store search, the number became 0. Return a 404 that did not meet the conditions specified by the client From the SEO point of view, I don't want the search engine to index 0 shop search result pages, so I don't want to index by specifying 404.
401 Unauthorized Use when no valid credentials are passed for an action that requires authentication.
403 Forbidden It is used when you are authenticated but the usage authority is insufficient. For example, when a read-only user attempts to update a resource.
However, there is one thing to consider when using a 403. When you access a resource that is private to a user. In this case, if you think about the system, you will want to use 403 because you do not have access authority, but if you return 403, the user will be informed that the resource exists. In such a case, it is better not to inform the existence itself, so it is better to return 404 with the meaning that it does not exist.
400 Bad Request It is used when an invalid request comes in, such as when the required parameters are not specified. If it doesn't apply to 401, 403, 404, I usually use 400.
410 Gone It is used when the URL to be permanently deleted is accessed, such as after the period ends and the page is deleted on the limited-time campaign page. It's a shame, but lately there are many pages left unattended even after the campaign period is over, and it becomes a noise when searching, which is an obstacle. I would like you to carry out the task of deleting it when the deadline has passed.
This is the status code when a server-induced error occurs.
500 Internal Server Error Use when an unexpected error occurs on the server. I don't think it's common for programs to explicitly generate a 500 error, so if there is a part that explicitly returns a 500 error, I think it's better to modify it so that an appropriate error is returned.
503 Service Unavailable It is used when the maintenance page is displayed because it cannot be accessed normally due to regular maintenance.
You may want to return an error message to the client when an error occurs, but make sure the message is correct. The point of view is whether you can receive the error message and take action. A common bad example is a pattern that returns a message that does not specify an item such as "The input item is invalid" when one input condition is not met on a screen with multiple input items. With this, you don't know which item is wrong and you don't know what to do. Let's tell the item and the cause of the error, such as "Enter the value of the xx item as a numerical value".
However, there are cases where it is NG to tell the user in detail. For example, if you specify an existing ID on the login screen where you enter the ID / password and return "wrong password" when the password is incorrect, the user will be informed that the ID exists. .. With this implementation, a malicious user can enter an appropriate ID to create a list of existing IDs. Whether the login screen does not have an ID or only the password is different, it is good to return a message such as "Invalid login information" that cannot identify what is wrong.
I think that modern web applications often have separate front and back ends. As a result, the backend often provides functionality as an API. When creating an API, when returning an enumerated item, you should return it so that you can understand the meaning just by looking at the value. For example, if you have the following status:
enum status {
todo: 1,
in_progress: 2,
done: 3
}
If you want to return this to the front desk, you should return the name (such as'to do') instead of the number (1, 2, 3).
If you return a numerical value like status = 1, what is 1 when you see the response? However, if you return the name like status ='todo', you can understand the meaning just by looking at it.
When returning an array, check whether the order of the array is unique.
In the following, the reviews array narrowed down by user_id is returned, but since order is not specified, it is uncertain what order will be returned. It's fine if it's certain that the caller doesn't care about the order, but let's specify the order so that the order is fixed.
def my_reviews(user_id)
Review.where(user_id: user_id)
end
Below, the descending order of created_at is specified. If this is the case, it seems that you can get the newly created ones in order.
def my_reviews(user_id)
Review.where(user_id: user_id).order(created_at: :desc)
end
However, there are still problems with this. The order is not fixed when created_at has the same review. In order to determine the order surely, it is good to consider whether the columns specified in order are unique as shown below. This time, I specified the descending order of id in the second sort to make it unique.
def my_reviews(user_id)
# 1. created_Arrange in descending order of at. However, created_at has no unique restrictions, so it can be the same day
# 2.If it is the same day, sort them in descending order of id. The id is unique so it cannot have the same value
# =>The order is uniquely determined
Review.where(user_id: user_id).order(created_at: :desc, id: :desc)
end
During code reviews, you may find implementations that take a long time to understand or are misleading. Such an implementation should be as simple as possible, as everyone who sees this code will feel the same and increase the man-hours of code reading and increase the probability of embedding bugs. If you can't make it simple, you can add a comment. If you are using a framework such as Rails, you may misunderstand that you are using it away from the framework as shown below. Misleading code has a high probability of not creating bugs later.
class Review
belongs_to :user
enum status {
open: 0,
close: 1,
}
end
class User
#× Even though it is the default association, it is narrowed down by specifying conditions
has_many :reviews, -> { open }
#○ If you want to set a condition, use a name that understands it.
has_many :open_reviews, -> { open }, class_name: 'Review'
end
class Hoge
def fuga(user_id)
user = User.find user_id
#If you look only here, there is a high possibility that you will mistakenly think that you have acquired all the reviews of the user.
user.reviews
end
end
YAGNI As many of you may know, there is the word YAGNI. See wikipedia for details. https://ja.wikipedia.org/wiki/YAGNI
As mentioned above, even if you imagine and implement a function that you do not use yet, it is unlikely that you can use it as it is. Be careful if you see the code you are writing imagining the future. It's painful to have the implemented code removed, but it's often better to remove it. If you leave it, it may get in the way later, or you may have to fix it even though you haven't used it for refactoring, which may reduce your productivity.
Make methods and constants that are not called from the outside private. If it is public, the range of influence when modifying the method or constant will be the entire source, but if it is private, you only have to worry about the same class, so it is quite easy to investigate the range of influence of source modification such as refactoring. Become.
Each class has its own responsibilities. Check that your responsibilities are being followed.
For example, consider the action that activates your account.
Since the processing executed at the time of activation should be known by the Account model that has the responsibility of Account, it is better to implement it according to the policy like ○ below.
class AccountController
#× The controller knows what to do at the time of activation
def activate
account = Account.find_by(token: params[:token])
account.status = :activated
account.activated_at = Time.now
account.token = nil
account.save!
end
#○ The controller does not know what to do at the time of activation and asks the account model
def activate
account = Account.find_by(token: params[:token])
account.activate!
end
end
class Account
def activate!
status = :activated
activated_at = Time.now
token = nil
save!
end
end
includes, eager_load, preload If you are uncertain about these differences, please see the Qiita article below, which you should have seen at least once if you are using Rails. https://qiita.com/k0kubun/items/80c5a5494f53bb88dc58
includes is a convenient existence that uses preload and eager_load properly. When I started using Rails, I thought it would be better to use includes, but the current idea is the opposite. I try to use basic preload or eager_load. This is because I think we should understand whether or not to join when implementing it. Includes that may or may not join depending on the implementation are difficult to control and may behave unexpectedly, so it is better not to use them.
ActiveRecord is very useful, but it feels like SQL is far from the Rails engineer.
If you write SQL yourself, you will be willing to issue SQL that you wouldn't write.
Among them, the one that you see in particular is a pattern that joins in vain.
A concrete example is shown below.
A method find_by_organization that specifies the organizatoin_id to get the users who belong to that organization.
class User
has_many :user_organization
def self.find_by_organization(organization_id)
#× organizations tables do not need to be joined
# select * from users inner join user_organizations on users.id = user_organizations.user_id inner join organizations on user_organizations.organization_id = organizations.id where organizations.id = #{organization_id}
joins(user_organization: :organization).merge(Organization.where(id: organization_id))
# ○ user_Only organizations are combined
# select * from users inner join user_organizations on users.id = user_organizations.user_id where user_organizations.organization_id = #{organization_id}
joins(:user_organization).merge(UserOrganization.where(organization_id: organization_id))
end
end
class Organization
has_many :user_organization
end
class UserOrganization
belongs_to :user
belongs_to :organization
end
Recommended Posts