乐胖代购免代理版

はじめに

2014年8月11日の晩に放送されたソニックガーデンのweb勉強会、SonicGardn Studyでは「いつまでクソコードを書き続けるの？〜出来るプログラマだけが知っているコードレビュー7つの秘訣〜」というタイトルで、弊社ソニックガーデンの西見さん(@mah_lab)が講演してくれました。

デキるプログラマだけが知っているコードレビュー7つの秘訣 from Masahiro Nishimi

いつまでクソコードを書き続けるの？〜出来るプログラマだけが知っているコードレビュー7つの秘訣〜 - YouTube

この放送の中でも触れられていたように、ソニックガーデンではコードレビューを大事にしています。

ただ、勉強会のスライドの中では具体的なコード例や指摘の例がほとんど出てこなかったので、「実際どんな感じなの？」という疑問を持った方もいたんじゃないかと思います。

そこで今回は「入社間もない頃の僕」が書いたコードをソニックガーデンのメンバーにコードレビューしてもらったときの具体例を書いてみます。

コードレビューを受ける前のコード

今からお見せするのがちょうど2年前、2012年8月に書いた僕のコードです。
ソニックガーデンへの入社が2012年6月だったので、本当に入社間もない頃です。

当時の僕のRuby歴は半年ぐらいでした。
前職ではJavaやC#をメインで使っていて、Rubyをちゃんと使い始めたのはソニックガーデンの選考期間が始まってからです。

さて、この下に載せたのはTwitter用のボットプログラムのソースコードです。
"random_tweet"がメインのメソッドで、データベースに登録されているツイート用のデータをランダムに取得してツイートします。

では、どんなコードだったのか見てみましょう。
「自分だったらここをこう指摘する！」というレビュアーの視点になって読んでみると面白いかもしれません。

app/models/my_tweet.rb

require 'twitter'

class MyTweet < ActiveRecord::Base
  attr_accessible :tweet
  validates :tweet, presence: true, length: { maximum: 140 }

  def self.random_tweet
    configure_twitter
    tweet = MyTweet.all.shuffle.first.tweet
    post tweet
  end

private
  def self.post(tweet)
    Rails.logger.info "Posting...#{tweet}"
    begin
      Twitter.update tweet
    rescue => ex
      Rails.logger.error ex
    end
  end

  def self.configure_twitter
    pit = Pit.get(
      "mytweet",
      :require => {
        "twitter.consumer_key"       => '',
        "twitter.consumer_secret"    => '',
        "twitter.oauth_token"        => '',
        "twitter.oauth_token_secret" => '',
    })

    # HerokuではPitが使えないので環境変数を使う
    pit["twitter.consumer_key"]       ||= ENV["twitter.consumer_key"]
    pit["twitter.consumer_secret"]    ||= ENV["twitter.consumer_secret"]
    pit["twitter.oauth_token"]        ||= ENV["twitter.oauth_token"]
    pit["twitter.oauth_token_secret"] ||= ENV["twitter.oauth_token_secret"]

    Twitter.configure do |config|
      config.consumer_key       = pit["twitter.consumer_key"]
      config.consumer_secret    = pit["twitter.consumer_secret"]
      config.oauth_token        = pit["twitter.oauth_token"]
      config.oauth_token_secret = pit["twitter.oauth_token_secret"]
    end
  end
end

このときはランチタイムの全員コードレビューでした (そして蜂の巣にされました・・・)

このコードをランチタイムにソニックガーデンのメンバー全員でコードレビューしてもらいました。
当時の僕は「まあ、50行ぐらいのちっちゃなプログラムなんで、コードレビューもすぐに終わると思いますよ。ははっ」てな感じで高をくくっていました。

・・・ところが、次から次にメンバーから指摘をくらい、見事に蜂の巣にされました(苦笑)。
たしか30分ぐらいかけてこのちっちゃなコードをレビューしてもらった記憶があります。

当時の様子はソニックガーデンのFacebookページにも載っています。

f:id:JunichiIto:20140813073816p:plain

ソニックガーデンでは、ランチタイムにコードレビューをしたりします。

今日は、SonicGarden_bot（ https://twitter.com/sonicgarden_bot ）のコードレビューです。

少ないコードでも突っ込みどころを見つけて議論が盛り上がります。

こうしたコードレビューを通じて、お互いに知ってることを教えあったり、人のテクニックを盗んだり、何が良いコードなのかを共有したりして、切磋琢磨しています。

これが技術に取り組むソニックガーデンの舞台裏のひとつです。
SonicGarden Japan's Photos - August 3, 2012

ソニックガーデンのメンバーから挙がった指摘内容

ではここから、このときのコードレビューでソニックガーデンのメンバーから挙がった具体的な指摘内容を載せていきます。

