プログラミング初心者がコードレビューで学んだこと【まだ初心者編】

はじめに

前回は「超初心者編」として、学習1~2ヶ月目にRubyのコードレビューで学んだことをまとめました。

今回は「まだ初心者編」として、lsコマンド作りなど、Rubyでやや複雑なプログラム作りに取り組んだ際のレビューから学んだことをまとめたいと思います。

学んだこと

ブロックを使うと短く書ける場合がある

こんなコードがあった時、皆さんはどのようにリファクタリングしますか？

words = ["あ", "いう", "えおか"]

words_size_sum = words.map do |word|
  word.size
end.sum

puts words_size_sum
=> 6

まず、do ~ endはこの場合、ブロックを使った方がわかりやすいです。

puts words.map { |word| word.size }.sum
=> 6

さらに、sumメソッドにはブロックを渡すことができるので、mapが不要になります。 Enumerable#sum (Ruby 3.2 リファレンスマニュアル)

puts words.sum { |word| word.size }
=> 6

&:を使うと、超短くできます✨

puts words.sum(&:size)
=> 6

もとは4行で書いていた処理が、たったこれだけになりました！
ブロックを渡せるメソッドはうまく活用して、簡潔なコードを書いていきたいです❣️

mainメソッドは、プログラムの概要がわかるように書く

トップレベルにmainメソッドを定義すると、上から下へ読むことができるようになり、コードが読みやすくなります。Rubyスクリプトにもmainメソッドを定義するといいかも、という話 - Qiita
このmainメソッドを、プログラム全体の流れがわかるように書けると、読み手にとって親切になりますね✨

例えば、こんなコードを書いていました。

def main
  if options[:l]
    print_file_details(files)
  else
    # 行数を計算する処理
    # 行数に合わせてファイルを整える処理
    # 整えたファイルを表示する処理
  end
end

main

これを少しだけ書き換えます。

def main
  if options[:l]
    print_file_details(files) 
  else
    print_files(files)
  end
end

def print_files(files)
  # 行数を計算する処理
  # 行数に合わせてファイルを整える処理
  # 整えたファイルを表示する処理
end

main

else以降をメソッドに切り出すと、上のifの処理と対照的になりました。「lオプションがある時はfile_detailsを表示し、ない時はfilesを表示する」という処理が、一気にわかりやすくなりましたね✨

mainメソッドの中は、読む人が全体の流れを把握しやすくなるよう、メソッドの切り出しなどを工夫して書きたいと思います。

正規表現は色々な状況を想定して、正確に書く

ファイルの中の単語数を数える処理を書いていました。 例えば、sample.rbが下のようになっていた場合、単語の数（3つ）を数える処理を書こうとしていました。

I love  ruby!

そこで私が書いたコードがこちらです。

file_data = File.read("sample.txt")
=> "I love  ruby!\n"
words_count = file_data.scan(/\s+/).size
=> 3

File.readメソッドを使うと、sample.txtは"I love ruby!\n"という文字列になります。

この文字列の単語数を数えるため、正規表現\s+を使っています。 \sは空白文字（ [ \t\r\n\f\v]）にマッチするメタ文字列で、+は1回以上の繰り返しを表します。正規表現 (Ruby 3.2 リファレンスマニュアル)

つまり「空白文字の一回以上の繰り返しの数を数える」ため、"I love ruby!\n"の場合、[" "], [" "], ["\n"]の3つにマッチして、無事単語数の3を出力できています。

しかし、このようなファイルがあった場合はどうでしょうか。

ruby

単語の数は1つなので、当然1が出力されて欲しいです。

file_data = File.read("sample.txt")
=> "\n\nruby\n"
words_count = file_data.scan(/\s+/).size
=> 2

しかし、実際には2が出力されてしまいました😱

このファイルに対して「空白文字の繰り返し」を抽出すると、rubyの前の改行も含む["\n\n"], ["\n"]にマッチしてしまい、単語数を一つ多く数えてしまいます。

これを解決するために、以下のように修正しました。

words_count = file_data.scan(/.(\s+)/).size
=> 1

