はじめに
みなさんこんにちは。プログラミング歴5ヶ月の新米エンジニアです。
今回の記事では、これまでに受けたコードレビューの中で「なるほど！」と思ったものをいくつか紹介していきたいと思います。
条件式の返り値をそのまま変数に代入すればいいのでは？
これは、私が書いたこんなコードに対する指摘でした。
if @hoge.blank?
piyo = true
else
piyo = false
end
条件が真のとき変数にtrueを、偽のときfalseを代入するというものです。
条件式の返り値をそのまま変数に代入するような処理ですね。
レビューをもとにこれを書き換えるとこんな感じ。
piyo = @hoge.blank?
なんともスマートになりました。
返り値というものをそこまで意識していなかった私にとって、これは目から鱗のレビューだったといえるでしょう。
入っている値がぱっと見で分かるような変数名に
私は以前、こんな具合に変数を定義していました。
user = User.all
list = ['apple', 'orange', 'banana']
human = true
レビューをもとにこの変数たちを修正すると、
users = User.all
fruit_list = ['apple', 'orange', 'banana']
is_human = true
こんな感じになります。
一つめは、値が複数なら変数も複数形に。
二つめは、「何の」リストなのかを明確に。
三つめは、値がブーリアン型であることが分かるように。
些細な変化ですが、これによってコードの可読性は格段に上がります。
実際のコードはこんなに単純なものばかりではなく、どう言い表したらいいか分からない複雑なものがたくさんありますよね。
このような変数に対して中身を簡潔に言い表す名前を考えるのは至難の業ですが、負けじと食らいついていきたいと思います。
見れば分かることをコメントに書くな
これは私のこんなコメントに寄せられたものでした。
# 現在時刻をtimeに代入
time = Time.current
見ての通り、コードを日本語に直訳したようなコメントを残していたわけです。
これに対して、「それは見れば分かる。何をしているかではなく、何のためにしているかを説明しよう」と指摘を受けました。
当時の私は、コメントはあればあるほど可読性が上がると勘違いしていました。
今となっては読めば分かることを書いても意味がないことは当たり前なのですが、最初は主にコメントを読んでコードを理解していたので、自分が分かりやすいからと不要なコメントを量産してしまっていました。
コメントは本当に必要な場面で適切なものだけを残していきたいですね。
SQLを流すのはできるだけループの外で
SQLが流れるような処理をループの中でするな、というものですね。
ダメなコードの例はこんな感じ。
id_list = [1, 2, 3]
id_list.each do |id|
name= User.find(id).name
# nameに対する処理
end
これだと、id_listの長さ分だけSQLが流れることになります。
この例ではたかが3回なのでパフォーマンスへの影響は小さいですが、これが何万回にもなると話は変わってきます。
大量のSQLが流れることになり、処理が重たくなってしまうわけです。
改善するとこんな具合になります。
user_list= User.where(id: id_list)
user_list.each do |user|
name = user.name
# nameに対する処理
end
これなら流れるSQLは1本ですみますね。
初めの頃はとりあえず要件を満たすことしか考えていなかったので、パフォーマンス面を加味することも大事だということを学ぶことができました。
終わりに
今までに受けたレビューはまだまだたくさんありますが、その中でも特に印象的だったものを抜粋して紹介しました。
ひとつひとつのレビューを真摯に受け止め、同じレビューを何度も受けないよう精進していきたいと思います。
最後まで読んでいただきありがとうございました。