指摘1: configure_twitterでやっている初期化処理はconfig/initializersに移動させる

ツイートを投稿する前にconfigure_twitterというメソッドを呼んでconsumer_keyやconsumer_secretを初期化していますが、こういった処理はconfig/initializersの中でRailsの起動時に1回だけ実行する方がRailsらしい、との指摘を受けました。

指摘2: Pitではなく環境変数を直接使えば良い

ソニックガーデンの別の案件でPitというgemを使ってcredentialな情報を管理していたのでそれを真似てここでもPitを使っていました。
しかし、こういう場面で使ってもメリットがないので環境変数から直接取得すれば良いと言われました。

たしかにローカルで開発するときはPitで、Herokuで動かすときは環境変数というのも一貫性がなくて変です。

指摘3: "shuffle.first"は"sample"で置き換えられる

「データベースの中からランダムに1件のツイートを選ぶ」という処理を実現するために"MyTweet.all.shuffle.first"というコードを書いてたんですが、「それは"MyTweet.all.sample"って書けばいいよ」という指摘を受けました。
たしかに、同じことをやるなら1つのメソッドで実現した方がいいですよね。

2014.08.14追記: "all.sample"のパフォーマンスが気になるという意見について
Twitter等で「えっ、"all.sample"なんて使っても大丈夫？」という意見をちらほら見かけました。

はい、このプログラムでは多くても数百件程度のレコードしか扱わないので、"all.sample"でも問題ありません。（いわゆる富豪的プログラミング）
また、スケジューラーが実行するのでレスポンスタイムをミリ秒単位で縮める必要もありません。

もちろん、要件によっては"all.sample"すると「恐ろしい事態」が起きる可能性もあります。
そのリスクを理解した上で、"all.sample"を使うかどうか判断する必要があるでしょう。
(Railsに詳しくない方のために説明すると、allメソッドはテーブル上の全レコードをインスタンス化して、配列に詰め込むメソッドです。)

ちなみに、パフォーマンスやセキュリティ上の懸念を指摘するのもコードレビューの重要な目的のひとつです。
なので、「"all.sample"で大丈夫？」って気付いた人はレビュアーとしてかなりセンスの高い人だと思います！

指摘4: "post tweet"はクラスメソッドではなくインスタンスメソッドにする

ツイートを投稿する処理では"post"というクラスメソッドを使っていたんですが、「MyTweetのインスタンスがツイートの内容(tweet)を持っているんだから、インスタンス自身がツイートを実行すれば良い」という指摘を受けました。
たしかにそれもそうです。あまり何も考えずにクラスメソッドを作っていました。

指摘5: クラスメソッドはprivateキーワードの下に定義してもprivateにならない

前述のpostメソッドはprivateキーワードの下に定義していました。
もちろんこれはpostメソッドをprivateメソッドにしたかったからなんですが、「クラスメソッドはprivteキーワードの下に定義してもprivateにならないよ」という指摘を受けました。
この点も当時の僕はそんな仕組みになっているとは全く知りませんでした。

その指摘を受けたあと、「じゃあどうやってprivateにしたらいいんだろう？」と思ってこんな記事をQiitaに投稿しました。

Rubyのクラスメソッドはインスタンスメソッドと同じようにprivateにならない

リファクタリング後のコード

というわけで、指摘の内容を受けて修正したコードがこちらです。

config/initializers/twitter.rb

Twitter.configure do |config|
  config.consumer_key       = ENV['TWITTER_CONSUMER_KEY']
  config.consumer_secret    = ENV['TWITTER_CONSUMER_SECRET']
  config.oauth_token        = ENV['TWITTER_OAUTH_TOKEN']
  config.oauth_token_secret = ENV['TWITTER_OAUTH_TOKEN_SECRET']
end

app/models/my_tweet.rb

require 'twitter'

class MyTweet < ActiveRecord::Base
  attr_accessible :tweet
  validates :tweet, presence: true, length: { maximum: 140 }

  def self.random_tweet
    random_select.post_tweet
  end

  def self.random_select
    all.sample
  end

  def post_tweet
    logger.info "Posting...#{tweet}"
    begin
      Twitter.update tweet
    rescue => ex
      logger.error ex
    end
  end
end

いかがでしょうか？
MyTweetクラスの実装がかなりスッキリしたと思います。

・・・が、今の自分から見るとまだ気になる点がいくつかあります。

今の自分からの指摘1: "random_select"メソッドが大した仕事をしていない

"random_select"メソッドの実装は"all.sample"を呼んでいるだけです。
もしこのメソッドが他に使われていないのであれば、わざわざメソッド化する必要はないと思います。

今の自分からの指摘2: "post_tweet"メソッドのbegin - endは省略できる

