メソッド分割による処理の単純化 #176
メソッド分割による処理の単純化 #176
Conversation
ochaochaocha3
force-pushed
the
reduce_cognitive_complexity-20190926
branch
from
6d9a22e
to
57780c0
Compare
CodeClimateでCognitive Complexityの問題を指摘された箇所。 メソッドを分けて単純な処理にする。 また、不要なテストを削除した。
CodeClimateでCognitive Complexityの問題を指摘された箇所。 メソッドを分けて単純な処理にする。
ochaochaocha3
force-pushed
the
reduce_cognitive_complexity-20190926
branch
from
57780c0
to
b384c8d
Compare
| # @return [Array<Cinch::Target>] | ||
| # @note Cinch::Channel, Cinch::User は、どちらも Cinch::Target の | ||
| # サブクラスのため、Cinch::Target と同様に扱う。 | ||
| def to_cinch_target_array(targets) |
There was a problem hiding this comment.
1つのメソッドで処理をしていたときには処理を統一できませんでしたが、メソッドを分割したことで、 to_cinch_target_array と to_cinch_target でほぼ同一の処理をしていることが明確になりました。
メソッドを統一し DRY に出来ないでしょうか。
There was a problem hiding this comment.
両者の case 式の when 節の部分が同じですが、返すのが配列かそうでないオブジェクトかという違いがあるので、これ以上の統一は難しいように思えました（統一しようとすると、結局 if でフラグを見るのが必要になりそうです）。
| end | ||
| @plugin_paths = config.plugins.map { |name| | ||
| [name, "#{PLUGINS_ROOT_PATH}/#{name.underscore}"] | ||
| } |
There was a problem hiding this comment.
複数行からなるブロックは {} ではなく、do - end 構文を使うように統一していたと思います。
今回の {} の採用には何か意味があるのでしょうか。
There was a problem hiding this comment.
最近は、ブロック付きメソッド呼び出しの戻り値を使う場合 { } を使うようにしていました。特に map や select、reduce では、戻り値を変数に代入したり、メソッドチェーンで戻り値をさらに利用したりするので、ほぼすべてで { } を使っているのではないかと思います。log-archiverでも、例えば以下の部分で { } を使っていました。
https://github.com/cre-ne-jp/log-archiver/blob/684282991d9582765fd6d09602d3acee53e56beb/app/models/hour_separator.rb#L31-L32
do ... end については、戻り値よりブロック内でする（do）こと（≒副作用）に主眼を置く場合に使っていました。
外部では、クックパッドのスタイルガイドで、この書き方について言及されています。
[MUST] ブロック付きメソッド呼び出しでは、
do/end記法でブロックを書くこと。i.e. blockの副作用が目的[MUST] ブロック付きメソッド呼び出しの戻り値に対して処理を行う場合は、ブロックを中括弧で書くこと。
# good puts [1, 2, 3].map {|i| i * i } # bad puts [1, 2, 3].map do |i| i * i end # good [1, 2, 3].map {|n| n * n }.each {|n| puts Math.sqrt(n) } # bad [1, 2, 3].map do |n| n * n end.each do |n| puts Math.sqrt(n) end
| @@ -26,32 +25,46 @@ def initialize(config) | |||
| # 読み込みエラー時に該当要素をスキップするか | |||
| # @return [Array] 読み込んだプラグイン構成要素のクラスの配列 | |||
| def load_each(component_name, skip_on_load_error = false) | |||
| loaded_classes = component_paths(component_name).map { |class_name, path| | |||
| try_load(class_name, path, skip_on_load_error) | |||
| } | |||
There was a problem hiding this comment.
{} と do - end 構文の使い分けについて、同上です。
| end | ||
| @plugin_paths.map { |name, path| | ||
| ["#{name}::#{component_name}", "#{path}/#{component_name_underscore}"] | ||
| } |
There was a problem hiding this comment.
{} と do - end 構文の使い分けについて、同上です。
| @@ -26,32 +25,46 @@ def initialize(config) | |||
| # 読み込みエラー時に該当要素をスキップするか | |||
| # @return [Array] 読み込んだプラグイン構成要素のクラスの配列 | |||
| def load_each(component_name, skip_on_load_error = false) | |||
| loaded_classes = component_paths(component_name).map { |class_name, path| | |||
There was a problem hiding this comment.
この行は長すぎるという指摘対象にならないでしょうか。
loaded_classes =
component_paths(component_name).map do |class_name, path|
このように書き換えた方が良いと思います。
There was a problem hiding this comment.
ぎりぎりですが80文字以内（79文字）だったので、OKにさせてください😅
|
動作確認は出来ました。 |
|
レビューの返信、ありがとうございました。すべて承知しました。 ブロックの書き方は、既存の他のコードも今回の基準で随時書き直していきたいと思います。 |
|
こちらこそ、レビューありがとうございました。それではマージいたします。 |
CodeClimateでCognitive Complexity（コードの複雑さ）が大きいと指摘されたメソッドを分割して、処理を単純にしました。