Hatena::ブログ(Diary)

moroの日記 このページをアンテナに追加 RSSフィード

2008-09-23

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.

これも同じく。RSpecBDDしてると必然的にこうなります*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)

楽々ERDレッスン (CodeZine BOOKS)

Railsレシピブック 183の技

Railsレシピブック 183の技

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

no6vno6v 2008/09/23 16:27 自分もこれ読んで冷や汗かいてました。
11.は UI もそうですが、設計のことじゃないでしょうか?
現時点で必要なものだけを、シンプルに実装して入れるようにしましょう。
将来必要になるかもしれない機能なんて入れちゃダメ、と。

yuguiyugui 2008/09/23 16:58 ヘルパーにSQL片を書いたら負けかなと思っている。ビューにSQL片を書く奴は死ねばいいのにと思っている。

セレクトボックスやなんかで必要なら、そういう種類のfinderが必要とされているということなので私はモデルにメソッドを足しますねぇ。ただ、find_by_xxx は許して欲しいんですけど。

moromoro 2008/09/23 18:54 > no6vさん
ありがとうございます。designはやっぱ普通に訳して良さそうですね。追記させていただきました。

> Yuguiさん
> セレクトボックスやなんかで必要なら、そういう種類のfinderが必要とされて
> いるということなので私はモデルにメソッドを足しますねぇ

まぁ確かにそうですねぇ。この頃だとnamed_scopeを使うのがいいんでしょうね。

どうせnamed_scopeを使うなら削除フラグやactivationテーブルにリレーションを張ったりして、それをラップしたnamed_scopeと組み合わせるとあれでそれな入力項目のメンテ機能ができそうな気がしてきました。いいかも。

スパム対策のためのダミーです。もし見えても何も入力しないでください
ゲスト


画像認証