wc command #10
wc command #10
Conversation
05.wc/wc.rb
Outdated
| def file_stats | ||
| ARGV.map do |file_name| | ||
| contents = IO.readlines(file_name).join | ||
| stats = build_data(contents) | ||
| [file_name, { lines: stats[:lines], chars: stats[:chars], bytes: stats[:bytes] }] | ||
| end.to_h | ||
| end | ||
|
|
||
| def build_data(contents) | ||
| lines = contents.scan(/(\n|\r)/).count | ||
| chars = contents.split(/\s+/).count | ||
| bytes = contents.size | ||
| { lines:, chars:, bytes: } | ||
| end |
There was a problem hiding this comment.
これでも悪くはないのですがmain -> file_stats -> build_data と main -> build_data の依存があるのが気になりますね。あるメソッドを呼び出す側はそのメソッドの抽象度で記述されているべきで、さらに中で使っているメソッドを使うのは抽象度があっていないように思います。雑に例えると、都道府県を列挙しているのにいきなり市区町村がまじってたら変ですよね。そんなイメージです。
今回の場合、標準入力からの入力も [] で1要素の配列として扱う、などとすればもうちょっと統一的に扱えるかなと思います。
05.wc/wc.rb
Outdated
| formatted_stats = if ARGV.size == 1 | ||
| format_file_stats(file_stats:, filter_params:) | ||
| elsif ARGV.size >= 2 | ||
| file_stats_with_total = calcurate_total file_stats | ||
| format_file_stats(file_stats: file_stats_with_total, filter_params:) | ||
| else | ||
| input = readlines.join | ||
| stat = build_data(input) | ||
| format(stat:, filter_params:) |
There was a problem hiding this comment.
条件分岐はできるだけ局所的にするとどこが違うのかがわかりやすいです。
formatted_stats = (トータルの表示) if ARGV.size >= 2
formatted_stats += (トータル以外の表示)としたほうが、トータルの表示のみ、ファイル数が2以上のときに必要なんだな、ということがわかりやすいかと思います。また、この方がDRYにしやすい、というメリットもあります。
05.wc/wc.rb
Outdated
| end | ||
|
|
||
| def file_stats | ||
| ARGV.map do |file_name| |
There was a problem hiding this comment.
ARGV に依存するのはできるだけ main などのエントリポイントとなるものだけにして、そこから呼び出されるメソッドには引数として渡したほうがよいでしょう。そうすると ARGV 以外で引数を指定するようになったときも main だけの変更で済みます。同様に、純粋なプログラミング上の計算に閉じないもの、たとえば標準入力/出力、DB操作、Webへのアクセスなどはできるだけ依存を内部的なロジックの中に作らないことがベストプラクティスになっています。
05.wc/wc.rb
Outdated
| end.max | ||
| end | ||
|
|
||
| __FILE__ == $PROGRAM_NAME && main |
There was a problem hiding this comment.
&& を条件分岐的に使うのはやめましょう。 if を使うようにしましょう。
05.wc/wc.rb
Outdated
| contents = [readlines.join] | ||
| source_names = [:stdin] | ||
| else | ||
| contents = ARGV.map{ |file_name| IO.readlines(file_name, nil).pop } |
There was a problem hiding this comment.
IO.readlines(file_name, nil).pop の部分、ややこしく見えますね。
File.read(file_name) で十分ではないでしょうか？
ファイルを一括で文字列で読み込む、というのは多くの人がやるであろう処理です。Rubyはいろいろな便利メソッドが用意されているので、そういう場合は該当するメソッドがないか、探してみてください。
https://docs.ruby-lang.org/ja/latest/class/File.html
05.wc/wc.rb
Outdated
| end | ||
| contents_with_source_name = source_names.zip(contents).to_h | ||
| stats = calcurate_stats(contents_with_source_name) | ||
| col_width = stats.has_key?(:stdin) ? 7 : max_col_width(stats) |
There was a problem hiding this comment.
この 7 は定数として名前をつけてあげるとよいです。
コード中の数値は時間が経つと何でこの値なのかのコンテキストが失われやすく、マジックナンバーと呼ばれたりします。
05.wc/wc.rb
Outdated
| stats.map do |name, stat| | ||
| filtered_stat_values = stat.filter { |k| filter_params.keys[0..].include?(k) }.values | ||
| formatted_stat_values = filtered_stat_values.map { |v| v.to_s.rjust(col_width) }.join(' ') | ||
| name == :stdin ? formatted_stat_values : "#{formatted_stat_values} #{name}" |
There was a problem hiding this comment.
ここで条件分岐が発生してしまっているのがややもったいなく思いますね。
修正する必要まではないですが、 一案として、そもそものデータ構造を { lines: (数値), words: (数値), bytes: (数値), file_name: (文字列 or nil) } としてあげれば、ここは "#{formatted_stat_values} #{name}" のみでよいかなと思いました。間の空白分出力されるのを避けるなら [formatted_stat_values, name].compact.join(' ') ですかね。
05.wc/wc.rb
Outdated
|
|
||
| def build_data(contents, file_name) | ||
| lines = contents.scan(/(\n|\r)/).count | ||
| words = contents.split(/[\s^ ]+/).count |
There was a problem hiding this comment.
細かいですが、
test
のように最初に空行があるファイルだとカウントがおかしくなりそうです。
05.wc/wc.rb
Outdated
| puts formatted_stats | ||
| end | ||
|
|
||
| def calcurate_stats(contents_with_filename) |
3 participants
ご確認お願いします。