\sの前に、任意の1文字（改行を除く）にマッチする.を入れ、「任意の１文字に続く、空白文字の繰り返し」を数えるようにしました。文字列の前の改行にはマッチせず、正しく単語数の1が出力できました。

正規表現を使用するときは、いろいろなパターンを想定して、正確に書かなければならないと学びました。特に空白や空行には、注意しようと思います。

例外処理を条件分岐のように使わない

ファイルのオーナー名を取得して、表示する処理を書いていました。

require 'etc'
file_status = File.stat('sample.rb') # ファイルsample.rbの情報を格納したオブジェクトを変数に入れる

def get_user_name(file_status)
  Etc.getpwuid(file_status.uid).name # ファイルのオーナーのユーザIDから、ユーザ名を取得して返す
rescue ArgumentError
  file_status.uid.to_s # ユーザIDからユーザー名が見つからなかった場合は、IDをそのまま返す
end

この処理では、「ユーザ名が取得できたらユーザ名、できなかったらユーザIDを返す」というのを、rescueを使って書いています。

このように例外を使って処理を制御するのは、可能な限り避けなくてはいけません。Railsアプリケーションにおけるエラー処理（例外処理）の考え方 - Qiita

この場合は、例外を補足する形ではなく、単純にifなどで分岐させるべきでした。

def get_user_name(file_status)
  user_info = Etc.getpwuid(file_status.uid)
  user_info.name ? user_info.name : file_status.uid.to_s
end

Railsのエラー処理でも、同じような指摘を受けたことがありました。
例外をキャッチして条件分岐のように使うのは、可能な限り避けようと思います。

外部環境に依存する処理は、極力独立させる

プログラム実行時に引数があった場合に、引数から得たファイル名を使って詳細を取得する処理を書いていました。

def main
  file_details = (ARGV.empty? ? create_details_from_stdin : create_details_from_arguments)
end

def create_details_from_arguments
  file_names = ARGV
  # file_namesからファイルの詳細情報を取得する処理
end

このコードの問題点は、ARGVに依存する場所が2箇所あることです。
ARGVとは、プログラムを実行した時に与えられた引数を保管する配列です。Object::ARGV (Ruby 3.2 リファレンスマニュアル)
この処理は、外部環境に依存していて不安定になりやすいので、影響範囲を限定させるために、できる限り独立させるのが望ましいです。

レビューを受けて、下のようなコードに修正しました。

def main
  file_names = ARGV
  file_details = (file_names.empty? ? create_details_from_stdin : create_details_from_arguments(file_names))
end

def create_details_from_arguments(file_names)
  # file_namesからファイルの詳細情報を取得する処理
end

外部環境に依存する処理を1箇所に閉じ込め、独立させることができました。 おまけに「ファイル名を取得元をARGVじゃなくて違うところにしたくなっちゃった！」みたいなことが起きた場合でも、mainメソッドだけの変更で済むようになったというメリットもあります✨

ARGVやネットワークアクセスなど、外部環境に依存する処理は、できる限り独立させたいと思います。

おまけ

force pushを避ける

レビューを受けている最中、バグがあるのに誤ってgit pushしてしまったため、慌ててgit push -fを使って取り消しました。

force pushを使うと、レビュアーが変更点が追いにくくなってしまうそうです。また、チームで開発している場合はメンバーがプッシュできなくなるなど、多大な支障が出る可能性もあります。

Matzもこのように言及していました。

最近だと rm -rf / よりも git push -fのほうが怖いかも

— Yukihiro Matz (@yukihiro_matz) November 23, 2021

危険だと知らず使ってしまいましたが、間違ってpushした場合は、過去を抹消せず、新しくコミットするべきでした。

まとめと感想

今回改めてレビューを見返して、やはり指摘事項は難しく、消化しきれていないことが結構あると感じました😖 ただ、メンターさんに丁寧に説明してもらったことは文字に残っているので、復習して以前より理解することができたと思います❣️

Railsの課題でもたくさんレビューをいただいているので、いつかまとめたいと思っています。