wcコマンドの実装 by kutimiti1234 · Pull Request #12 · kutimiti1234/ruby-practices

kutimiti1234 · 2024-07-07T09:22:09Z

標準入力を受け取れるように設定
複数ファイルを吹けとれるように設定
"-l" "-w" "-c"のオプションに対応

JunichiIto

コメントしました！

JunichiIto · 2024-07-13T07:29:36Z

05.wc/wc.rb

+  print_output(output_text, max_width)
+end
+
+DISPLAY_ADJUST_NUMBER = 1


number以外の名前がいいかも？
https://bootcamp.fjord.jp/pages/number-count-size-length

また定数はrequireの下あたりにまとめられることが多いですね

JunichiIto · 2024-07-13T07:30:53Z

05.wc/wc.rb

+  { line_display_width: max_lines_width, word_display_width: max_words_width, bytesize_display_width: max_bytesizes_width }
+end
+
+def parse_and_remove_options(argv)


removeってどの処理を指してるんでしょう？

JunichiIto · 2024-07-13T07:31:26Z

05.wc/wc.rb

+require 'optparse'
+
+def main
+  argv_options = parse_and_remove_options(ARGV)


argv_ はいらないかも
https://bootcamp.fjord.jp/pages/avoid-too-specific-var-name

JunichiIto · 2024-07-13T07:31:53Z

05.wc/wc.rb

+
+def main
+  argv_options = parse_and_remove_options(ARGV)
+  output_text = []


配列やハッシュは複数形にすることが多いです

JunichiIto · 2024-07-13T07:32:51Z

05.wc/wc.rb

+  argv_options = parse_and_remove_options(ARGV)
+  output_text = []
+  ARGF.each(nil) do |input_text|
+    wc_info = []


最後に to_h するのではなく、最初からハッシュとして扱う方が自然だと思います

Suggested change

wc_info = []

wc_info = {}

JunichiIto · 2024-07-13T07:40:24Z

05.wc/wc.rb

+  line = output_line[:line]&.to_s&.rjust(max_width[:line_display_width])
+  word = output_line[:word]&.to_s&.rjust(max_width[:word_display_width])
+  bytesize = output_line[:bytesize]&.to_s&.rjust(max_width[:bytesize_display_width])
+  filename = output_line[:filename] == '-' ? '' : output_line[:filename]


ファイル名が "-" になるケースってどんなときですか？

ARGFで標準入力を受け付ける場合に、スクショの通り"-"となってしまいます。

なるほどー。
その情報が第三者にはぱっとわからないので（＝マジックナンバーならぬマジックストリング）、情報を付加した方がいいですね。

そして今回の提出物ではこの処理がなくなってますね。これはなぜ？ 🤔

こちら削除行った理由としては、ARGF以外の処理で受け取れればもしかして標準入力を"-"として受け取らずにすむのではないかと考えて、一旦削除していたためです。

その情報が第三者にはぱっとわからないので（＝マジックナンバーならぬマジックストリング）、情報を付加した方がいいですね。

こちらについて今回のpushでより説明的な形で実装しました。

JunichiIto · 2024-07-13T07:40:53Z

05.wc/wc.rb

+
+def get_max_width(output_text)
+  max_lines_width = output_text.map { |entry| entry[:line].to_s.length + DISPLAY_ADJUST_NUMBER }.max || 0
+  max_words_width = output_text.map { |entry| entry[:word].to_s.length }.max + DISPLAY_ADJUST_NUMBER || 0


Suggested change

max_words_width = output_text.map { |entry| entry[:word].to_s.length }.max + DISPLAY_ADJUST_NUMBER || 0

max_words_width = output_text.map { |entry| entry[:word].to_s.length + DISPLAY_ADJUST_NUMBER }.max || 0

ここだけこうしないのはなぜ？と思いました

JunichiIto · 2024-07-13T07:44:58Z

05.wc/wc.rb

+end
+
+def get_max_width(output_text)
+  max_lines_width = output_text.map { |entry| entry[:line].to_s.length + DISPLAY_ADJUST_NUMBER }.max || 0


|| 0 っているんですか？（maxがnilになるケースってある？）

改めて考えると特に必要ないと思います。

JunichiIto · 2024-07-13T07:46:02Z

05.wc/wc.rb

+  max_lines_width = output_text.map { |entry| entry[:line].to_s.length + DISPLAY_ADJUST_NUMBER }.max || 0
+  max_words_width = output_text.map { |entry| entry[:word].to_s.length }.max + DISPLAY_ADJUST_NUMBER || 0
+  max_bytesizes_width = output_text.map { |entry| entry[:bytesize].to_s.length + DISPLAY_ADJUST_NUMBER }.max || 0
+  { line_display_width: max_lines_width, word_display_width: max_words_width, bytesize_display_width: max_bytesizes_width }


max_width[:line_display_width] みたいなキーだとちょっと冗長ですね。
max_width[:line] でも十分かなーと思います

JunichiIto · 2024-07-13T07:47:01Z

05.wc/wc.rb

