Ruby on Rails Code Quality Checklist抄訳

オレンジニュース経由でこんなものを見かけました。

Ruby on Rails Code Quality Checklist

これはいいチェックリスト。あとだしジャンケンぽいですが、私がいつも思っていることがいろいろ書いてあってすばらしいです。これをすべてYesにするのは難しいというか机上の空論ぽいところもありますが、これを目指すことには価値はあると思います。

ということで項目だけを抄訳(&地の文は私感)を書いてみます。誤訳などがあればツッコミお待ちしています。

コントローラのアクションではfindやnew以外のモデルメソッドは一つくらいにしなさい(必要なら.newや.updateメソッドをオーバーライドするといい)。

原文: 1. Each controller action only calls one model method other than an initial find or new. (Make custom .new or .update methods in the model with all necessary).

そうそう。コントローラは薄く、モデルを厚く。ただ.newをオーバーライドするよりかセッターをオーバーライドする方がおすすめです。Railsレシピブックでいえば「075.カラムに対応しない値を保存可能にする」とか「096. モデルに対応したフォームで入力項目を作成する」とかそのあたりをご参考に。あとはERDレッスンを読むといいと思います。

コントローラとビューの間で受け渡すインスタンス変数は1つか2つに抑えなさい

原文: 2. Only one or two instance variables are shared between each controller and view.

これも同じく。RSpecでBDDしてると必然的にこうなります*1。正しい設計に導いてくれるRSpec++。

モデルや変数名はわかりやすくて短いものにしたほうにしなさい

原文: 3. All model and variable names are both immediately obvious (to a new developer) and as short as possible without using abbreviations.

これは言わずもがな(まぁ難しいけど)。

二カ所以上からアクセスされるカスタムfind(find_hogeとか)は、替わりにnamed_scopeを使いなさい

原文: 4. All custom "finds" accessed from more than one place in the code use named_scope instead of a custom method.

微妙だけどおおよそ賛成。9割くらいはnamed_scopeでいいと思うので、まずはnamed_scopeにできないか考えるというのはいい作戦だと思います。

ビューやヘルパーではfindやfind_by_は使わないようにしなさい

原文: 5. A .find or .find_by_ is never called in a view or view helper. ...

これは最初は??だったんですが、本文での説明を読んで納得。

Item.find_all_by_user_id(@user).each{ }

ではなく

@user.items.each{ }

を使った方がいいということですね。

コントローラから渡される(その画面での本質的な)モデルオブジェクトとの関連を使ってビューを組み立てましょう、ということで。これもほとんどの場合賛成です。

ただセレクトボックスの選択肢を作る場合とかは仕方ないと思う。

Railsにもとから入ってる機能を自分で作り込まないようにしなさい

原文: 6. There is zero custom code that duplicates functionality of a built-in function in rails.

まったくその通りです。でもRailsの機能を全部把握するとかなかなか難しいですけど心がけたいですね。(宣伝: よく使うものは「Railsレシピブック」に書いてあるYO!!)

開発中はアグレッシブにDRY化しなさい

原文: 7. Code has been aggressively DRYed during development.

総論賛成です。開発中はいいと思います。ただapp/以下ではeval族を使わない運動を個人的に推進中ですので、コードレビューなんかでその辺は(重複しても)eval族を廃した方がいいと思ってます。

evalしたい場合はもう少しがんばってプラグインなんかまで抽象化したいものです。

二つ以上のモデルで使われる機能はライブラリやモジュールにしなさい

原文: 8. All functionality used in two or more models has been turned into a library/module.

異論なし

二つ以上のアプリケーションで使われるロジックはgem化したプラグインにしなさい

原文: 9. All logic duplicated between two or more apps has been turned into a gemified plugin.

ですよねー。むずいけど。

STIは使うな!!

原文: 10. STI is not used anywhere

賛成です。私もSTIはだいぶ嫌いです。
本文の方でポリモルフィック関連はありとのこと。こちらも賛成。

デザイン(UIのことかな?)はユーザにとってもっともシンプルに価値を提供できるものにしなさい。

原文: 11. Every design choice should yield the most simplistic design possible for the need of users at the current time. No guesses for future functionality were designed into the application. ...

ほかのものとちょっと毛色が違いますね。訳にいまいち自信がないです。
http://www.matthewpaulmoore.com/articles/1276-ruby-on-rails-code-quality-checklist#simplicity でGetting Realに触れられていますから、たぶんそういう話だと思います。

追記: 2008-09-23 18:48追記

no6vさんから指摘を受けてもう一度みてみましたが、やっぱりdesignはUIに限らず設計全般という意味でよさげな気がしてきました。YAGNIということですな、おそらく。

できるだけ外側からテストを書いてカバレッジをあげなさい

原文: 12. Close to full test coverage exists at the highest level of the application: on and between controller actions. Coverage is highest for code used by the most number of end users.

テストのカバレッジをあげると同時に、ユーザが実際にさわるレイヤに近いところからのテストを充実させよ、という話だと思います。integration testを充実させようね、ということかな。

これもわかっているけど難しいですよね。ただテストの資産価値を上げるためにも、ある程度作った機能に関してはブラックボックスなテストを増やしていくように心がけたいです。

共有リポジトリにコミットする前にすべてのテストを通しなさい

原文: 13. All tests pass before code is merged into a shared repository.

異論なし。テストが増えてくると大変ですが、その辺の運用をみんなどうやって効率化しているか知りたいですね。

バグを直したらリリース前に回帰テストしなさい

原文: 14. Every fixed defect on a deployed product has tests added to prevent regression.

そりゃそうですね。

使っているプラグインはコードレビューしなさい

原文: 15. Every plugin installed has been code reviewed.

Railsプラグイン界はずいぶんと玉石混淆なので、コードレビューした上で自分らでメンテナンスする覚悟ができないプラグインは使わない方がいいと思います。
レビュー結果やパッチをプラグイン作者にフィードバックするとなおいいと思います。

以上です。最近はほかにもセキュリティ周りなんかで思うところがあったりするので、社内勉強会をしたいですね。もちろん、「社内」といってもなんかやれば資料などは公開するつもりです。

参考&宣伝

楽々ERDレッスン (CodeZine BOOKS)

Railsレシピブック 183の技

*1:ビューのspecでassignをいっぱい設定するのがいやになるので