いからしです。

今日は若手社員のプログラムレビュー時によくあがる、「まあ直さなくても大丈夫だけどこうするともっといいよね」的なTipsをまとめます。規模としては「1メソッド内での小技」程度のものとします。

また、ここに書く内容は100%の正解ではありません。プロジェクトの仕様や設計方針に応じた個々の都合、プログラマそれぞれの宗教感、デスマーチ中でそれどころじゃない、など様々なケースがあると思いますので。

「そういうやり方もあるよねえ」という参考にでもしていただけたら幸いです。

 

思いやりのある条件式

例えば「今日が2020/03/01から2020/03/15の間である場合に、OKと標準出力する」という実装を行うとします。

ここでやりがちなのは下記のようなプログラミング(あ、Rubyで書きます)。

「RubyならComparable使えばええやんけ」という指摘はナンセンスです。今はそういう話をしているのではありません。あなたの良くない癖です。

またどうでもいいですが、いからしはRubyの文字列リテラルはダブルクォート派です。式展開が必要になったときにクォートまで修正するのがダルいので。

 

さて。上記の7行目、もしくは10行目の条件式は何がイケてないのかというと、パッと見で条件が頭に入ってきづらいところです。

ただでさえ時間の大なり小なりは「あれ?こっちでいいんだっけ?」となりがちですね(二日酔いの日などは特に顕著です)。

下記のようにするだけでだいぶわかりやすくなります。

コメントにも書いてますが「from < now < to」と並んでいると直感的に理解しやすいですね(二日酔いの日でもスルリと理解できます)。

ここで覚えてほしいのは「時間の範囲条件はfrom < target < to」で並べるとよい」ことではなく、「条件式はパっと見たときにわかりやすい、思いやりのあるものがよい」ということです。

「えーっと、ここがANDでtrueになって、その右側の括弧の中もtrueだから…」みたいなやつは最悪。読むのもストレスだし改修時にバグも出やすくなります。

実装者は吊し上げてチームメンバーの前で公開(ry… シンプルかつ読みやすい条件式を心がけたいものです。

 

具体的なメソッド名、ローカル変数名、仮引数名

職業プログラマはいつも多忙です(偏見)。

各メソッドにちゃんとRDocやJavaDocのようなコメントを整備すべきと思いながらも、「忙しいし周りもやってないし、ていうか処理を切り出しただけのprivateメソッドにまでそんなもん書いてられっかよ…」という気持ちで働いています(偏見)。

いからしはそれこそ、処理のまとまりを切り出しただけのprivateメソッドにまでそれらのコメントを残すべきとは思っていません。

がしかし、だからこそ「そのメソッドに何を渡すと何が返るのか」「どのような副作用を及ぼすのか」などは引数と戻り値を見ただけでスルっと理解できるものであるべきです。

 

ということで例示。

第一引数に渡された文字列について、第二引数に渡された桁数を超過した範囲を削除し、末尾に三点リーダ(…)を付与して返却する。ただし元の文字列が第二引数の桁数以内であれば、そのまま返却する。また第三引数に三点リーダの代替となる付与文字列を渡すこともできる。」という実装を行うとします。

まずはイケてない例から。

このメソッドのイケてない点は

  • メソッド名が「convert」(変換)という抽象的すぎる名称。何をどう変換するのかわからない。
  • 第一引数が「str1」(文字列1)という抽象的すぎる(ry…。メソッド名が具体的ならまだしも、この状態ではstr1が何に使われるのかわからない。
  • 第二引数が「digits」(桁数)という(ry…。
  • 第三引数が(ry…。

ということで、至る所の名称が抽象的過ぎて実装の中身を見ないと何がなんだかという感じです。

もちろんこの程度の実装であれば読解にさほど時間はかからないと思いますが、それでも工夫ひとつで削減できるはずの時間を他人のアレなコードに取られるのは癇に障る ちょっとモヤモヤしますし、実装規模が大きくなるにつれてより大きな負担になってしまいます。

リファクタリファクタ。

改善点としては

  • メソッド名を処理の具体的な内容「omit」(省略)に変更。より具体的に「omit_str」としがちですが、引数名を具体的にすることで対応できるので、メソッド名としては冗長。
  • 第一引数を「処理対象の文字列」を意味するものに変更。
  • 第二引数を「桁数上限」を意味するものに変更。ここで気を付けたいのは、「以下」と「未満」、「以上」と「超過」を厳密に区別した単語をチョイスすること。嘘を書いてはいけません。
  • 第三引数を「末尾に付与する文字」を表すものに変更。「suffixって識別子だろ」という声が聞こえる気もしますが、まあプログラマなら慣習的に理解できるはず(?)。

 

この手法はメソッド名と引数だけでなく、ローカル変数にも適用されるべきです。

例えば

  • 「time」(日付)ではなく「business_start_time」(営業開始時間)
  • 「value」(値)ではなく「allowable_upper_limit」(許容上限値)

とかとか。

 

また、注意して欲しいのは「名称は常に具体的であるべき」ではないということ。

例えば「親クラスに処理のフローと、フロー内でコールされる抽象メソッドを実装。

各子クラスでは抽象メソッドのみ実装」というようなよくあるポリモフィズムの場合、親クラスの抽象メソッド名は当たり前ですが、「check_permit_execute」(後続処理をしてもよいかのチェック)とか「convert」(変換処理、処理対象が子クラスで異なる)とか、そういうフワっとしたものになるべきです。

 

あと命名という部分で一つ。

「data」「info」のような単語は極力使うべきではないといからしは考えています。

メソッドはデータや情報を処理するものだし、変数はそれらを格納するものなので。

「そのデータは具体的に何なのか」はわかるはずなので、それを名称とすべきです。

 

コメントは書くべきものを書け

言われるのも言うのも経験ある新人レビュー事項ランキング1位(俺調べ)、「コメントはちゃんと残そうね」。

コレ正しいんですけど言葉が足りてない感あって、厳密には章題のとおりで「書くべきものを書け」だと思います。

では書くべきものが何なのかというと、それは「実装から読み取ることができない事情や仕様」です。

逆に書くべきではないのは

  • 呼び出しているメソッドの処理内容(呼び出し先メソッドを見ればよい)
  • 実装の解説。

なのだけど、これをベタベタと書いてしまうケースは結構多い。

 

具体的な例を示します。

【呼び出しているメソッドの処理内容をコメントしているケース】

 

【実装を解説しているケース】

 

どっちの例も「うん、ほらね…?まあ、コードをね?見ればわかるよね…?」ってなるやつですね。

ちょっと1つ目の例は直しようがないので、2つ目に適切なコメントを入れてみます。

「content_type = 〜」のコメントは若干言い訳じみている気もしますが、まあ書かずにあとで「何でコイツこんなとこで処理してんだ!(憤怒)」と言われないためにも書いておいて損はないです。保身も大事。いや、別に保身のためにコメントして欲しいわけではないですけど。

あと背景や仕様の説明が長くなるようであれば、別ファイルでREADMEを切って、ソースコードないのコメントはそこへのリンクにするのが良いと思います。

 

まとめ

いつも通りまとめるほどのことはないのですが、結局こういう部分って「後からコードを見る人に対する思いやり」or「自分がdisられたくないがためのプライド」のいずれかがあると伸びやすいのかなーと思ったりします。

ちなみにいからしは完全に前者ですね。思いやりで走る車があったら1日で世界一周できるレベル。

とフザけたところで、ではまた。