+
+def print_output(output_text, max_width)
+  output_text.each do |output_line|
+    puts format_output_line(output_line, max_width)


format_output_line はメソッド化しなくてもいい気がしました
https://bootcamp.fjord.jp/pages/when-you-should-define-method

JunichiIto · 2024-07-13T21:59:06Z

05.wc/wc.rb

+
+require 'optparse'
+
+DDISPLAY_ADJUST_LENGTH = 1


Suggested change

DDISPLAY_ADJUST_LENGTH = 1

DISPLAY_ADJUST_LENGTH = 1

で良かったりしません？

あと、このプログラムには display, print, output という用語が出てきますが、それぞれ意図があって使い分けてますか？（僕からするとどれもほぼ同じ意味に思える）

JunichiIto · 2024-07-13T22:00:34Z

05.wc/wc.rb

+    wc_info[:bytesize] = input_text.size if options[:c]
+    output_texts << wc_info
+  end
+  output_texts << file_total_info(output_texts) if output_texts.size > MULTIPLE_DISPLAY


2以上なら複数（multiple）というのはある意味当たり前なので、MULTIPLE_DISPLAYという定数名はこれといった情報を提供していないことになります。
なので、定数をなくしても構わないかも？

Suggested change

output_texts << file_total_info(output_texts) if output_texts.size > MULTIPLE_DISPLAY

output_texts << file_total_info(output_texts) if output_texts.size > 1

JunichiIto · 2024-07-13T22:01:20Z

05.wc/wc.rb

+    wc_info[:line] = input_text.lines.count if options[:l]
+    wc_info[:word] = input_text.split(/\s+/).size if options[:w]
+    wc_info[:bytesize] = input_text.size if options[:c]
+    output_texts << wc_info


左辺と右辺の名前を見直してみましょう。
https://bootcamp.fjord.jp/pages/readable-variable-name

JunichiIto · 2024-07-13T22:01:48Z

05.wc/wc.rb

+    output_texts << wc_info
+  end
+  output_texts << file_total_info(output_texts) if output_texts.size > MULTIPLE_DISPLAY
+  max_width = get_max_widths(output_texts)


ここもですね
https://bootcamp.fjord.jp/pages/readable-variable-name

JunichiIto · 2024-07-13T22:02:53Z

05.wc/wc.rb

+  file_total_info
+end
+
+def print_output(output_text, max_width)


「printする」と「出力する（output）」はほぼ同じ意味なので、もう少し具体性のあるメソッド名を付けた方がいいかも

JunichiIto · 2024-07-13T22:08:05Z

05.wc/wc.rb

+end
+
+def get_max_widths(output_text)
+  max_lines_width = output_text.map { |entry| entry[:line].to_s.length + DDISPLAY_ADJUST_LENGTH }.max


+ DDISPLAY_ADJUST_LENGTH の処理は名前からして表示のタイミングで行う処理では？と思いました。
get_max_widthsというメソッド名なら純粋に最大の文字列長を取得する方が驚きが少ないと思います。

参考 https://blog.jnito.com/entry/2023/07/12/115733

JunichiIto · 2024-07-13T22:08:37Z

05.wc/wc.rb

+    opt.parse!(argv)
+  end
+
+  unless options.key?(:l) || options.key?(:w) || options.key?(:c)


Suggested change

unless options.key?(:l) || options.key?(:w) || options.key?(:c)

if options.empty?

でも同じかな？

JunichiIto · 2024-07-13T22:12:05Z

05.wc/wc.rb

+end
+
+def file_total_info(output_texts)
+  file_total_info = output_texts.inject({}) do |result, hash|


メソッド名と同じ変数名は避けましょう
https://qiita.com/jnchito/items/fb438aa0786bb3cbf919

あと、hashという名前も抽象的過ぎるのでもっと具体性のある名前にした方がいいと思います
https://bootcamp.fjord.jp/articles/78

JunichiIto · 2024-07-13T22:12:47Z

05.wc/wc.rb

+    file_name = ARGF.filename
+    wc_info[:filename] = file_name


Suggested change

file_name = ARGF.filename

wc_info[:filename] = file_name

wc_info[:filename] = ARGF.filename

変数に入れなくていいかも

JunichiIto · 2024-07-13T22:15:35Z

05.wc/wc.rb

+  line = output_line[:line]&.to_s&.rjust(max_width[:line_display_width])
+  word = output_line[:word]&.to_s&.rjust(max_width[:word_display_width])
+  bytesize = output_line[:bytesize]&.to_s&.rjust(max_width[:bytesize_display_width])
+  filename = output_line[:filename] == '-' ? '' : output_line[:filename]


なるほどー。
その情報が第三者にはぱっとわからないので（＝マジックナンバーならぬマジックストリング）、情報を付加した方がいいですね。

そして今回の提出物ではこの処理がなくなってますね。これはなぜ？ 🤔

JunichiIto · 2024-07-13T22:16:18Z

コメントしました！

kutimiti1234 · 2024-07-15T14:41:07Z

