人力検索はてな
モバイル版を表示しています。PC版はこちら
i-mobile

悪いコードの例がよくわかりません

デキるプログラマだけが知っているコードレビュー7つの秘訣(DevLove版)
http://www.slideshare.net/rootmoon/ss-38278479

このスライドの14ページ目に悪いコードの例が載っていますが、恥ずかしながらなぜ悪いかわかりません。
このコードはどのように書くのが正解なのでしょうか?



●質問者: 匿名質問者
●カテゴリ:コンピュータ
○ 状態 :終了
└ 回答数 : 3/3件

▽最新の回答へ

1 ● 匿名回答1号
redirect_to ...

が、ダメダメ。
ユーザの属性を判定するだけかと思いきや、別ページに飛ばすという副作用がある。
後、メソッド名もダメ。
checkadmin? ではなく is_admin? の方が良い。
redirect_to は、クライアント側で書くべきでしょう。

 is_admin = ○○○.is_admin?
 unless is_admin then
 redirect_to ...
 end

プレゼンを聞いてないので、詳細は不明ですが、admin じゃない場合に、/members/(id) のページに飛ばす意味が分からない、というのもダメです。


2 ● 匿名回答3号

一番ダメなのは回答1号さんの書かれている通り、redirect_toですね。
メソッド名を見た限りチェックロジックに見えるのに、呼んだら画面遷移するとかふざけんなと思います。

メソッド名についてはrubyのコードなのでis_admin?よりもadmin?の方がさらに良いかと。
#is_admin?だとrubocopのデフォルト設定で指摘事項に上がりますね

また、コードを見てみるとこのメソッドは以下の内容で十分であることがわかります。

def admin?
 session[:login].admin
end

周辺のコードにもよるので一概には言えませんが、ここまで単純なメソッドであれば、クライアント側のコードを以下のようにしてadmin?メソッドはなくしてしまうのも一手ですね。

unless session[:login].admin # もしくは○○○.admin?
 redirect_to ?
 return
end

3 ● 匿名回答4号

真偽を返すだけの機能の必要性は何だと感じさせることがダメなのだと思います。
こういった機能を必要とするロジックは、構造的に整理されていません。
従って、メンテナンスで破綻する可能性が大きいです。

関連質問

●質問をもっと探す●



0.人力検索はてなトップ
8.このページを友達に紹介
9.このページの先頭へ
対応機種一覧
お問い合わせ
ヘルプ/お知らせ
ログイン
無料ユーザー登録
はてなトップ