こんにちは、ソニックガーデンの西見です。岡田さんの突撃記事、「どうすればコードは美しくなる！？コードレビュー交流会に潜入！」はご覧いただけましたでしょうか？　この記事では、コードレビュー交流会で実際にどのようなコードレビューが行われていたのかを、主にエンジニア向けにお伝えしようと思います。

ソニックガーデンブログの主な読者さまにはちょっとテクニカルすぎる内容になっているとは思いますが、日頃エンジニアはこんなことを考えているというご参考までに、ぜひご覧ください。

• どうすればコードは美しくなる!?コードレビュー交流会に潜入!【前編】
• エンジニア向け！コードレビュー交流会に学ぶ美しいコードとは【後編】

コードレビュー交流会で実際に話に上がった5つのポイント

というわけで、実際にコードレビュー交流会で話に上がった5つのポイントを、コードのビフォー・アフターとあわせてご紹介したいと思います。

ポイント1：コメントで伝えるより、コードで上手く伝えることを考える

UPDATE_INTERVAL = 60 # 秒

例えばこのコードではコメントで秒を表すことが表現されていますが、実際にこの定数を使っている場面では、定数名を見るだけでは単位が分かりません。それならこういう風にコードを変えてみてはいかがでしょう。

UPDATE_INTERVAL_SEC = 60

変数名に単位を入れることで、秒を表すことが明確に分かるようになりました。ちょっとした工夫ですが、コメントの増殖を抑えることができますし、リーダブルですよね。

ポイント2：複雑な条件分岐がある場合は、まずメソッドに切り出してみる

def health
  instance_health = nil
  if status == Status::RUNNING
    if @alarms.find{ |_, value| value[:state] == Alarm::ALARM }.nil?
      instance_health = Health::OK
    else
      instance_health = Health::DANGER
    end
  else
    instance_health = Health::DISABLED
  end

  instance_health
end

例えばこのコードでは@alarms.find{ |_, value| value[:state] == Alarm::ALARM }.nil?が結局のところ何を判定しようとしているのか、ひと目では分かりません。レビューを経て「アラームがないこと」を条件としていることがわかったので、以下のように条件文を切り出してみました。

def running?
  status == Status::RUNNING
end

def no_alarm?
  @alarms.any?{ |_, value| value[:state] == Alarm::ALARM }
end

def health
  if running?
    if no_alarm?
      Health::OK
    else
      Health::DANGER
    end
  else
    Health::DISABLED
  end
end

条件文に名前をつけることで、条件分岐の見通しがよくなります。元のコードではinstance_healthというローカル変数を引き回していましたが、ロジック上は特に必要がなかったため、あわせて整理しています。

ポイント3：便利なメソッドを知ってコード量を減らす

def format
  instances = []
  @instances.each do |instance|
      instances << instance.to_h
  end

  instances
end

@instancesの中身をHashのインスタンスに加工する処理を行っているコードですが、このような配列の中身を加工して返すという処理はRubyがとても得意としているところなので、こんな風に書くことができます。

def format
  @instances.map(&:to_h)
end

1行で書くことによって処理が分かりづらくなってしまっては問題ですが、この場合はシンプルに処理が表現ができているため、コードとして分かりやすくなっています。

ポイント4：変数を減らす

def call
  aws_information = {
    :update_time => Time.now,
    :ec2_instances => ec2_instances,
    :rds_instances => rds_instances
  }
  aws_information
end

aws_informationという変数にHashのインスタンスを代入して、最後に返しています。特に変数に代入しなければならない理由はなさそうなので、このように整理してみました。

def call
  {
    :update_time => Time.now,
    :ec2_instances => ec2_instances,
    :rds_instances => rds_instances
  }
end

できるだけ不要な処理を省くことで、コードの見通しを良くしています。

ポイント5：DRYを広い視点で持ってコード量を減らす

class Instances

   # ... (中略) ...

   def each
     @instances.each{ |instance| yield instance }
   end

   def each_with_index
     @instances.each_with_index{ |instance, i| yield instance, i }
   end

   def select
     select_instanecs = @instances.select{ |instance| yield instance }
     Instances.new.init_with_instances(select_instanecs)
   end

   def count
     @instances.count
   end

   # ... (中略) ...

end

each_with_indexやselectといったメソッドは、RubyではEnumerableというモジュールで提供されています。基本的にeachメソッドを実装しさえすれば、each_with_indexやselectといったメソッドを自動的に使えるようになるので、これらのメソッドを自分で実装する必要がなくなります。今回は機能的にもEnumerableをインクルードするだけで問題がなさそうだということがわかったので、Enumerableをインクルードして、機能的に重複したコードを削除しました。

class Instances

   # ... (中略) ...

   include Enumerable

   def each
     @instances.each
   end

   # ... (中略) ...

end

コーディングにはDRY（Don’t Repeat Yourself）、つまり「重複したコードがないようにする」という原則がありますが、今回の整理もDRY原則に則っています。自分の書いたコードの中だけではなく、Rubyという言語が用意してくれているあらゆる機能を含めてDRYにすることで、絶対的なコード量を減らして、コード全体の見通しを良くしています。

何よりも重要なこと：コードレビューを継続すること

ここまで具体的なコード例でコードレビューのポイントをご紹介してきましたが、このようなプログラミング上のテクニックももちろん重要ながら、「ここのコード大丈夫？」「このコードはもっと上手く書けるよ」といったコードレビューを継続していくことがとても重要なことだと個人的には考えています。システムは成長を続けますし、その成長にあわせて同時にコードも改善していくことで、保守性の高いシステムが構築されるからです。

その点、今回コードレビュー交流会でお話させていただいた北九州市立大学の山崎研究室の学生さんたちはコードレビューの習慣を持ってらっしゃるとのことで、本当に素晴らしいなと思いました。ぜひこれからも内部でコードレビューを続けていって欲しいなと思います。

ソニックガーデンではこのようなコードレビューを通じた交流会を引き続き行っていきたいと考えています！　興味のある方はぜひお気軽にお問い合せください。@mah_lab宛にTwitterでメンションをいただくのでも大丈夫です。

あわせて読みたい

• どうすればコードは美しくなる!?コードレビュー交流会に潜入!
• ドキュメントをなくしてもうまくいく？ 〜 人に依存するリスクへの対処とは
  （Social Change! 株式会社ソニックガーデン SonicGarden 代表 倉貫義人のブログ）

ライティング：西見 公宏（にしみ まさひろ）
ユーザにとっての「わかりやすさ」を追求するユーザー・エクスペリエンスのエキスパートを目指すプログラマ。清く正しく美しいソースコードが好き。まーくんが365日間更新し続けるブログ（仮）というブログで毎日記事を投稿しています。