全体的に
https://bootcamp.fjord.jp/pages/avoid-too-specific-var-name
を参考に、wc_修飾子を省略しました。ロジックを見ればそれぞれの変数がwcに対する変数であることがわかると考えたためです。
(関数名についてのみ普遍的な操作ではなく、特殊な操作であることを強調したいと考えたためwc_修飾子を残しました。)

kutimiti1234 · 2024-07-15T14:44:39Z

05.wc/wc.rb

+  counts = []
+  ARGF.each(nil) do |standard_input_or_file|
+    count = {}
+
+    count[:filename] = ARGF.filename
+    count[:line] = standard_input_or_file.lines.count if options[:l]
+    count[:word] = standard_input_or_file.split(/\s+/).size if options[:w]
+    count[:bytesize] = standard_input_or_file.size if options[:c]
+    counts << count


ここではwcコマンドの実際に行っている処理として"count"が一番適当であると考えて、配列をcountsにして、それぞれの集計値をcountとして格納しました。
また"ARGF.each(nil) do |input_text|"のinput_textが抽象的すぎたため、具体的に入力として考えられる２つの要素「標準入力」「ファイル」を表す名前にしました。

うーん、standard_input_or_file は逆にちょっと具体的過ぎますね。
input_textの方がまだほどよい抽象度だったように思います。
https://bootcamp.fjord.jp/articles/78

kutimiti1234 · 2024-07-15T14:46:37Z

05.wc/wc.rb

+  rows = counts
+  # 複数行出力する場合は、集計行を表示する
+  rows << get_wc_total_row(counts) if counts.size > 1
+  max_column_widths = get_max_column_widths(rows)


countsに集計行を入れてしまうと、countsの実態から外れてしまうため、rowsに集約する形にしました

kutimiti1234 · 2024-07-15T14:50:00Z

05.wc/wc.rb

+def get_wc_total_row(counts)
+  total_row = counts.inject({}) do |result, column_name|
+    result.merge(column_name) { |_key, current_val, adding_value| current_val + adding_value }
+  end
+  total_row[:filename] = TOTAL
+  total_row
+end
+
+def show_wc_rows(rows, max_column_widths)
+  rows.each do |row|
+    cells = {}
+    row.each_pair do |column_name, cell|
+      cells[column_name] = if column_name == :filename
+                             # 標準入力を受け取る際に例外的に"-"が入力されるため除外する
+                             cell unless cell == STANDARD_INPUT_EXCEPTIONAL_WORD
+                           else
+                             cell.to_s.rjust(max_column_widths[column_name] + SHOW_ADJUST_SPACE_SIZE)
+                           end
+    end
+    puts "#{cells[:line]}#{cells[:word]}#{cells[:bytesize]} #{cells[:filename]}"
+  end
+end


最終出力を行う関数について、イメージとしてexcelの表を考えていたため、変数名にrow.column.cellを採用しました。

rowのhashにアクセスする際のkeyとしては、"column_name"で統一しました。count_nameと悩みましたが、filenameの列も存在するため、より包括的と考えられるcolumnを採用しました。

JunichiIto

コメントしました！

JunichiIto · 2024-07-15T23:04:41Z

05.wc/wc.rb

+  counts = []
+  ARGF.each(nil) do |standard_input_or_file|
+    count = {}
+
+    count[:filename] = ARGF.filename
+    count[:line] = standard_input_or_file.lines.count if options[:l]
+    count[:word] = standard_input_or_file.split(/\s+/).size if options[:w]
+    count[:bytesize] = standard_input_or_file.size if options[:c]
+    counts << count


うーん、standard_input_or_file は逆にちょっと具体的過ぎますね。
input_textの方がまだほどよい抽象度だったように思います。
https://bootcamp.fjord.jp/articles/78

JunichiIto · 2024-07-15T23:05:44Z

05.wc/wc.rb

+    count[:bytesize] = standard_input_or_file.size if options[:c]
+    counts << count
+  end
+  rows = counts


ある変数を別の変数に入れ替える、ということは滅多にやらないですね。
ここはcountsを使い続けるという形でも良いと思ったのですが、そうしなかった理由はありますか？

こちらについてですが、

rows << get_wc_total_row(counts) if counts.size > 1

の左辺と右辺で実態が異なるように考えたためrowsに書き換えました。

JunichiIto · 2024-07-15T23:06:12Z

05.wc/wc.rb

+    counts << count
+  end
+  rows = counts
+  # 複数行出力する場合は、集計行を表示する


このコメントはWHYを説明してないので不要ですね。（コードを見ればわかる内容になっている）
https://qiita.com/jnchito/items/f0d90af4ed44b7484103

JunichiIto · 2024-07-15T23:11:11Z

05.wc/wc.rb

+
+def get_wc_total_row(counts)
+  total_row = counts.inject({}) do |result, column_name|
+    result.merge(column_name) { |_key, current_val, adding_value| current_val + adding_value }


filenameも文字列連結されていくのはちょっとメモリの無駄遣いな気がするのでできたら避けたいですね。
（昨今のマシンスペックならメモリの消費もほとんど問題にならないとはいえ）

