wataメモ

日々のメモをつらつらと書くだけ

Code ClimateでGPAを4.0にした

Ruby Rails

　Code ClimateでGPAを表記上4.0にした。 静的解析ツールはリソース監視と同様に常に掛けておいて、コードの質をチェックしていかないと行けない。 前回の「Ruby on Railsでサービスを作ってみた」メモのコードを静的解析を掛けて、いくつかissueがあったので修正した。 基本的には「Complexity」の修正だけだった。 前のSTIで修正した部分もあるが、今回はそれ以外の修正した例をメモしていく。

主な改修パターン

  1. 記述の簡潔にする
  2. メソッド分けを行う
  3. データ構造を見直す

　記述を簡潔にするのは、言語としての書き方や、ライブラリの機能をきちんと活用することで実現することが出来る。 冗長的に書いてしまっている部分のリファクタリングや、データ構造の見直しを行うことになる。

app/models/multiple_choice.rb

全体

全体before
  def sel_sum i
    r = []
    s = {}
    self.choices.each {|c| s[c[:value].to_s] = 0}
    self.answers.joins(:collaborator).where(collaborators: {status: 1}).each do |a|
      begin
        JSON.parse(a.text).each {|v| s[v] += 1}
      rescue
        if a.text.present?
          s[a.text] = 0 if s[a.text].blank?
          s[a.text] += 1
        end
      end
    end
    self.choices.each do |c|
      r << (s[c[:value].to_s].present? ? s[c[:value]] : 0)
    end
    r
  end
全体after
  def sel_sum
    s = Hash.new 0
    self.answers.joins(:collaborator).where(collaborators: {status: 1}).pluck(:text).each do |t|
      JSON.parse(t).each {|v| s[v] += 1}
    end
    self.choices.inject [] {|r, c| r << s[c[:value]]}
  end

　だいぶ短くなっているが、記述的に短く書いた部分とHashのデフォルト値を使った事によって簡潔にしている。 rubyのHashにはnewで初期値を渡す事ができ、存在しないキー値でアクセスされた場合に初期値を返すように出来る事を利用した。

初期化処理

初期化処理before
self.choices.each {|c| s[c[:value].to_s] = 0}

　この初期化部分が以下で済むようにした。

初期化処理after
s = Hash.new 0

　もちろん厳密に言えばこれは等価ではない。コントロールされた値だけを0にしているのではなく、どんな値でも0で返すからだ。

ループ部分

ループ部分before
self.answers.joins(:collaborator).where(collaborators: {status: 1}).each do |a|
  begin
    JSON.parse(a.text).each {|v| s[v] += 1}
  rescue
    if a.text.present?
      s[a.text] = 0 if s[a.text].blank?
      s[a.text] += 1
    end
  end
end
ループ部分after
self.answers.joins(:collaborator).where(collaborators: {status: 1}).pluck(:text).each do |t|
  JSON.parse(t).each {|v| s[v] += 1}
end

　この部分は単一選択と処理を共通化させたいと思っていた部分があったが、ここに来る時点では複数選択なのが確定しているので処理を簡潔にできた。 ついでにtextカラムしか使っていないのでpluckして性能改善もした。 Hashのデフォルト値を与えているのでいきなり「+= 1」出来る。
　pluckの後にeachせずにmapでJSON.parseして、flattenしてからeachでインクリメントしても良かったが、なんとなく自重した。
　Active Recordの部分はモデル側に書いておいた方が見通しが良かったかも知れない。

結果作成ループ部分

結果作成ループ部分before
r = []
self.choices.each do |c|
  r << (s[c[:value].to_s].present? ? s[c[:value]] : 0)
end
r
結果作成ループ部分after
choices.inject [] {|r, c| r << s[c[:value]]}

