Implement 'wc' in Ruby by blacktoad30 · Pull Request #9 · blacktoad30/ruby-practices

blacktoad30 · 2025-03-30T06:26:39Z

05.wc/lib/wc_methods.rb: New file.
05.wc/wc.rb: New file.

* 05.wc/lib/wc_methods.rb: New file. * 05.wc/wc.rb: New file.

yoshitsugu

コメントしました！

yoshitsugu · 2025-04-01T23:07:23Z

05.wc/wc.rb

+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require_relative './lib/wc_methods'


あまり今回はファイルを分ける意味がないかなと思ったんですが、どうでしょうか。
たしかに実行ファイルとライブラリとして使用するファイルを分けることはよくあるパターンではあります。とはいえ今回は簡単なスクリプトですし、1ファイルで全体を見渡せたほうがいいかなと思いました。

分けた理由は、定義した各々のメソッドの動作を irb -r ./lib/wc_methods.rb で都度確認できるようにしたかったからです。
また、今回は作成および提出できなかったものの、テストコードで使用することも考えていました。

遅くなりましたが、なるほどですね。
そういう場合も1ファイルでできるようなイディオム的な書き方もあるにはあります。

if __FILE__ == $0 errno = main(ARGV) exit(errno) end

参考: https://stackoverflow.com/questions/4687680/what-does-if-file-0-mean-in-ruby

一旦ここではこのままでよしとします。

yoshitsugu · 2025-04-01T23:16:41Z

05.wc/lib/wc_methods.rb

GitHub上には可視化されていないですが、Page breakがところどころはいっているようですね、これは意図的ですか？

エディタでカーソルを移動する際に、目印のように使用していました。
削除いたします。

yoshitsugu · 2025-04-01T23:18:32Z

05.wc/lib/wc_methods.rb

+
+def parse_args(args)
+  copy_args = args.dup
+  optsym_by_opt = { 'l' => :newline, 'w' => :word, 'c' => :byte }


変数名がややこしく感じます。 option_name_to_long_name_map とかもうちょっと説明的でよいです。

また、この値は変わらないものだと思うので、定数にするとよいかなと思います。
https://docs.ruby-lang.org/ja/latest/doc/spec=2fvariables.html#const

コード上不変であることがわかりやすい

再代入すると警告がでる

などのメリットがあります。

yoshitsugu · 2025-04-01T23:19:28Z

05.wc/lib/wc_methods.rb

+  copy_args = args.dup
+  optsym_by_opt = { 'l' => :newline, 'w' => :word, 'c' => :byte }
+
+  optarg_by_opt = OptionParser.new.getopts(copy_args, 'lwc')


こちらの変数名も parsed_options くらいでいいのではと思います。

yoshitsugu · 2025-04-01T23:23:24Z

05.wc/lib/wc_methods.rb

+  optarg_by_opt = OptionParser.new.getopts(copy_args, 'lwc')
+
+  optarg_by_opt.transform_keys!(optsym_by_opt)
+  optarg_by_opt.value?(true) || optarg_by_opt.transform_values! { |_| true }


Ruby的には

optarg_by_opt.transform_values! { |_| true } unless optarg_by_opt.any?

のような書き方のほうが自然かなと思います。

yoshitsugu · 2025-04-01T23:44:50Z

05.wc/lib/wc_methods.rb

+rescue SystemCallError => e
+  count = e.is_a?(Errno::EISDIR) ? WcCount.new : nil


このあたり、Errorを条件分岐のように使わず、たとえば File#directory? を使うなど
https://docs.ruby-lang.org/ja/latest/method/File/s/directory=3f.html

Rubyの豊富なメソッドを使うことで代用できないでしょうか。一般的なベストプラクティスとしてErrorは「想定外のことが起きた」ことに用いられるべきで、このように想定されるものがある場合はできるだけ通常の条件分岐を使うべき、とされることが多いです。

yoshitsugu · 2025-04-01T23:48:45Z

05.wc/lib/wc_methods.rb

+  fd = valid_path == '-' ? $stdin.fileno : IO.sysopen(valid_path.to_s)
+
+  IO.open(fd) { |io| wc_count_for_io(io.set_encoding('ASCII-8BIT')) }