JunichiIto · 2024-07-15T23:13:11Z

05.wc/wc.rb

+def show_wc_rows(rows, max_column_widths)
+  rows.each do |row|
+    cells = {}
+    row.each_pair do |column_name, cell|


Suggested change

row.each_pair do |column_name, cell|

row.each do |column_name, cell|

each_pair を使う人って僕の周りでは見たことがないので、eachの方がいいと思います。
（僕もeach_pairってなんだっけ？と思って調べてしまった）

JunichiIto · 2024-07-15T23:15:26Z

05.wc/wc.rb

+    row.each_pair do |column_name, cell|
+      cells[column_name] = if column_name == :filename
+                             # 標準入力を受け取る際に例外的に"-"が入力されるため除外する
+                             cell unless cell == STANDARD_INPUT_EXCEPTIONAL_WORD


Suggested change

cell unless cell == STANDARD_INPUT_EXCEPTIONAL_WORD

cell unless cell == STDIN_FILENAME

標準入力は除外するんだなーというのが伝われば十分かも

JunichiIto · 2024-07-15T23:17:18Z

05.wc/wc.rb

+                             # 標準入力を受け取る際に例外的に"-"が入力されるため除外する
+                             cell unless cell == STANDARD_INPUT_EXCEPTIONAL_WORD
+                           else
+                             cell.to_s.rjust(max_column_widths[column_name] + SHOW_ADJUST_SPACE_SIZE)


Suggested change

cell.to_s.rjust(max_column_widths[column_name] + SHOW_ADJUST_SPACE_SIZE)

hoge = max_column_widths[column_name] + SHOW_ADJUST_SPACE_SIZE

cell.to_s.rjust(hoge)

変数に入れた方が読みやすそうです

JunichiIto · 2024-07-15T23:18:23Z

05.wc/wc.rb

+  end
+end
+
+def get_max_column_widths(wc_rows)


戻り値を返すメソッドは何でも get って名付けられますし、Rubyの場合は get_ や set_ で始まるメソッド名を避ける慣習があるのでget以外の動詞を付けたいですね

JunichiIto · 2024-07-15T23:19:15Z

05.wc/wc.rb

+end
+
+def get_max_column_widths(wc_rows)
+  max_lines_width = wc_rows.map { |entry| entry[:line].to_s.length  }.max


Suggested change

max_lines_width = wc_rows.map { |entry| entry[:line].to_s.length }.max

max_lines_width = wc_rows.map { |entry| entry[:line].to_s.length }.max

RuboCopに怒られませんでした？

こちらに関してですが、修正前のコードでrubocopを実行しても特にエラーは出ませんでした。
見た目として揃っていないので、次回の修正に含めたいと思います。

うーん、おかしいですね。僕が試したらエラーになりました。

wc.rb:55:60: C: [Correctable] Layout/ExtraSpacing: Unnecessary spacing detected. max = rows.map { |entry| entry[column_name].to_s.length }.max ^

わざとスペースを追加したコードと、それをrubocopにかけたときの結果をスクショで見せてもらえますか？

あと、以下のコマンドの実行結果も見たいです。

rubocop --show-cops Layout/ExtraSpacing

以下のようにEnabled: trueになっていればこのルールは有効化されているはずです。

$ rubocop --show-cops Layout/ExtraSpacing # Supports --autocorrect Layout/ExtraSpacing: Description: Do not use unnecessary spacing. Enabled: true VersionAdded: '0.49' AllowForAlignment: true AllowBeforeTrailingComments: false ForceEqualSignAlignment: false

JunichiIto · 2024-07-15T23:21:18Z

05.wc/wc.rb

+
+require 'optparse'
+
+SHOW_ADJUST_SPACE_SIZE = 1


Suggested change

SHOW_ADJUST_SPACE_SIZE = 1

SEPARATOR_SIZE = 1

これぐらいの名前でも十分かも？

JunichiIto · 2024-07-17T23:05:26Z

05.wc/wc.rb

+    count[:bytesize] = input_text.size if options[:c]
+    counts << count
+  end
+  counts << calculate_counts_total(counts) if counts.size > 1


Suggested change

counts << calculate_counts_total(counts) if counts.size > 1

counts << calculate_total_counts(counts) if counts.size > 1

こっちの方が英語として自然かも

JunichiIto · 2024-07-17T23:06:24Z

05.wc/wc.rb

+    counts << count
+  end
+  counts << calculate_counts_total(counts) if counts.size > 1
+  max_column_widths = calculate_wc_max_column_widths(counts)


Suggested change

max_column_widths = calculate_wc_max_column_widths(counts)

max_widths = calculate_max_widths(counts)

修飾語は省略できそうです

https://bootcamp.fjord.jp/pages/avoid-too-specific-var-name

JunichiIto · 2024-07-17T23:10:13Z

05.wc/wc.rb

+
+def main
+  options = parse_options(ARGV)
+  counts = []


左辺と右辺で実態が異なるように考えたためrowsに書き換えました