　よくArrayを外で宣言して、ループ内で何かしらのデータを突っ込んでそのArrayを返す処理。 injectメソッドを利用し初期値に空のArrayを渡す。そしてinjectの結果をそのままメソッドの戻り値とすることで、外側の変数が不要となった。 ここでも同じくHashのデフォルト値のおかげで三項演算子の利用がなくなっている。

app/controllers/answer_controller.rb

全体

全体before
class AnswerController < ApplicationController
  def create
    s = Surveys.current_group(request.subdomain).eager_load(:questions, :collaborators).find_by({
      id: params[:survey_id],
      questions:     {id:   params[:question_id]},
      collaborators: {uuid: session[:uuid]}
    })
    q = s.questions.first
    c = s.collaborators.first

    if params[:answer].present?
      a = c.answers.where(question_id: q.id).first_or_initialize answer_params
      # override text if question is multiple or date
      a.text = params[:answer][:text].to_json if q.multiple? || q.date?
      a.save!
    else
      # if no answer pathed destroy all answers for this question.
      c.answers.where(question_id: q.id).destroy_all
    end

    return redirect_to survey_question_path(s, q.next) if q.next
    c.done!
    redirect_to finish_survey_path
  end

  alias_method :update, :create

  private
  def answer_params
    params.require(:answer).permit :text
  end
end
全体after
class AnswerController < ApplicationController
  before_action :find_info, only: [:create, :update]

  def create
    @answer.attributes = answer_params
    # override text if question is multiple or date
    @answer.text = params.fetch(:answer, {})[:text].to_json if @question.multiple? || @question.date?
    @answer.save!

    return redirect_to survey_question_path(@survey, @question.next) if @question.next
    @collaborator.done!
    redirect_to finish_survey_path
  end

  alias_method :update, :create

  private
  def answer_params
    params.fetch(:answer, {text: ''}).permit :text
  end

  def find_info
    @survey = Surveys.current_group(request.subdomain).my_answer params, session[:uuid]
    @question = @survey.questions.first
    @collaborator = @survey.collaborators.first
    @answer = @collaborator.answers.where(question_id: @question.id).first_or_initialize
  end
end

　before_actionを利用してActive Record部分のメソッド分けを行った。 当初はDBアクセス処理が一箇所だけだったので分けていなかったが、createメソッドを簡潔にするためにbefore_actionにした。 単なるメソッドわけにしても良かったが、そうすると呼び出しを書かなければならなくなるのを嫌った。

パラメータハンドリング部分

パラメータハンドリング部分before
if params[:answer].present?
  a = c.answers.where(question_id: q.id).first_or_initialize answer_params
  # override text if question is multiple or date
  a.text = params[:answer][:text].to_json if q.multiple? || q.date?
  a.save!
else
  # if no answer pathed destroy all answers for this question.
  c.answers.where(question_id: q.id).destroy_all
end
パラメータハンドリング部分after
@answer.attributes = answer_params
# override text if question is multiple or date
@answer.text = params.fetch(:answer, {})[:text].to_json if @question.multiple? || @question.date?
@answer.save!

　パラメータが渡ってこない場合（1つも選択肢を選ばなかった場合）の為にif文を入れていて、渡ってきていなかったらdelete、渡ってきたらupdate or insertを行っていた。 レコードを消す必要は特になかったので、save!に集約し、パラメータが渡ってこない場合はfetchを使いデフォルト値を返すようにし分岐を消した。 Strong Parametersもrequireではなくfetchに変更している。

Strong Parameters部分before
params.require(:answer).permit :text
Strong Parameters部分after
params.fetch(:answer, {text: ''}).permit :text

まとめ

　GPA4.0に維持しようとすると、思ったよりも厳しくメソッドを簡潔にしないといけないと感じた。 ただ、これぐらい簡潔にすることを心がけるのはメリットも大きい。 より良い記述方法は無いのかと考えるきっかけにもなるからだ。 その場合に単純なメソッド分けに頼るのではなく、言語の仕様、ライブラリの使い方やデフォルト値の考えを導入することをしっかりと検討する方が良いだろう。