自動でコードレビュー

    rails my-posi

    当面は無料で提供されるらしいのでやってみて損はないかな？

    　セキュリティホールをふせぐという謳い文句に惹かれて「SideCI」使ってみました。レビューの対象に使ったプロジェクトは前回の記事でRails4にバージョンアップしたばかりのこれです。
    　使っている人の感想とか探せばもっと見つかると思いますが自分が参考にしたのはここです。中身はBrakemanやRails Best Practiceなどのツールを組み合わせたものらしいけど、統合メニューで操作できるのは確かに有難いかも。色々メニュー項目がありますが自分が使ってみたものについて感想と、指摘された改善点についてざっと書きます。
    　自分が書いたプログラムにどのような難癖を付けられるのか気になる人もいると思いますので参考にしてください:grin:

    • ニュースフィード
      　Test&Deployの機能や通知機能を使う気がない自分にとってはあまり必要がないメニューかも

    • Dashboard

    ダッシュボード画面初期状態


    ダッシュボード画面コード改修後


    • セキュリティ 
      <%= link_to("結果をTwitterでつぶやく", "https://twitter.com/intent/tweet?text=#{u(@strategy.get_message_str)}&url=" + request.url) %>

      　たしかRails3以降は自動で文字列をエスケープしてくれるはずと思って検索したらここにもそう書いてあります。それなのに警告が出るのはおそらくSideCI上のRails Best Practiceのオプション設定の問題だと思います。もしそうだとしたらGemfileに使用しているRailsのバージョンが書いてあるのだからオプション設定もそれに合わせてほしいところですが、一応以下のように修正して警告を出さないようにすることは出来ました。

      <%= link_to("結果をTwitterでつぶやく", "https://twitter.com/intent/tweet?text=#{h(@strategy.get_message_str)}&url=#{h(request.url)}") %>

      念のためhtmlタグやシングルクォートを出力して確認しましたがh()で囲まなくてもちゃんとエスケープされていました。無駄な時間を取られましたが、まぁ確認する機会を得られたと前向きに考えるべきなのでしょうかヽ(~〜~　)ノ\ ハテ?

    • コード品質

      • Protect mass assignment
        このサイト（Rails4はattr_accessibleが使えない）に書いてある通り、Rails4だとエラーになるからRailsをバージョンアップした時に削除したのに、そこが悪いと指摘されました。これはどういうことでしょう？これもセキュリティの警告と同じでRails Best PracticeがRails4に対応していないものを使っているのか、オプション設定が厳しめになっているのかそんなところでしょう。よく分からないけど無視することにしました。こうなってくるとコードレビューしたことが混乱を引き起こすという本末転倒になってる気もしますが^^;本気でこのツールを使う気があるなら運営元に質問すればおそらく解決するでしょう。

      • Move Model Logic into the Model
        自分が納得したとこだけ修正しました。

      • Remove empty helpers
        中身が空のヘルパーファイルがありますよ。自動生成したくなかったらconfig.generators.helper = falseしておけということですね。

      • Replace instance variable with local variable
        納得。Rails2の頃からのソースで、よく理解していないまま書いていたコードがそのままでした。

      • restrict auto-generated routes strategies (except: [:new])
        以下のように定義していたルートに対して、

      resources :strategies do
  collection do
    get :children
  end
  member do
    get :paint
    post :copy
  end
  resources :positions, :only => ["destroy", "update", "create", "edit"]
end

      　「newが使われていませんよ」と指摘してくれたわけです。これはいいかも。以下のように修正しました。

      resources :strategies, :except => ["new"] do
      • Simplify render in controllers
        このプロジェクトはModel用のテストスクリプト（Model用のspecファイル）しか書いていなくて、Railsのバージョンアップをした時に以下のようなRailsが出力する雛形のコードに対するテストが不十分でしたが、バグ出しツールとして役に立ちました。saveに失敗したら’new’テンプレートをrenderするとコードでは書いていますが、newテンプレートは存在していませんでした。但しファイルが存在しないことを指摘してくれたわけではありません。
        respond_to do |format|
    if @user.save
      format.html { redirect_to @user, notice: 'User was successfully created.' }
      format.json { render action: 'show', status: :created, location: @user }
    else
      format.html { render action: 'new' }
      format.json { render json: @user.errors, status: :unprocessable_entity }
    end
  end

      • Simplify render in views
        部分テンプレートを使うとき:partialというオプションを使わなければならない機会っていうのは無いってことなのか？と、新たな疑問が湧きましたが、リンク先に書いてある通りに修正しました。

      • remove trailing whitespace
        行末のスペースを削除しろってことだけど、これは細かい！まぁチームで開発するときには一応統一しておいたほうがいいでしょうね。
        あと、一つのファイルで最初に見つかった行だけを指摘するみたいなので、他の場所にも存在しないかよく確認してからcommitしましょう。

      • remove unused methods
        また使うかもしれないから残しておこうなんて思ったコードが結構残っていました。自分一人のプロジェクトなので他人を混乱させる心配はないのですが、せっかくgitでバージョン管理しているのだからバッサリ削除すべきですね。

    • テスト & デプロイ設定
      　自分が使用しているtwitterAPI用の環境変数とかSSH Keyを登録するのが不安なので使用するのは止めておきました。まぁherokuではなんの警戒もせずtwitterの認証コードを環境変数にセットして使っているのに、なんで使わないのかと問われれば返答に困りますが、仕事で使う機会があれば使ってみたいと思います。