ここですが、テーブル構造（行と列で構成されるデータ）は例外的に左辺と右辺が異なってもいいと思います。
また、14行目のcountはcountsにした方がハッシュである感が出ると思います。
（countだと1や5みたいな数値が入ってそうに見える）

なので、最初からここはrowsにして、

rows << counts

みたいな形で書くのはOKです。

JunichiIto · 2024-07-17T23:11:18Z

05.wc/wc.rb

+  end
+  counts << calculate_counts_total(counts) if counts.size > 1
+  max_column_widths = calculate_wc_max_column_widths(counts)
+  show_wc_rows(counts, max_column_widths)


Suggested change

show_wc_rows(counts, max_column_widths)

show_rows(counts, max_column_widths)

ここもwcは省略できそう

JunichiIto · 2024-07-17T23:14:19Z

05.wc/wc.rb

+  counts_without_filename = counts.map { |count| count.except(:filename) }
+
+  total = counts_without_filename.inject({}) do |result, count|
+    result.merge(count) do |_key, current_val, adding_value|
+      current_val + adding_value
+    end
+  end


Suggested change

counts_without_filename = counts.map { |count| count.except(:filename) }

total = counts_without_filename.inject({}) do |result, count|

result.merge(count) do |_key, current_val, adding_value|

current_val + adding_value

end

end

total = counts.inject({}) do |result, count|

result.merge(count) do |key, current_val, adding_value|

current_val + adding_value unless key == :filename

end

end

こうした方が短くなります。

JunichiIto · 2024-07-17T23:23:32Z

05.wc/wc.rb

+                             column_width = max_column_widths[column_name] + SEPARATOR_SIZE
+                             cell.to_s.rjust(column_width)
+                           end
+    end


参考: eachの代わりにto_hメソッドを使う方法もあります。

cells = row.to_h do |column_name, cell| value = if column_name == :filename # ... [column_name, value] end

JunichiIto · 2024-07-17T23:26:34Z

05.wc/wc.rb

+                             cell.to_s.rjust(column_width)
+                           end
+    end
+    puts "#{cells[:line]}#{cells[:word]}#{cells[:bytesize]} #{cells[:filename]}"


44行目を

" #{cell}" unless cell == STDIN_FILENAME

にすると、ここはまとめてjoinできます。

Suggested change

puts "#{cells[:line]}#{cells[:word]}#{cells[:bytesize]} #{cells[:filename]}"

puts cells.values_at(:line, :word, :bytesize, :filename).join

JunichiIto · 2024-07-17T23:27:25Z

05.wc/wc.rb

+    count[:filename] = ARGF.filename
+    count[:line] = input_text.lines.count if options[:l]
+    count[:word] = input_text.split(/\s+/).size if options[:w]
+    count[:bytesize] = input_text.size if options[:c]


Suggested change

count[:bytesize] = input_text.size if options[:c]

count[:byte] = input_text.size if options[:c]

全部4文字にすると見た目にスッキリするかも（些細な改善ですが）

JunichiIto · 2024-07-17T23:28:14Z

05.wc/wc.rb

+def calculate_wc_max_column_widths(rows)
+  max_lines_width = rows.map { |entry| entry[:line].to_s.length }.max
+  max_words_width = rows.map { |entry| entry[:word].to_s.length }.max
+  max_bytesizes_width = rows.map { |entry| entry[:bytesize].to_s.length }.max


55-57行目はentryに渡している添え字が違うだけなのでDRYにできそうです

JunichiIto · 2024-07-17T23:34:15Z

05.wc/wc.rb

+end
+
+def get_max_column_widths(wc_rows)
+  max_lines_width = wc_rows.map { |entry| entry[:line].to_s.length  }.max


うーん、おかしいですね。僕が試したらエラーになりました。

wc.rb:55:60: C: [Correctable] Layout/ExtraSpacing: Unnecessary spacing detected. max = rows.map { |entry| entry[column_name].to_s.length }.max ^

わざとスペースを追加したコードと、それをrubocopにかけたときの結果をスクショで見せてもらえますか？

あと、以下のコマンドの実行結果も見たいです。

rubocop --show-cops Layout/ExtraSpacing

以下のようにEnabled: trueになっていればこのルールは有効化されているはずです。

$ rubocop --show-cops Layout/ExtraSpacing # Supports --autocorrect Layout/ExtraSpacing: Description: Do not use unnecessary spacing. Enabled: true VersionAdded: '0.49' AllowForAlignment: true AllowBeforeTrailingComments: false ForceEqualSignAlignment: false

kutimiti1234 · 2024-07-18T09:46:08Z

すみません。
こちら試したところきちんと作動しておりました。

提出前にRubocopを使用したつもりだったのですが、何か勘違いしていたようです。
お手数をおかけしてしまい申し訳ありません。

kutimiti1234 · 2024-07-18T12:44:31Z

    puts cells.values_at(:line, :word, :bytesize, :filename).join

で実装してしまうと、以下の通りとなってしまうので、

cell.to_s.rjust(column_width) + ' ' * SEPARATOR_SIZE

