wcコマンドの修正 by blacktoad30 · Pull Request #10 · blacktoad30/ruby-practices

blacktoad30 · 2025-10-23T04:21:14Z

ペアプロでいただいたアドバイスを元に修正した。

Define methods directly without defining separate classes or modules. * 05.wc/lib/wc_methods.rb: Remove file. * 05.wc/lib/word_count.rb: Remove file. * 05.wc/wc.rb: Merge into this file.

* 05.wc/wc.rb (#word_count): 上から順番に読めるようになった。 (#word_count_for_string, #word_count_for_string_per_type): Remove method. (#count_newline, #count_word, #count_bytesize): New method.

yoshitsugu

コメントしました！

yoshitsugu · 2025-10-23T23:15:03Z

05.wc/wc.rb

+require 'optparse'

-errno = main(ARGV)
+OPTION_STRING = 'lwc'


AVAILABLE_OPTION_CHARS とかですかね。下の DEFAULT_OPTION_CHARS と同じ OPTION_CHARS だとわかるほうがよいかなと思います。

yoshitsugu · 2025-10-23T23:22:53Z

05.wc/wc.rb

+
+  option_chars = extract_option_chars(options)
+
+  format_string = generate_format_string(args, option_chars)


ここでは args が OptionParser.getopts によって破壊的に変更されていることが前提になっていると思います。こういった実行順により暗黙的に状態が変わってしまうようなコードはあとあと見つけづらいバグを埋めこんでしまう可能性が高いため、できるだけ状態変化が明示的にわかるようなコードにしたほうがよいです。
今回であればたとえば、

def parse_commandline_options options = OptionParser.getopts(ARGV, OPTION_STRING) [options, ARGV] end

のように、破壊的変更と外部環境(ARGV)への依存を閉じ込めたメソッドを作ってあげて、

options, filenames = parse_commandline_options

とするなど、できるだけ明示的なコードになるとよいと思います。

yoshitsugu · 2025-10-23T23:25:12Z

05.wc/wc.rb

+  option_chars = extract_option_chars(options)
+
+  format_string = generate_format_string(args, option_chars)
+  results = word_count_results(args, option_chars)


何らかの働きをするメソッドは動詞 ( print, load, show など) や動詞 + 目的語 (print_filenames など) という命名にするとわかりやすいです。今回ですと、 count_words_by_options などですかね。

yoshitsugu · 2025-10-23T23:43:53Z

05.wc/wc.rb

+      option_chars.to_h { [_1, 0] }
+                  .merge!(*results.filter_map { _1[:count] }) { |_, total, count| total + count }


ここもちょっとややこしいので、もうちょっと素朴に書けるかなと思います。

init_value_for_total = option_chars.to_h { [_1, 0] } count_total = results.each_with_object(init_value_for_total) do |result, total| option_chars.each do |option| total[option] += result[:count][option] end end

などですかね。

yoshitsugu · 2025-10-23T23:46:14Z

05.wc/wc.rb

+end
+
+def word_count_results(parsed_args, option_chars = DEFAULT_OPTION_CHARS)
+  paths = parsed_args.empty? ? ['-'] : parsed_args


実際に - というファイル名のファイルがあったら困るので、 - は使わないほうがよいと思います。 nil にするとか、文字列じゃないものにしましょう。

yoshitsugu · 2025-10-23T23:51:11Z

05.wc/wc.rb

+  word_count['l'] = count_newline(lines) if option_chars.include?('l')
+  word_count['w'] = count_word(lines) if option_chars.include?('w')
+  word_count['c'] = (file?(path) ? size(path) : count_bytesize(lines)) if option_chars.include?('c')


効率は落ちますが、ここではとりあえず全部計算しておいて、オプションによる条件づけは表示時のみでもよいですよ。

yoshitsugu · 2025-10-23T23:52:40Z

05.wc/wc.rb

+    num = 0
+    line.split { num += 1 if _1.match?(/[[:graph:]]/) }
+    num


これってたとえば lines.flat_map(&:split).compact.count とかではだめなんでしょうか。
ただ空白で分割して数える、のほうが素直に思いました。

yoshitsugu · 2025-10-24T00:04:15Z

05.wc/wc.rb

+end
+
+def count_newline(lines)
+  lines.sum { |line| line.count("\n") }


lines.count ではダメなんでしたっけ?

yoshitsugu · 2025-10-24T00:06:08Z

05.wc/wc.rb

+def calc_digit(parsed_args)
+  paths = parsed_args.empty? ? ['-'] : parsed_args
+
+  default_digit = paths.any? { exist_non_regular_file?(_1) } ? 7 : 1


今回はプラクティスなので、通常ファイル以外がわたされた場合はあまり考えなくてよいです。
また、やるとしてもただエラー表示するだけでよいかなと思います。

yoshitsugu · 2025-10-24T00:09:06Z

05.wc/wc.rb

+  format_string = Array.new(enabled_option_count, "%#{digit}d")
+
+  format_string << '%s' unless parsed_args.empty?
+
+  format_string.join(' ')


%dや%sなどの表記は慣れていないと読みづらいので、 rjust などで対応できるならそちらのほうがよいのですが、どうでしょうか。

* 05.wc/wc.rb (#main) <init_value_for_total, count_total>: Separate the initial value and the total processing. (#word_count_results): Create `Hash` simply.

* 05.wc/wc.rb (#main): No arguments are required as `ARGV` is used within the method. (#parse_commandline_options): New method. (#generate_format_string, #calc_digit, #word_count_results): Rename argument.

* 05.wc/wc.rb (#word_count) <buf>: Use `String` instead of `[String]`. (#count_newline, #count_word, #count_bytesize): Stop using `Array#sum`.

* 05.wc/wc.rb (#calc_digit, #word_count_results): Stop treating '-' as standard input. <paths>: Do not rewrite this argument. (#stdin?, #exist_non_regular_file?, #exist?, #regular_file_size) (#file?, #size): Remove unused method. (#word_count): Change the return value to nested `Hash`.

Use `Hash#values_at` or `String#rjust` instead of. * 05.wc/wc.rb (#generate_format_string, #word_count_results) (#print_word_count_result): Remove method. (#main): Extract the process to output the count and its total. <counts>: New variable. (#count_newline_word_byte): Similar to `#word_count`, but counts all items (newline, word, and byte). (#print_counts, #stdin?, #total_counts): New method. (#calc_digit): Change arguments and processing for use with `#print_counts`.

blacktoad30 · 2025-11-03T05:09:14Z

@yoshitsugu お疲れさまです。レビューしていただき、ありがとうございます!
コメントを参考に修正しました。以下の理由により、未修正の部分が2箇所あります。

#main

AVAILABLE_OPTION_CHARS とかですかね。下の DEFAULT_OPTION_CHARS と同じ OPTION_CHARS だとわかるほうがよいかなと思います。

DEFAULT_OPTION_CHARS としたのは、 String#chars で得られた [String] (ただし、全ての要素は長さ1の String) であるからです。
Rubyのソースコードを辿れば char[] に行き着くのかもしれませんが、私は string と chars はニュアンスが違うのかなと考え、命名いたしました。
#count_word

これってたとえば lines.flat_map(&:split).compact.count とかではだめなんでしょうか。ただ空白で分割して数える、のほうが素直に思いました。

GNU Coreutils の wc コマンドの挙動を参考に作成したからです。 (別のメソッドにて、引数の - を標準入力として扱っていたのもそのためです。)
空白文字で区切られたバイト列に、表示可能文字がまったく含まれていない場合はカウントされていないことを確認しています。

yoshitsugu · 2025-11-03T09:05:29Z

DEFAULT_OPTION_CHARS としたのは、 String#chars で得られた [String] (ただし、全ての要素は長さ1の String) であるからです。
Rubyのソースコードを辿れば char[] に行き着くのかもしれませんが、私は string と chars はニュアンスが違うのかなと考え、命名いたしました。

なるほど。つまり String のときはRubyのStringそのものをあらわしたい。 CHARS のときは String の配列、かつ各要素の String は長さが1であることを意味したい、ということですね。意図して使い分けている、ということであれば一旦そのままで大丈夫です 👌
一応、RubyっぽさでいうとRubyはduck typingな言語ですので、そもそもの発想としてあまり型を明示しないほうがそれっぽいです。

GNU Coreutils の wc コマンドの挙動を参考に作成したからです。 (別のメソッドにて、引数の - を標準入力として扱っていたのもそのためです。)
空白文字で区切られたバイト列に、表示可能文字がまったく含まれていない場合はカウントされていないことを確認しています。

うーん、それは僕の当初の指摘に答えていないように思います。「こういう実装が既にあるからこうした」というのは、その説明だけ見ると、自分の考えがないように思います。例えば、「今の実装にあるメリットはXXXで、lines.flat_map(&:split).compact.count にするとそのメリットが失なわれてしまう。結果としては同じかもしれないが、実装としては XXX を重視したいので、そのままにした」などの説明がないと今の説明だと無責任な印象になりますね。
ここではそういう無責任な意図が実際にあったかどうかは問題ではなく、そう見える、というところがポイントです。実務でも「この実装がこうなっていたからこうしました」というのは他の候補がないときのとっかかりとしては一定の価値がありますが、今のように対抗となる候補がある場合の理由としては成り立たないと言われるかなと思います。

blacktoad30 added 5 commits October 9, 2025 14:42

fix: Merge into one file

cb2cdc9

Define methods directly without defining separate classes or modules. * 05.wc/lib/wc_methods.rb: Remove file. * 05.wc/lib/word_count.rb: Remove file. * 05.wc/wc.rb: Merge into this file.

refactor: word_count

85c5b63

* 05.wc/wc.rb (#word_count): 上から順番に読めるようになった。 (#word_count_for_string, #word_count_for_string_per_type): Remove method. (#count_newline, #count_word, #count_bytesize): New method.

Merge branch 'my-wc-fix--simplify-word_count' into my-wc-fix--simplify

a00bce0

fix: 引数解析の結果をなるべくいじらない

3fb9b6a

Merge branch 'my-wc-fix--simplify-parse_args' into my-wc-fix--simplify

1cd1c23

yoshitsugu reviewed Oct 24, 2025

View reviewed changes

blacktoad30 added 5 commits November 3, 2025 13:12

refactor: Make some code easier to understand

957298f

* 05.wc/wc.rb (#main) <init_value_for_total, count_total>: Separate the initial value and the total processing. (#word_count_results): Create `Hash` simply.

refactor: Use ARGV

b024333

* 05.wc/wc.rb (#main): No arguments are required as `ARGV` is used within the method. (#parse_commandline_options): New method. (#generate_format_string, #calc_digit, #word_count_results): Rename argument.

refactor: Use String for counting

e9ade88

* 05.wc/wc.rb (#word_count) <buf>: Use `String` instead of `[String]`. (#count_newline, #count_word, #count_bytesize): Stop using `Array#sum`.


		option_chars = extract_option_chars(options)

		format_string = generate_format_string(args, option_chars)

		option_chars.to_h { [_1, 0] }
		.merge!(*results.filter_map { _1[:count] }) { \|_, total, count\| total + count }

Conversation

blacktoad30 commented Oct 23, 2025

Uh oh!

yoshitsugu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blacktoad30 commented Nov 3, 2025

Uh oh!

yoshitsugu commented Nov 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants