「デキるプログラマだけが知っているコードレビュー7つの秘訣（DevLove版）」内のコード解説

先日デキるプログラマだけが知っているコードレビュー7つの秘訣（DevLove版）のスライドを公開したのですが、「ダメなコード例のどの辺がダメなのか、スライドがなくて分からない」という声をいただいたので、解説を載せようと思います。Railsのコードなので、Railsのコンテキストに従った説明になってしまいますが、ご容赦を。。

1 問題のコード
2 あわせて読みたい

問題のコード

スライドに載せたのはangel.rbのApplicationController#chackadmin?メソッドです。管理者でない場合はメンバーの削除を許可しない、といったアクセス制限のために、MembersController#destroy内の処理のような形で利用されていました。

ついでに、この処理を僕が書くとしたら、というコードをsample.rbとして載せてみました。

コーディングスタイルの観点：メソッド名とコードの内容が合っていない

実際に講演でスライドのコードを例にお話したのは、コーディングスタイルの観点の話でした。メソッド名とコードの内容が合っていない、ということですね。

Rubyでは通常、末尾に?がつくメソッドは単純にtrueまたはfalseを返すのみで、それ以外の処理が行われないことを期待されています。コード例ではreturnの前にredirect_toが書かれていることで、単純にtrueまたはfalseを返すメソッドだ、という期待を裏切っています。

if文を判定している最中にリダイレクトしちゃうなんて、ちょっとつらいです。。そういうのはifを抜けた後にやって欲しい。

redirect_to current_member and return unless checkadmin?

なら、まだ分かる気がします。

「えー、こんなことぐらいでダメなコード呼ばわりかよ」

と思われるかも知れませんが、読む側は一つ一つのメソッドを踏みしめながらコードを読んでいます。そんな中、末尾に?がついていたり、!がついているようなコードに対しては「あ、ここはBooleanを返しているんだな」「ここは破壊的な処理をしているんだな。どんな内容が破壊的に変更されるんだろう？」と、メソッド名に従ってある程度処理を予測しながら読んでいるんですね。それだけに一般的な命名規約と違うコードを見ると、「ああ・・・何を基準に読んで良いのか分からない・・・全てのメソッドを一字一句読み込むしかないのか」と、かなり感情を揺り動かされてしまうんですよね。

10段ぐらいif文がネストしているとか、for文がネストしているとか、念のためもう一度値を代入しておくとか、そういった派手なものもありますが、普段コードを読んでて「うげー」と思うのは、大体期待している処理と内容が違う例だったりします。

あとは一見複雑そうで何をしているのか分からない処理に名前がついていなかったり、とか。

こういったものがリファクタリングで紹介されている、いわゆる「コードの臭い」というものなのだと思います。

Rubyのコーディングスタイル

RubyのコーディングスタイルについてはThe Ruby Style Guideをベースにお話するようにしています。大体のRubyのコードはこのスタイルガイドとスタイルが類似しているので、おさえておくとコードが読みやすくなりますし、書きやすくなります。

更にリーダブルかどうかを見るときの観点としてはRubyやRailsでリファクタリングに使えそうなイディオムとか便利メソッドとかで紹介されている観点も合わせて見ています。

その他のポイント

そもそも各アクション毎に権限に応じたアクセスコントロールを書くよりは、フィルタを使う方が可読性が高まりますし、再利用もしやすいです。いくつかのアクションで同じような判定をするなら、フィルタを使うのがDRYで良いですね。
302リダイレクトを行う場合のLocationヘッダは絶対URIを指定するようにRFCで求められて（SHOULD）います。なので、redirect_to member_url(id)と、絶対URIを設定するようにするのがより適切でしょう。
このコード断片だけでは分からないと思われますが、実際のコードではsessionの値にMember（ActiveRecord::Base）のオブジェクトがそのまま入っています。このプロジェクトではCookieセッションを採用していたため、Marshal.dump(session)して暗号化した値がそのままクッキーに保存されることになります。クッキーの容量は大体4KBまで（ブラウザによって異なります）なので、その他にもセッションに保存したり、またはMemberクラスが肥大化したりしたときに溢れてしまう可能性があって危険ですね。ユーザーを特定するために必要な情報（IDなど）のみをクッキーに保持するようにした方が良いのではないかと思います。

session[:login]がnilのときにadminメソッドを呼び出したときNoMehodErrorになるのでは？というご指摘もあるかと思いますが、コード例ではそもそも認証の処理を書いていないので割愛しました。