の形に修正しました。(下の画像は修正後)

kutimiti1234 · 2024-07-18T13:01:18Z

05.wc/wc.rb

+  keys = rows.map(&:keys).flatten.uniq
+  keys.each do |key|
+    max_widths[key] = rows.map { |entry| entry[key].to_s.length }.max unless key == :filename
+  end


keys = rows.map(&:keys).flatten.uniq

について、もっと驚きの少ない実装がないか以下の二通りの方法を考えましたが、要件を満たさなさそうだったため上記を採用しました。

rows.eachを使う方法

rowのキーを用いることができても、余分なループが生じてしまうこと

rows[0]などで最初のハッシュを取得してキーを取得する

突然最初のハッシュを取得して唐突に思えること。

実質的に上記の案と変わらない。

keys = rows.map(&:keys).flatten.uniq

についてkeysをnameとするか迷いましたが、左辺と右辺の一致を考えるとmap(&:keys)が右辺にあるのであれば、左辺にnameが来るのは不自然であると思いkeysを採用しました。

max_widths[key] = rows.map { |entry| entry[key].to_s.length }.max unless key == :filename

上記の左辺と右辺は、右側に{....}.maxが存在するためmax_widthで問題ないと判断しました。

JunichiIto

コメントしました〜。

JunichiIto · 2024-07-19T00:47:10Z

05.wc/wc.rb

+      columns[name] = if name == :filename
+                        value unless value == STDIN_FILENAME
+                      else
+                        column_width = max_column_widths[name]


Suggested change

column_width = max_column_widths[name]

width = max_widths[name]

ここも修飾語はなくしましょう

JunichiIto · 2024-07-19T00:48:53Z

05.wc/wc.rb

+                        value.to_s.rjust(column_width) + ' ' * SEPARATOR_SIZE
+                      end
+    end
+    puts columns.values_at(:line, :word, :byte, :filename).join


で実装してしまうと、以下の通りとなってしまうので

あ、すいません、前回のコード例がちょっと間違ってました。
意図してたコードはこうです。

Suggested change

puts columns.values_at(:line, :word, :byte, :filename).join

puts columns.values_at(:line, :word, :byte, :filename).join(' ')

SEPARATOR_SIZE はなくしちゃってもいいかもしれませんね

JunichiIto · 2024-07-19T00:52:01Z

05.wc/wc.rb

+  keys = rows.map(&:keys).flatten.uniq
+  keys.each do |key|


もっと驚きの少ない実装がないか以下の二通りの方法を考えましたが

ここはもっとシンプルに考えていいと思います。

Suggested change

keys = rows.map(&:keys).flatten.uniq

keys.each do |key|

%i[line word byte].each do |key|

こんな感じで十分かなと思ったのですが、どうでしょうか？

あと、eachの代わりにto_hを使うとこのメソッドの実装をよりシンプルにできそうです
https://docs.ruby-lang.org/ja/latest/method/Array/i/to_h.html

JunichiIto · 2024-07-19T00:57:21Z

05.wc/wc.rb

+  if options.empty?
+    options[:l] = true
+    options[:w] = true
+    options[:c] = true
+  end
+  options


Suggested change

if options.empty?

options[:l] = true

options[:w] = true

options[:c] = true

end

options

options.empty? ? { l: true, w: true, c: true } : options

こんな感じにするとよりスッキリするかも

JunichiIto · 2024-07-19T01:01:26Z

05.wc/wc.rb

+end
+
+def calculate_total_rows(rows)
+  total = rows.inject({}) do |result, count|


Suggested change

total = rows.inject({}) do |result, count|

total = rows.inject({}) do |result, counts|

14行目の命名に合わせるならcountsが良さそうです

JunichiIto · 2024-07-19T01:04:18Z

05.wc/wc.rb

+end
+
+def show_rows(rows, max_column_widths)
+  rows.each do |row|


Suggested change

rows.each do |row|

rows.each do |counts|

rowsの中身はcountsと呼ぶ、と一貫性を持たせると良いかもと思いました

JunichiIto

コメントしました！

JunichiIto · 2024-07-20T00:19:43Z

05.wc/wc.rb

+  rows = []
+  ARGF.each(nil) do |input_text|
+    counts = {}
+
+    counts[:filename] = ARGF.filename
+    counts[:line] = input_text.lines.count if options[:l]
+    counts[:word] = input_text.split(/\s+/).size if options[:w]
+    counts[:byte] = input_text.size if options[:c]
+    rows << counts
+  end


Suggested change

rows = []

ARGF.each(nil) do |input_text|

counts = {}

counts[:filename] = ARGF.filename

counts[:line] = input_text.lines.count if options[:l]

counts[:word] = input_text.split(/\s+/).size if options[:w]

counts[:byte] = input_text.size if options[:c]

rows << counts

end

rows = build_rows(options)

rowsを作る処理もメソッド化するとmainメソッドの見通しが良くなりそうです。

JunichiIto · 2024-07-20T00:25:08Z

05.wc/wc.rb

