-
Notifications
You must be signed in to change notification settings - Fork 0
Self Review Check List
This check list is supposed to be used on implementation. Also it could be copied and pasted in the description on a Pull Request. The list should be customized for your team's needs. So you don't need to use all of them. The goal of this is to help developers write readable, maintainable and performant code.
Anyone can write code a computer can understand, but professional developers write code humans can understand.
Clean code is a reader-focused development style that produces software that's easy to write, read and maintain.
- Don't use concern.
- Don't use helpers unless it's really needed.
- PR doesn't include unrelated changes? (1 PR should do 1 thing)
- Follow the coding style guides?
You should use linters, such as rubocop for Ruby and ESLint for JS.
- Did you check the code on github yourself before asking for review?
- No
No newline at end of file
warning on github? - Did you check the code works properly yourself (on the browser if needed)?
- Does the code meet the requirements?
- Are the commits rebased (squashed) for the reviewers to understand the changes easily?
- No test (spec) errors?
- Correct every feedback you got?
You should correct not only the line where you've got feedback, but also the same/similar code in other lines.
- No unnecessary comment?
Code should be clear enough without comments. But you can write comments when needed.
- Write annotation comments where necessary? (such as
TODO
/NOTE
/FIXME
/OPTIMIZE
)
- Did you leave notes on
README
, underdocs
orwiki
about what other developers should know.
- Are variable and method names self-explaining?
There are only two hard things in Computer Science:
cache invalidation
andnaming things
. -- Phil Karlton - Are the scope of variables appropriate? (e.g. local variable, instance variable, class instance variable)
Bear in mind that class instance variables are not thread safe.
- Does the code follow SOLID?
- No code duplication? (Or comply with rule of three?)
- This article might help you think when you refactor your code.
- No magic numbers?
- Comply with YAGNI?
- Use transaction properly?
Make sure if you want to create/change the records atomically.
- Make methods private appropriately?
If there're lots of private methods, it's a sign to extract them to a class. (See an example. You can also extract a totally new class instead of creating a class in the existing class.)
- Write tests (models / requests / system) where necessary? Write tests, but try not to write unnecessary tests. See below.
-
TDD再考 (Reconsider TDD)
There're eight articles. They're Japanese articles, but there're links to the English articles.
- Private methods
- You don't need to write unit tests for private methods. But if there're many private methods, that might be against SRP. Then you should think of creating a new class.
-
TDD再考 (Reconsider TDD)
- Add indexes to foreign keys?
- Add indexes on columns that are used for sql
WHERE
clauses? - Add unique indexes where necessary? (Bear in mind that the unique validation provided by Rails doesn't work perfectly)
- Add composite indexes (unique index) on columns where needed in intermediate tables?
e.g. add_index :job_categories, [:job_id, :category_id], unique: true
- Add constraints, such as
null: false
,default: <something>
, where necessary?
- Add validation, such as
uniqueness
andpresence
, where necessary? (Database error, which shouldn't be shown to users, is shown if there's database constraint only without validation by Rails) - No N + 1 queries?
- Normally bullet detects them, but it doesn't in some cases. So you should check the log, e.g.
tail -f log/development.log
. - Sometimes bullet detects them wrongly. So make sure if there're actually inefficient queries. If not, think of adding whitelist.
- Normally bullet detects them, but it doesn't in some cases. So you should check the log, e.g.
- Not fat model? (If it is, think of moving the logic to a class as mentioned at the followings)
- Use
find_each
orfind_in_batches
where necessary? - Add
inverse_of
in intermediate tables as follows?class Appointment < ApplicationRecord belongs_to :physician, inverse_of: :appointments belongs_to :patient, inverse_of: :appointments end
- No view related logic in models?
- Use bulk insert with activerecord-insert when there're lots of insert at once? Bulk insert is built in on Rails6.
Because inserting a record one by one brings high load against the database.
- Create a method chainable class method/scope? (ref: https://techracho.bpsinc.jp/hachi8833/2020_08_13/92483)
@posts =
Post
.where(params.permit(:name))
.by_user(params[:user_id])
.with_comments(params[:show_comments])
.with_tags(params[:tags])
# In Post class
def self.with_comments(comments)
return all unless comments
includes(:comments)
end
- Not fat controller? (No Logic in controller?)
- Organized appropriately?
- Can only permitted users (e.g.
current_user
and/oradmin
) do CRUD operations to their possessions?Say there're user A and B. A shouldn't be able to destroy B's stuff.
- When accessing secret information, is login required and the content encrypted?
- CSRF, XSS, etc
- No complex logic in views and helpers? (If there is, think of moving the code to Decorators (e.g. draper) or View Objects)
- Avoid
accepts_nested_attributes_for
? (see)
- Do they have versioned namespaces, such as
v1
? - Does the routing follow the theory regarding CRUD, Verbs, and Actions?
- Does a GET endpoint Read only? (No Side effect (CUD from CRUD) on GET)?
- Use cache (conditional get and/or middleware such as Redis) properly?
- Did you write documents of APIs? (You can use Swagger or something)
- Resolvers shouldn't have logic. If they have, move the logic to somewhere else.
Never use ES5 and CoffeeScript for a new project.
- Uglified and minified?
- Extract classes appropriately? (ES 2015 or later) (Here's an example, but it's not certain that this is best.)
- No unnecessary client side validation?
The client side validation by JS is useful for better UX and/or decreasing the traffic to the server. But bear in mind that you have to write extra code. So you shouldn't do that unless it's really required. You should write validation on the database (where the data is stored) and model level (to show appropriate error messages). Most of the time, that is enough.
ENV is a simple key/value store. All values are converted to strings. Nested configuration structures are not supported.
- Reviewing code each other among junior developers. Then, asking a senior developer for review.
- Sharing review feedback among developers everyday. (Then you can learn very quickly!!)
- Writing down review feedback on a spreadsheet or something and share them to other developers. (Then you're respected from other developers and your bosses!!)
- Consider using uuid as id (rails 5.0 or after)
- 外部キー以外で、
_id
という名前は使用しない。- Rails の外部キーと被ることがある。開発者も、そうであることを予期(期待)するのが自然。値としても integer または bigint を期待する。
-
_id
を使いたい場合 (外部サービスなどで提供されるキーで string のもの。例えば、stripe_customer_id
やtransaction_id
など) は、id_token
またはxx_id_token
とする 。- 例:
transaction_id
→transaction_id_token
- 例: StripeCustomerクラスに
stripe_customer_id
を追加したいと思った時は、id_token
(unique)とする。
- 例:
-
id_token
orxxx_id_token
自体が馴染みのある名称ではないので、そのクラスの中で一意に識別できるキーという共通認識を持つ必要がある。 - リポジトリ全体でルールが統一されていないと効果が半減(または混乱の元?)する。