匿名質問者
匿名質問者匿名質問者とは「匿名質問」を利用して質問した質問者。
「匿名質問」では、ユーザー名を公開せずに匿名の質問ができます。
詳しくはこちら

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


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

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

回答の条件
  • 1人10回まで
  • 13歳以上
  • 登録:2014/08/24 17:12:01
  • 終了:2014/08/31 17:15:05

回答(3件)

匿名回答1号 No.1

匿名回答1号「匿名質問」を利用した質問に回答すると「匿名回答○号」と匿名で表示されます。
「匿名質問」では、ユーザー名を公開せずに匿名の質問ができます。
詳しくはこちら
2014/08/24 17:40:22

redirect_to ...

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

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

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

匿名回答3号 No.2

匿名回答3号「匿名質問」を利用した質問に回答すると「匿名回答○号」と匿名で表示されます。
「匿名質問」では、ユーザー名を公開せずに匿名の質問ができます。
詳しくはこちら
2014/08/25 10:52:37

一番ダメなのは回答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
匿名回答4号 No.3

匿名回答4号「匿名質問」を利用した質問に回答すると「匿名回答○号」と匿名で表示されます。
「匿名質問」では、ユーザー名を公開せずに匿名の質問ができます。
詳しくはこちら
2014/08/25 12:46:14

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

  • 匿名回答2号
    匿名回答2号 2014/08/24 17:53:11
    http://stackoverflow.com/questions/20424221/argumenterror-in-memberscontrollerlogin-invalid-byte-sequence-in-us-ascii
    うーん

この質問への反応(ブックマークコメント)

「あの人に答えてほしい」「この質問はあの人が答えられそう」というときに、回答リクエストを送ってみてましょう。

これ以上回答リクエストを送信することはできません。制限について

絞り込み :
はてなココの「ともだち」を表示します。
回答リクエストを送信したユーザーはいません