+    columns = {}
+    counts.each do |name, value|
+      columns[name] = if name == :filename
+                        value unless value == STDIN_FILENAME


正規表現で置き換えるのも一手です。

STDIN_PATTERN = /^-$/ value.sub(STDIN_PATTERN, '')

正規表現の見方については https://qiita.com/jnchito/items/893c887fbf19e17d3ff9 のその1とその3をチェックしてみてください

JunichiIto · 2024-07-20T00:29:25Z

05.wc/wc.rb

+    columns = {}
+    counts.each do |name, value|
+      columns[name] = if name == :filename
+                        value unless value == STDIN_FILENAME
+                      else
+                        width = max_widths[name]
+                        value.to_s.rjust(width)
+                      end
+    end


参考：ややトリッキーですが、to_hを使ってかなり行数を減らすこともできます。

Suggested change

columns = {}

counts.each do |name, value|

columns[name] = if name == :filename

value unless value == STDIN_FILENAME

else

width = max_widths[name]

value.to_s.rjust(width)

end

end

columns = counts.to_h do |name, value|

text = name == :filename ? value.sub(STDIN_PATTERN, '') : value.to_s.rjust(max_widths[name])

[name, text]

end

JunichiIto · 2024-07-20T00:33:54Z

05.wc/wc.rb

+  max_widths = calculate_max_widths(rows)
+  show_rows(rows, max_widths)


Suggested change

max_widths = calculate_max_widths(rows)

show_rows(rows, max_widths)

show_rows(rows)

rowsだけ渡してもらえればあとはいい感じに出力しておきまっせ、という設計の方がメソッドの再利用性が高そうです

JunichiIto

1点コメントしましたが大きな問題ではないのでこれでOKです！🙆‍♂️

JunichiIto · 2024-07-20T01:39:54Z

05.wc/wc.rb

+  rows = []
+  ARGF.each(nil) do |input_text|
+    counts = {}
+
+    counts[:filename] = ARGF.filename
+    counts[:line] = input_text.lines.count if options[:l]
+    counts[:word] = input_text.split(/\s+/).size if options[:w]
+    counts[:byte] = input_text.size if options[:c]
+    rows << counts
+  end


Suggested change

rows = []

ARGF.each(nil) do |input_text|

counts = {}

counts[:filename] = ARGF.filename

counts[:line] = input_text.lines.count if options[:l]

counts[:word] = input_text.split(/\s+/).size if options[:w]

counts[:byte] = input_text.size if options[:c]

rows << counts

end

rows = ARGF.each(nil).map do |input_text|

counts = { filename: ARGF.filename }

counts[:line] = input_text.lines.count if options[:l]

counts[:word] = input_text.split(/\s+/).size if options[:w]

counts[:byte] = input_text.size if options[:c]

counts

end

空の配列を用意してループで詰め込むだけの処理はmapで置き換えられます。（チェリー本参照）

あとfilenameは最初から入れてしまうのもアリです

kutimiti1234 added 10 commits June 30, 2024 16:39

wcコマンドの入力と出力を記述

1d98489

lオプション、wオプション、表示形式の実装

0245795

実態を表した変数名に変更。また、表示スペースをCONSTANTで調整)

c32b622

不要なファイルをコミットしたので削除

1881ba6

集計行を追加

5497ac7

表示の調整

a95e93b

-w -c コマンド使用時に不要な隙間が発生していたので修正。また、一部見え方を修正

6004c3f

集計行は複数引数のみ表示されるように修正。また一部関数の複雑さを軽減

0f17f1a

不要なファイルを削除

5853d2f

誤って作成したファイルを削除

31eca07

JunichiIto reviewed Jul 13, 2024

View reviewed changes

kutimiti1234 added 7 commits July 13, 2024 18:44

変数名の変更と不要な処理を削除

fd88d65

定数の宣言を移動

1180241

冗長なメソッドを削除

5ce6290

ロジックを単純なものに修正

0ef9086

不要な処理を削除

8ce47eb

変数名を変更

a90767c

変数名を修正

15c4248

JunichiIto reviewed Jul 13, 2024

View reviewed changes

kutimiti1234 added 10 commits July 14, 2024 09:41

不必要な定数を削除

7928ac5

削除した定数が誤っていたため、正しい定数に修正

07f858b

ARGF.filenameを直接格納

f88ef39

関数名,変数名をより説明的に変更

152e4be

get_max_column_widthsの機能が直感的にわかるように設定

c313029

2個ファイルを指定した際に集計業が出ないバグを修正

3f04331

冗長な条件分を簡潔に記述

3572c72

変数名を説明的に変更

8480673

標準入力の例外的なfilenameに対する処理

1d6ca7c

input_textが抽象的するギルため具体的に。また、関数名を修正

7900ee3

kutimiti1234 commented Jul 15, 2024

View reviewed changes

JunichiIto reviewed Jul 15, 2024

View reviewed changes

変数名と関数名をわかりやすく。ロジックも読みやすくなるように修正

955cd88

JunichiIto reviewed Jul 17, 2024

View reviewed changes

