1

私はいくつかの発見に以下の更新を提供しました...

現在のコードにあるメソッドは次のとおりです。

def query(data_set, conditions)
  query_data_set = data_set.dup
  search_conditions = parse_conditions(conditions)

  search_conditions.each do |condition|
    if condition.class == Array
      condition.each do |term|
        if term.class == Symbol
          query_data_set = entity.send term, query_data_set
        else
          query_data_set = search_data_text(query_data_set, term)
        end
      end
    end

    if condition.class == Hash
      condition.each do |key, value|
        query_data_set = method("query_#{key}").call(data_set, value)
      end
    end
  end

  query_data_set
end

Rubocop がこれを嫌う理由は 3 つあります。

C: Assignment Branch Condition size for query is too high. [17.03/15]
  def query(data_set, conditions)

C: Method has too many lines. [19/7]
  def query(data_set, conditions)

C: Use next to skip iteration.
  search_conditions.each do |condition|

イテレーションをスキップしたくないので、なぜ使用するのかまったくわかりませんnext。このコードでは、反復をスキップすることが理にかなっている場合はありません。

他の苦情に移ると、このメソッドの先頭で、既に 1 つのアクション ( への呼び出しparse_conditions) が分割されていることがわかります。への呼び出しもありますsearch_data_text。ここでの私の唯一のポイントは、それが理にかなっていると思われる場合にモジュール化しようとしたことです。

その大きなsearch_conditions.eachブロックを別のメソッドに移動したとしても、Rubocop は新しいメソッドも長すぎると文句を言うでしょう。これは、 2 番目のメソッドが呼び出す3 番目のメソッドをさらに追加することを意味すると思いますか? それは私には奇妙に思えます。というか、あまり分岐しなくていいということなのかな。しかし、なぜ分岐が悪いのでしょうか? 他の構造 (case...when など) に切り替えても、まだ分岐しています。また、ネストされた配列、シンボルを含む配列、またはハッシュでは処理が異なるため、これらの条件をテストする必要があります。

このような問題を見て、効果的で効率的な解決策を見つけられるように直感を構築しようとしています....しかし、私がやっていることは、ひどく非効率的で時間の無駄です. 私のコードが悪い理由がわからないので、そのトレードオフが心配です。

これを試してみて、上記のメソッドを読みやすさの外観を維持しながらRubyistが好むスタイルガイドラインに適合する状態にする方法を理解するのを手伝ってくれませんか?

---------------------- 更新 ----------------------

私が思いつくことができる最高のものはこれです：

def query(data_set, conditions)
  query_data_set = data_set.dup
  parse_conditions(conditions).each do |condition|
    query_data_set = check_conditions(condition, query_data_set)
  end
  query_data_set
end

def check_conditions(condition, data)
  if condition.class == Array
    condition.each do |term|
      data = entity.send term, data if term.class == Symbol
      data = search_data_text(data, term) unless term.class == Symbol
    end
  end

  if condition.class == Hash
    condition.each do |key, value|
      data = method("query_#{key}").call(data, value)
    end
  end
  data
end

このcheck_conditionsメソッドは、Rubocop にはまだ長すぎ、分岐条件のサイズが大きすぎます。

私が見る限り、私がチェックしたところはどこにも違いを示すことができませんでしたが、できる唯一のことは、おそらく配列とハッシュのチェックからメソッドを作成することです。つまり、各if条件はcheck_conditions独自のメソッドを取得します。しかし、それは私には不必要にばかげているように思えます。私は基本的にロジックを分解し、変数をさまざまなメソッドに渡します。これは、メソッド数を任意の値未満に保つことができるようにするためです。

これは、デザインを行う方法として、私にはかなり間違っているように感じます。しかし、ロジックを変更して必要なことを達成できるようにする方法はわかりませんが、メソッドごとに 7 行未満で実行できます。

4

3 に答える 3

0

あなた自身の答えを拡張するために、機能的な良さを提案するかもしれません...（つまり、AKA一次関数にEnumerable.inject加えて）。lambda

(私はあなた自身のコードのロジックを疑問視したり変更したりしていないことに注意してください。単に機械的に変換しただけです)。

def query(data_set, conditions)
  parse_conditions(conditions).inject(data_set) do |data_set, condition|
    check_conditions(condition, data_set)
  end
end

processors = {
  'Array' => lambda do |condition, data|
               condition.inject(data) do |data,term|
                 term.class == Symbol ? entity.send(term, data)
                                      : search_data_text(data, term) 
               end
             end,
  'Hash' => lambda do |condition, data|
               condition.to_a.inject(data) do |data, pair|
                 method("query_#{pair[0]}").call(data, pair[1])
               end
             end
}

def check_conditions(condition, data)
  processors[condition.class.name].call(condition, data)
end

コードの数行が少なくなり (重要ではありません)、明示的な if/else 制御構造と明示的な "data = " および "return data" 部分がなくなりました。すべてが非常に明確になり、あいまいな小さなエラーが発生する可能性が少なくなりました。

もちろん、機能的なスタイルを知っている/好むチームで作業している場合にのみ、そのようなことをしてください。掘り下げる人もいれば、嫌う人もいます。

Hashとに取り組んでいない場合Arrayは、関数の代わりに OO を使用することをお勧めします (つまり、2 つの処理メソッドをそれぞれのクラスに分散します)。ハッシュと配列を再度開くことで技術的に可能です。もちろんお勧めできません。

于 2016-01-14T21:07:19.440 に答える
0

わかりました、それを行う方法を見つけました。最もクリーンではないかもしれませんが、他に何も機能していませんでした。これが私がしたことです：

def query(data_set, conditions)
  query_data_set = data_set.dup
  parse_conditions(conditions).each do |condition|
    query_data_set = check_conditions(condition, query_data_set)
  end
  query_data_set
end

def check_conditions(condition, data)
  data = process_condition_array(condition, data) if condition.class == Array
  data = process_condition_hash(condition, data) if condition.class == Hash
  data
end

def process_condition_array(condition, data)
  condition.each do |term|
    data = entity.send term, data if term.class == Symbol
    data = search_data_text(data, term) unless term.class == Symbol
  end
  data
end

def process_condition_hash(condition, data)
  condition.each do |key, value|
    data = method("query_#{key}").call(data, value)
  end
  data
end

したがって、確かに少し冗長ではありますが、合理的だと思う方法が 1 つあります。しかし、すべてが1か所にありました。これは 4 つのメソッドになり、コードのリーダーとして、これらのメソッドを通じて変数をトレースする必要があります。それが進歩だと思います。:-/

興味深いことに、このすべての後、私のクラス全体が Ruboco によって長すぎると見なされるようになりました。はぁ。とにかく、その特定の恣意的なスタイルで生きることは、他の恣意的なスタイルで生きるよりも少なくとも口当たりが良い.

于 2015-07-03T23:04:43.027 に答える