単に File.open や File.read を使うとか、ファイルディスクリプタを生で取り回さない方法でできないか考えてみてください。

yoshitsugu · 2025-04-01T23:51:40Z

05.wc/lib/wc_methods.rb

+  path, message = wc_result.deconstruct_keys(%i[path message]).values
+
+  warn "wc: #{path}: #{message}"


単に

warn "wc: #{wc_result[:path]}: #{wc_result[:message]}"

でよい気がします。

yoshitsugu · 2025-04-02T00:33:09Z

05.wc/lib/wc_methods.rb

+  print_counts =
+    wc_result.count.deconstruct_keys(print_opts & WcCount.members).values
+
+  padding_width >= 2 &&
+    print_counts.map! do |count_value|
+      count_value.to_s.rjust(padding_width)
+    end


available_count_keys = print_opts & WcCount.members formatted_counts = wc_result.count.values_at(**available_count_keys).map { _1.to_s.rjust(padding_width }

くらいでしょうか。ポイントとしては

print_opts & WcCount.members はちょっと読みづらいなと感じるので一旦名前をつけた

padding_width == 1 の場合に rjust しても出力としては変わらないはずなので省略(もちろん無駄な計算は増えますが、このくらいは問題ないと判断)

yoshitsugu · 2025-04-02T00:33:57Z

05.wc/lib/wc_methods.rb

+      count_value.to_s.rjust(padding_width)
+    end
+
+  print_opts.include?(:path) &&


上と同様ですが、基本Rubyではあまり && や || を使って条件分岐のようなことはやらず、 if や unless を使ってやることが多いです。
print_opts.include?(:path) と path == '-' で同等のことを別の方法で判断しているように見えるので、どちらかに統一したほうがよいように思いました。
そもそも path を nil にすれば、この場合分けも不要になるかなと思います。

* 05.wc/lib/wc_methods.rb (#main): Return exit status. (#print_wc_data): No longer return exit status.

* 05.wc/lib/wc_methods.rb (#wc_count_with_message) <File.read> (#wc_count_for_valid_path) <File.open>: Change from singleton method of `IO` class. (#wc_count_for_valid_path) <IO.sysopen>: No longer use.

* 05.wc/lib/wc_methods.rb (OPTION_STRING, WORD_COUNT_TYPES) (OPTION_NAME_TO_WORD_COUNT_TYPE): New constant. (WcCount) <Data.define>: Use `WORD_COUNT_TYPES`. (#main): Return enabled option keys only. (#parse_args): Extract constant. Simplify option parsing.

* 05.wc/lib/wc_pathname.rb: New file. (WORD_COUNT_TYPES, IO_BUFFER_SIZE): New constant. (WcPathname): New class. (#word_count_for_io, #word_count_for_string) (#word_count_for_string_per_type): New method for `WcPathname#word_count`.

* 05.wc/lib/wc_methods.rb: Load `05.wc/lib/wc_pathname.rb`. (WORD_COUNT_TYPES): Remove duplicate constant. (WcData, WcResult, WcCount): Remove `Data` subclass. (#readable_files?, #wc_results, #wc_count_with_message) (#wc_count_for_valid_path, #wc_count_for_io, #wc_results_total) (#print_wc_data, #padding_width, #simple_output?) (#include_non_regular_files, #print_warn, #format_wc_result): Remove method. (#main): Use new method. Return error if `$stdin` is directory. (#extract_word_count_types, #displayed_output_format, #adjust_digit) (#word_count_results, #with_system_call_error_handler) (#word_count_error_handler, #print_word_count_result): New method.

* 05.wc/lib/word_count.rb: New file. (WordCount, WordCount::IO, WordCount::String): New module. (IO): Mix-in `WordCount::IO`. (String): Mix-in `WordCount::String`.

* 05.wc/lib/wc_methods.rb (OPTION_NAME_TO_WORD_COUNT_TYPE) <WordCount::TYPES>: Rename constant. (#main) <word_count_types> (#displayed_output_format) <one_type_one_operand>: Use `WordCount#.extract_types`. (#extract_word_count_types): Remove method. * 05.wc/lib/wc_pathname.rb: Load `05.wc/lib/word_count.rb`. (WORD_COUNT_TYPES, IO_BUFFER_SIZE): Remove duplicate constant. (WcPathname#word_count): Use `WordCount::IO#word_count`. <WordCount::TYPES>: Rename constant. (#word_count_for_io, #word_count_for_string) (#word_count_for_string_per_type): Remove method.

* 05.wc/lib/wc_pathname.rb (WcPathname): Does not inherit from `Pathname`. (WcPathname::USE_FILETEST_MODULE_FUNCTIONS): New private constant. (WcPathname#initialize): Set `@path` only. (WcPathname#inspect, WcPathname#to_s, WcPathname#open) (WcPathname#exist?, WcPathname#directory?, WcPathname#file?) (WcPathname#readable?, WcPathname#size): New method. (WcPathname#regular_file?): Remove method. Migrate to `WcPathname#file?`. (WcPathame#regular_file_size?, WcPathame#exist_non_regular_file?) (WcPathame#readable_non_directory?, WcPathame#word_count): Use new method.

* 05.wc/lib/wc_methods.rb (#word_count_results) <count, message>: Use `WcPathname#word_count`, `WcPathname#word_count_message`. (#with_system_call_error_handler, #word_count_error_handler): Remove method. * 05.wc/lib/wc_pathname.rb (WcPathname#word_count): Return the corresponding value instead of the exception (`Errno::ENOENT`, `Errno::EACCES`, `Errno::EISDIR`) that is thrown when reading a file fails. (WcPathname#word_count_message): New method.

* 05.wc/lib/wc_methods.rb (#main) <counts>: No longer use `#word_count_results`. (#word_count_results): Remove unused method. * 05.wc/lib/wc_pathname.rb (WcPathname#to_s) (WcPathname#regular_file_size?): Remove unused method. (WcPathname#word_count_result): New method. (WcPathname#word_count): Return the word count only. Change to private method. (WcPathname#word_count_message): Remove method. Integrated to `WcPathname#word_count_result`.

* 05.wc/lib/wc_pathname.rb (WcPathname#word_count): Remove the second argument. * 05.wc/lib/word_count.rb (WordCount::IO#word_count) (WordCount::String#word_count) <file_size>: The second argument is no longer used.

* 05.wc/lib/wc_methods.rb: Load `05.wc/lib/word_count.rb`. (#main) <wc_paths>: Use `WordCount::Pathname.new`. * 05.wc/lib/wc_pathname.rb: Delete file. <WcPathname>: Migrate to `WordCount::Pathname`. * 05.wc/lib/word_count.rb: (WordCount::Pathname): New class.

* 05.wc/lib/word_count.rb (WordCount::IO#word_count): Extract method. Change word counting. (WordCount::IO#each_buffer): New extracted method. (WordCount::String#word_count_per_type) <:word>: Count words that contain only ASCII printable characters.

* 05.wc/lib/wc_methods.rb (#main) <wc_paths>: If there is no file operand, an instance is created with no arguments. * 05.wc/lib/word_count.rb:(WordCount::Pathname#initialize) <@path>: This instance variable must be `String`. (WordCount::Pathname#to_path): New method. (WordCount::Pathname#stdin?, WordCount::Pathname#inspect) (WordCount::Pathname#open, WordCount::Pathname#exist?) (WordCount::Pathname#directory?, WordCount::Pathname#file?) (WordCount::Pathname#readable?, WordCount::Pathname#size): Use `WordCount::Pathname#to_path`. (WordCount::Pathname#word_count_result) <path>: Substitute 'standard input' if `@path` is empty string.

As far as I know, there are no methods in Ruby's built-in classes or standard libraries that can detect in advance whether a file will result in `Errno::EPERM`. The only way to deal with this is to use exception handling. * 05.wc/lib/wc_methods.rb (#main): Error status is determined by whether each element in `results` has an error message. <counts>: Remove variable. <results>: New variable. <count_total>: Use `results`. * 05.wc/lib/word_count.rb (WordCount::Pathname#readable_non_directory?): Remove unused method. (WordCount::Pathname#word_count_result): Add `Errno::EPERM` exception handling.

* 05.wc/lib/wc_methods.rb (#displayed_output_format) <:bytesize>: Rename symbol. * 05.wc/lib/word_count.rb (WordCount::Pathname#word_count) (WordCount::String#word_count_per_type) <:bytesize>: Rename symbol.

* 05.wc/lib/word_count.rb (WordCount::String#word_count): Integrate `WordCount::String#word_count_per_type`. (WordCount::String#word_count_per_type): Remove method. (WordCount::String#newline, WordCount::String#word): New method.

* 05.wc/lib/wc_methods.rb (#displayed_output_format): Use `#output_format_string`. (#output_format_string): New method.

blacktoad30 · 2025-09-11T23:53:03Z

@yoshitsugu ご無沙汰しております。再提出が遅くなってしまい、申し訳ございません。
改めてレビューのほど、よろしくお願いいたします。

yoshitsugu

ちょっと難しく考えすぎかなと思います。
プラクティスなので、ある程度エッジケースは落ちていてもかまいせん。
まずはシンプルなコードを書けるようになったほうが実務としてはよいと思います。このプラクティスのコードはクラスを作るまでもないシンプルなコードで十分なはずです。とりあえずやりたいことを整理して、シンプルに書いてみてください。

yoshitsugu · 2025-09-16T00:14:39Z

05.wc/wc.rb

+#!/usr/bin/env ruby
+# frozen_string_literal: true
+
+require_relative './lib/wc_methods'


遅くなりましたが、なるほどですね。
そういう場合も1ファイルでできるようなイディオム的な書き方もあるにはあります。

if __FILE__ == $0 errno = main(ARGV) exit(errno) end

参考: https://stackoverflow.com/questions/4687680/what-does-if-file-0-mean-in-ruby

一旦ここではこのままでよしとします。

yoshitsugu · 2025-09-16T00:16:27Z

05.wc/lib/wc_methods.rb

+OPTION_NAME_TO_WORD_COUNT_TYPE = OPTION_STRING.chars.zip(WordCount::TYPES).to_h.freeze
+
+def main(args)
+  displayed_items = parse_args(args)


displayed_items だと何が出力されたものに見えるのですが、おそらくここではオプションをパースした結果ですよね。 enabled_options とかのほうがわかりやすいのではと思ったんですが、どうでしょうか。

yoshitsugu · 2025-09-16T00:21:15Z

05.wc/lib/word_count.rb

+class IO
+  include WordCount::IO
+end
+
+class String
+  include WordCount::String
+end


オープンクラスを活用してのクラスの書き換えは実務では基本的にやらないほうがよいです。
書き捨てのコードなどとりあえず手早く動かしたいコードならよいですが、こういった変更はあとから追いづらくてメンテナンスがしづらくなりがちです。
やるとしたらRefinementを検討するなどしたほうがよいです。
https://techracho.bpsinc.jp/hachi8833/2017_03_23/37464

ここではシンプルに別メソッドとして定義したほうがよいのではと思います。

yoshitsugu · 2025-09-16T00:23:08Z

05.wc/lib/word_count.rb

+    word_count_types.to_h do |type|
+      case type
+      when *WordCount::TYPES
+        [type, __send__(type)]


__send__ などメタプログラミング的なメソッドも極力避けたほうがよいです。やるとしたら public_send を使ってせめてpublicなインターフェースにしかアクセスできないようにする、などの方法をとったほうがよいです。
なんとなく、こういうかっこいい書き方をしたくなる気持ちは僕もわかるのですが、結局あとから見ると読みづらく、仕事としてのコードとしては適さないことが多いです。

yoshitsugu · 2025-09-16T00:25:28Z

05.wc/lib/word_count.rb

+  def word_count(word_count_types = WordCount::TYPES, bufsize: BUFFER_SIZE)
+    each_buffer(bufsize).inject(:<<).to_s.word_count(word_count_types)
+  end
+
+  def each_buffer(limit = BUFFER_SIZE)
+    return to_enum(__callee__, limit) unless block_given?
+
+    loop do
+      yield readpartial(limit)
+    rescue EOFError
+      break
+    end
+
+    self
+  end


このあたりもかなり複雑な書き方をされていますが、単に String のメソッドである length や bytes などで対応可能なものしかプラクティスとしては設定していません。
https://docs.ruby-lang.org/ja/latest/class/String.html

まずはこれらのメソッドで対応できないか、見てみてください。

blacktoad30 · 2025-10-22T06:21:02Z

@yoshitsugu お疲れさまです。
前回のペアプロでのアドバイスを元に、一旦別のブランチにて修正いたしました。

https://github.com/blacktoad30/ruby-practices/blob/1cd1c234874145d40772ef85bcca31cfcd5f9b27/05.wc/wc.rb

コミット履歴があまり綺麗では無いので、必要であれば後ほど修正いたします。
お手数ですが、ご確認のほどよろしくお願いいたします。

Implement 'wc' in Ruby

f329d43

* 05.wc/lib/wc_methods.rb: New file. * 05.wc/wc.rb: New file.

yoshitsugu reviewed Apr 2, 2025

View reviewed changes

blacktoad30 added 19 commits July 4, 2025 22:07

refactor: Delete formfeed character

f61c5ec

refactor: Separate responsibility for returning exit status

3f3ff39

* 05.wc/lib/wc_methods.rb (#main): Return exit status. (#print_wc_data): No longer return exit status.

refactor: Use singleton method of File class

eda1b68

* 05.wc/lib/wc_methods.rb (#wc_count_with_message) <File.read> (#wc_count_for_valid_path) <File.open>: Change from singleton method of `IO` class. (#wc_count_for_valid_path) <IO.sysopen>: No longer use.

feat: Define WcPathname class

b010690

* 05.wc/lib/wc_pathname.rb: New file. (WORD_COUNT_TYPES, IO_BUFFER_SIZE): New constant. (WcPathname): New class. (#word_count_for_io, #word_count_for_string) (#word_count_for_string_per_type): New method for `WcPathname#word_count`.

feat: Define WordCount module

d09bfa9

* 05.wc/lib/word_count.rb: New file. (WordCount, WordCount::IO, WordCount::String): New module. (IO): Mix-in `WordCount::IO`. (String): Mix-in `WordCount::String`.

refactor: Reduce unnecessary method argument

14cccf1

* 05.wc/lib/wc_pathname.rb (WcPathname#word_count): Remove the second argument. * 05.wc/lib/word_count.rb (WordCount::IO#word_count) (WordCount::String#word_count) <file_size>: The second argument is no longer used.

fix: Count words correctly

7a74cac

* 05.wc/lib/word_count.rb (WordCount::IO#word_count): Extract method. Change word counting. (WordCount::IO#each_buffer): New extracted method. (WordCount::String#word_count_per_type) <:word>: Count words that contain only ASCII printable characters.

refactor: Rename :byte to :bytesize

3f63735

* 05.wc/lib/wc_methods.rb (#displayed_output_format) <:bytesize>: Rename symbol. * 05.wc/lib/word_count.rb (WordCount::Pathname#word_count) (WordCount::String#word_count_per_type) <:bytesize>: Rename symbol.

refactor: Extract format string generation into dedicated method

5e48a33

* 05.wc/lib/wc_methods.rb (#displayed_output_format): Use `#output_format_string`. (#output_format_string): New method.

yoshitsugu reviewed Sep 16, 2025

View reviewed changes

		rescue SystemCallError => e
		count = e.is_a?(Errno::EISDIR) ? WcCount.new : nil

		fd = valid_path == '-' ? $stdin.fileno : IO.sysopen(valid_path.to_s)

		IO.open(fd) { \|io\| wc_count_for_io(io.set_encoding('ASCII-8BIT')) }

		path, message = wc_result.deconstruct_keys(%i[path message]).values

		warn "wc: #{path}: #{message}"

Conversation

blacktoad30 commented Mar 30, 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!

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 Sep 11, 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!

blacktoad30 commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

blacktoad30 commented Oct 22, 2025 •

edited

Loading