kutimiti1234 added 2 commits July 18, 2024 21:20

clean:変数名とメソッド名をよりわかりやすく

a1770ba

clean:calculate_max_witdhsメソッドをDRYに

40fdf93

clean:ブロック内の変数をレシーバに合わせる

0d3f3c1

kutimiti1234 commented Jul 18, 2024

View reviewed changes

JunichiIto reviewed Jul 19, 2024

View reviewed changes

kutimiti1234 added 5 commits July 19, 2024 12:09

clean:表示時のスペースの挿入をより分かりやすい形に変更

eee8b7e

clean:変数名をより一貫した形に変更。また、条件式をより見やすい形に変更。

2a3f36b

fix:オプション指定しない際に、-lwcが指定されるはずだが、バグが混入していたため修正

8ecdc26

clean:メソッドcalulate_max_widthsをより分かりやすい形に変更

96f3e9f

clean:rubocop

18b16c4

JunichiIto reviewed Jul 20, 2024

View reviewed changes

kutimiti1234 added 5 commits July 20, 2024 09:59

clean:build_rowsメソッドに既存のmainのロジックを切り出し、メインメソッドの見通しを向上

ceb6a3d

clean:正規表現に置き換えることで条件分岐を削除

a418071

clean:メソッドの再利用性を向上

758644c

clean:build_rowsのメソッドに集計行の追加処理を含むとまとまりがよくなると判断したため

3bc64b5

clean:メソッドの条件分岐を3こう演算子で見やすく。また、定数の名前を実態に合わせる

775cc20

JunichiIto approved these changes Jul 20, 2024

View reviewed changes

	max_words_width = output_text.map { \|entry\| entry[:word].to_s.length }.max + DISPLAY_ADJUST_NUMBER \|\| 0
	max_words_width = output_text.map { \|entry\| entry[:word].to_s.length + DISPLAY_ADJUST_NUMBER }.max \|\| 0

	output_texts << file_total_info(output_texts) if output_texts.size > MULTIPLE_DISPLAY
	output_texts << file_total_info(output_texts) if output_texts.size > 1

	unless options.key?(:l) \|\| options.key?(:w) \|\| options.key?(:c)
	if options.empty?

	file_name = ARGF.filename
	wc_info[:filename] = file_name
	wc_info[:filename] = ARGF.filename

	row.each_pair do \|column_name, cell\|
	row.each do \|column_name, cell\|

	cell unless cell == STANDARD_INPUT_EXCEPTIONAL_WORD
	cell unless cell == STDIN_FILENAME

	cell.to_s.rjust(max_column_widths[column_name] + SHOW_ADJUST_SPACE_SIZE)
	hoge = max_column_widths[column_name] + SHOW_ADJUST_SPACE_SIZE
	cell.to_s.rjust(hoge)

	max_lines_width = wc_rows.map { \|entry\| entry[:line].to_s.length }.max
	max_lines_width = wc_rows.map { \|entry\| entry[:line].to_s.length }.max

	counts << calculate_counts_total(counts) if counts.size > 1
	counts << calculate_total_counts(counts) if counts.size > 1

	max_column_widths = calculate_wc_max_column_widths(counts)
	max_widths = calculate_max_widths(counts)

	show_wc_rows(counts, max_column_widths)
	show_rows(counts, max_column_widths)

	puts "#{cells[:line]}#{cells[:word]}#{cells[:bytesize]} #{cells[:filename]}"
	puts cells.values_at(:line, :word, :bytesize, :filename).join

	count[:bytesize] = input_text.size if options[:c]
	count[:byte] = input_text.size if options[:c]

	column_width = max_column_widths[name]
	width = max_widths[name]

	puts columns.values_at(:line, :word, :byte, :filename).join
	puts columns.values_at(:line, :word, :byte, :filename).join(' ')

	keys = rows.map(&:keys).flatten.uniq
	keys.each do \|key\|
	%i[line word byte].each do \|key\|

	total = rows.inject({}) do \|result, count\|
	total = rows.inject({}) do \|result, counts\|

		max_widths = calculate_max_widths(rows)
		show_rows(rows, max_widths)

	max_widths = calculate_max_widths(rows)
	show_rows(rows, max_widths)
	show_rows(rows)

Conversation

kutimiti1234 commented Jul 7, 2024

Uh oh!

JunichiIto 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!

JunichiIto Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kutimiti1234 Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JunichiIto Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunichiIto commented Jul 13, 2024

Uh oh!

kutimiti1234 commented Jul 15, 2024

Uh oh!

kutimiti1234 Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kutimiti1234 Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JunichiIto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

JunichiIto Jul 13, 2024 •

edited

Loading

kutimiti1234 Jul 13, 2024 •

edited

Loading

JunichiIto Jul 13, 2024 •

edited

Loading

kutimiti1234 Jul 15, 2024 •

edited

Loading

kutimiti1234 Jul 15, 2024 •

edited

Loading

kutimiti1234 Jul 16, 2024 •

edited

Loading

kutimiti1234 Jul 16, 2024 •

edited

Loading