`return "エラーメッセージ"` はやめてくれ

6456 ワード
アンチパターン アンチパターン テキストリンク

戻り値としてエラーメッセージを返す関数・メソッドはやめてください。

例えば、こんなファイルの内容をチェックする関数です。

# ファイルの内容が正しいかチェックする関数
def check_file_format(path)
  return 'ファイルが存在しません' unless File.exist?(path)
  contents = File.read(path)
  return 'ファイルが長すぎます' if contents.lines.size > 500
  return 'シバンを書いてください' if contents.lines.first.starts_with('#!')
  return '行末に空白があります' if contents.lines.any? { |l| l.ends_with?(' \n') }
  # 以下チェックが続く
end

# 関数を使う側（WEB版）
class FileController < ApplicationController
  def check
    @error_message = check_file_format(params[:path])
  end
end

# 関数を使う側（CLI版）
def main
  error_message = check_file_format(params[:path])
  puts(error_message)
end

こういった関数は、以下のような要望が出てきた時点で破綻します。

「ファイルが長すぎます」と指摘されて直した後、「行末に空白があります」と指摘されるのは面倒です。初めから両方を出すことはできませんか？

戻り値を「エラーメッセージ」から、「エラーメッセージのリスト」に変えればいいって？ ええ、あなたがこの関数を呼び出している、２００か所を全部直してくれるなら。

どうするべきだったのか？

エラーメッセージは、生文字列ではなく、クラスでラップして返しましょう。
クラスになっていれば、コンストラクタの引数を増やしたり、メソッドを足したりして、将来の変更に耐えることができます。

class Result
  def initialize(message)
    @nessage = message
  end

  def message
    @message
  end
end

def check_file_format(path)
  return Result.new('ファイルが存在しません') unless File.exist?(path)
  contents = File.read(path)
  return Result.new('ファイルが長すぎます') if contents.lines.size > 500
  return Result.new('シバンを書いてください') if contents.lines.first.starts_with('#!')
  return Result.new('行末に空白があります') if contents.lines.any? { |l| l.ends_with?(' \n') }
  # 以下チェックが続く
end