"post_tweet"メソッドではrescueするための begin - rescue - end がありますが、実質的には"Twitter.update tweet"を1行呼んでいるだけなので、beginとendは省略してしまって良いでしょう。

今の自分からの指摘3: "random_tweet"というメソッド名がイマイチ

"random_tweet"というメソッド名は「形容詞 + 名詞」のように見えるので「ランダムなツイートが返ってくるだけ？」と勘違いされるかもしれません。
何かの処理を行うメソッドは極力動詞から始める方がわかりやすいと思います。
なので、"post_random_tweet"のような名前の方がベターですね。

再リファクタリングしたあとのコード

というわけで「今の自分」がこのコードをもう一回リファクタリングするとこんな感じになります。

app/models/my_tweet.rb

require 'twitter'

class MyTweet < ActiveRecord::Base
  attr_accessible :tweet
  validates :tweet, presence: true, length: { maximum: 140 }

  def self.post_random_tweet
    all.sample.post_tweet
  end

  def post_tweet
    logger.info "Posting...#{tweet}"
    Twitter.update tweet
  rescue => ex
    logger.error ex
  end
end

最初のコードは46行ありましたが、これだと17行です。
かなりシンプルになりました。

あと、ここには書きませんでしたが、環境変数を直接 "ENV" から取り出すとテストを実行するときに環境変数の扱いを考慮しなければならなくなるので、最近ではRailsConfig gemを使って実行環境ごとに設定値を書き換えることが多いです。

まとめ

というわけで、今回は入社当時の僕が書いたホンモノのコードと、コードレビューで挙がった具体的な指摘内容を載せてみました。

予想以上の指摘を食らってそれなりの衝撃は受けましたが、「ムカついた」とか「泣きたくなった」とかそういったマイナスの感情はありませんでした。
ちゃんとした人からちゃんとした指摘を受けると、「教えてくれてありがとう。これでもっといいコードが書けるようになります！」と、すがすがしい気持ちになります。(身体は蜂の巣になってますが・・・)

当時の感想がyouRoomに書いていた日報にあったので、こちらに転載しておきます。

f:id:JunichiIto:20140813075319p:plain:w350

コードレビューはこれまた予想を覆されるほどの指摘事項の多さでしたが、確かにどれもごもっともな指摘ばかりでした。
前の会社だと、「それは個人の好みでしょ」的な議論になって、少し険悪な空気が流れたりする時もありましたが、ソニックガーデンではそういう方向には発展しないところが良いです。

(松村) 個人の好みが割りと似ているのかな？

う〜ん、というより、みんなが良いコードやRuby/Railsらしい書き方を心得ているとある程度、同じような結論に収斂していくのかもしれないです。
あとRailsがフルスタックのフレームワークなのも大きな理由かも。
独自設計や独自ルールの割合が大きければ大きいほど、意見が分かれていきそうな気もします。

ちなみに、ソニックガーデンのコードレビューは毎回こんなふうに一つのコードを全員でレビューするわけではありません。
通常は「リリース前のコードレビュー」という形で、masterブランチにマージするためのpull requestを他のメンバー(誰でもOK)がレビューしてコメントを入れるスタイルが多いです。

とはいえ、頻繁にメンバー全員でコードレビューをしていることには間違いありません。
こうやってコードレビューをやっているからこそ、良いコード書き方や良いコード、悪いコードに対する認識を共有することができるんだと思います。
まだコードレビューをやっていないという方は、ぜひみなさんのチームでもコードレビューの文化を取り入れてみてください。

ソニックガーデンが提供するRailsトレーニングコースもあります

そもそも優れたプログラマがいなくて有意義なコードレビューを実践できる自信がないという方は、ソニックガーデンのトレーニングコースを受講してみると良いかもしれません。

以下に詳細が載っているので、気になる方はそちらもぜひチェックしてみてください！

Ruby on Railsコードレビューサービスのご案内 - SonicGarden 株式会社ソニックガーデン

SonicGarden Studyで開発に役立つ情報をゲットしよう！

SonicGarden Studyは開発に役立つ情報を発信するweb勉強会です。
Ustreamで放送するので、日本全国どこからでも視聴することができます。

またDoorkeeperでコミュニティ登録してもらうと、告知ページの公開時にメール通知を受け取ることができます。
みなさんのコミュニティ登録をお待ちしています！

SonicGarden Study | Doorkeeper

P.S. 2年経ったら当時の新人も今や・・・！？

今ではこんなことを言われてます。

なかなか生々しい。今では伊藤さんが厳しいレビューをする側に。 link:ソニックガーデンで行われているコードレビューの具体例をお見せします (SonicGardn Study #11 の補足として) #sg_study http://t.co/iWNlau33aQ
— Yoshihito Kuranuki (@kuranuki) August 13, 2014