ククログ

株式会社クリアコード > ククログ > クリアなコードの作り方: 縦長・横長なメソッドを短くする

クリアなコードの作り方: 縦長・横長なメソッドを短くする

最近読んだRubyのコードではYARDのコードがキレイでした。

さて、長いメソッドは不吉なにおいがするからメソッドを分割するなどして短くしましょうとはよく言われることですが、ここでいう「長い」とは「縦に長い」ことを指していることがほとんどです。長いのが問題なのは縦に長いときだけではなく横に長いときもです。

縦に長いメソッド

まず、どうして縦に長いメソッドが問題かについてです。縦に長いメソッドには「処理を把握しづらい」という問題がある可能性が高いです。

どうして処理を把握しづらいか

処理を把握しづらい原因はいくつかあります。例えば、抽象度が低いのが原因です。

メソッドが縦に長くなっているときは、多くの処理が行われていることがほとんどです。これらの処理はメソッドになっていないため名前がついていません。処理に名前がついていない場合は実装を読まないとなにをしているかがわかりません。

せっかくなので実例を元にしてメソッドが長いのがどうして問題なのか、また、どのようによくすればよいかを説明します。例にするのはlogaling-commandのLogaling::Command::Application#lookupです。これを例に選んだ理由は、開発チームも整理したほうがよいコードだと認識しているコードだからです¹。

def lookup(source_term)
  config = load_config_and_merge_options
  repository.index
  terms = repository.lookup(source_term, config["source-language"], config["target-language"], config["glossary"])

  unless terms.empty?
    max_str_size = terms.map{|term| term[:source_term].size}.sort.last
    run_pager
    puts("[") if "json" == options["output"]
    terms.each_with_index do |term, i|
      target_string = "#{term[:target_term].bright}"
      target_string <<  "\t# #{term[:note]}" unless term[:note].empty?
      if repository.glossary_counts > 1
        target_string << "\t"
        glossary_name = "(#{term[:glossary_name]})"
        if term[:glossary_name] == config["glossary"]
          target_string << glossary_name.foreground(:white).background(:green)
        else
          target_string << glossary_name
        end
      end
      source_string = term[:snipped_source_term].map{|word|  word.is_a?(Hash) ? word[:keyword].bright : word }.join
      source, target = source_string, target_string.split("\t").first
      note = term[:note]
      source_language, target_language = config["source-language"], config["target-language"]
      case options["output"]
      when "terminal"
        printf("  %-#{max_str_size+10}s %s\n", source_string, target_string)
      when "csv"
        print(CSV.generate {|csv| csv << [source_string, target, note, source_language, target_language]})
      when "json"
        puts(",") if i > 0
        record = {
          :source => source_string, :target => target,
          :note => note,
          :source_language => source_language, :target_language => target_language
        }
        print JSON.pretty_generate(record)
      end
    end
    puts("\n]") if "json" == options["output"]
  else
    "source-term <#{source_term}> not found"
  end
rescue Logaling::CommandFailed, Logaling::TermError => e
  say e.message
end

今回のサンプルの中には以下の一行があります。

terms = repository.lookup(source_term, config["source-language"], config["target-language"], config["glossary"])

このコードからは「ソース（翻訳元）の単語（source_term）とソースの言語（config["source-language"]）とターゲット（翻訳先）の言語（config["target-language"]）と用語集（config["glossary"]）を使ってリポジトリ（repository）から単語（terms）を検索している（lookup）」ことがわかります。しかし、具体的にどのように検索しているかはわかりません。しかしそれでよいのです。コードを読むときは「ここで検索して単語を取得できた」という前提で読み進めます。こうすることで考えることを少なくできて、問題に集中できます。これが抽象化されているということです。

一方、以下の部分は何をしているかを知るために具体的に何をしているかを読まないといけません。

unless terms.empty?
  max_str_size = terms.map{|term| term[:source_term].size}.sort.last
  run_pager
  puts("[") if "json" == options["output"]
  terms.each_with_index do |term, i|
    target_string = "#{term[:target_term].bright}"
    target_string <<  "\t# #{term[:note]}" unless term[:note].empty?
    if repository.glossary_counts > 1
      target_string << "\t"
      glossary_name = "(#{term[:glossary_name]})"
      if term[:glossary_name] == config["glossary"]
        target_string << glossary_name.foreground(:white).background(:green)
      else
        target_string << glossary_name
      end
    end
    source_string = term[:snipped_source_term].map{|word|  word.is_a?(Hash) ? word[:keyword].bright : word }.join
    source, target = source_string, target_string.split("\t").first
    note = term[:note]
    source_language, target_language = config["source-language"], config["target-language"]
    case options["output"]
    when "terminal"
      printf("  %-#{max_str_size+10}s %s\n", source_string, target_string)
    when "csv"
      print(CSV.generate {|csv| csv << [source_string, target, note, source_language, target_language]})
    when "json"
      puts(",") if i > 0
      record = {
        :source => source_string, :target => target,
        :note => note,
        :source_language => source_language, :target_language => target_language
      }
      print JSON.pretty_generate(record)
    end
  end
  puts("\n]") if "json" == options["output"]
else
  "source-term <#{source_term}> not found"
end

これが抽象度が低いということです。抽象度が低い部分は細かく実装を読まないといけないため抽象度が高い部分に比べて処理を把握しづらくなります。なお、この部分は見つけた単語を整形して表示している部分です。

キレイにする方法

縦に長い場合はメソッドを分割します。これはよく言われていることですね。

今回のサンプルでは以下のようになります。

def lookup(source_term)
  config = load_config_and_merge_options
  repository.index
  terms = repository.lookup(source_term, config["source-language"], config["target-language"], config["glossary"])

  if terms.empty?
    "source-term <#{source_term}> not found"
  else
    report_terms(terms, config)
  end
rescue Logaling::CommandFailed, Logaling::TermError => e
  say e.message
end

これならLogaling::Command::Application#lookupが何をしているのかはすぐにわかりますね。「単語を検索して見つかった単語を出力」しています。

report_termsはlookupにあったコードそのままです。そのままですが、「このメソッドは単語を出力するメソッド」と思って読むと、そうと知らずに読むときよりも理解しやすくなります。

private
def report_terms(terms, config)
  max_str_size = terms.map{|term| term[:source_term].size}.sort.last
  run_pager
  puts("[") if "json" == options["output"]
  terms.each_with_index do |term, i|
    target_string = "#{term[:target_term].bright}"
    target_string <<  "\t# #{term[:note]}" unless term[:note].empty?
    if repository.glossary_counts > 1
      target_string << "\t"
      glossary_name = "(#{term[:glossary_name]})"
      if term[:glossary_name] == config["glossary"]
        target_string << glossary_name.foreground(:white).background(:green)
      else
        target_string << glossary_name
      end
    end
    source_string = term[:snipped_source_term].map{|word|  word.is_a?(Hash) ? word[:keyword].bright : word }.join
    source, target = source_string, target_string.split("\t").first
    note = term[:note]
    source_language, target_language = config["source-language"], config["target-language"]
    case options["output"]
    when "terminal"
      printf("  %-#{max_str_size+10}s %s\n", source_string, target_string)
    when "csv"
      print(CSV.generate {|csv| csv << [source_string, target, note, source_language, target_language]})
    when "json"
      puts(",") if i > 0
      record = {
        :source => source_string, :target => target,
        :note => note,
        :source_language => source_language, :target_language => target_language
      }
      print JSON.pretty_generate(record)
    end
  end
  puts("\n]") if "json" == options["output"]
end

なお、report_terms内ではターミナル出力・CSV出力・JSON出力の3種類の出力するためのコードが入っています。そのため、report_termsをさらに短くする場合はそこに注目してメソッドを分割することになります。

横に長いメソッド

それでは、次に、どうして横に長いメソッドが問題かについてです。横に長いメソッドには「遠くのオブジェクトにさわっている」という問題がある可能性が高いです。

どうして遠くのオブジェクトにさわるのが問題か

まず、遠くのオブジェクトにさわるということを説明します。

プログラムは「ここではこういう状態でプログラムが動く」という前提を踏まえながら書きます。例えば、メソッドの中で「このインスタンス変数」といえば「自分のインスタンス変数」という前提で書きます。

class Person
  attr_reader :first_name, :last_name
  def initialize(first_name, last_name)
    @first_name = first_name
    @last_name = last_name
  end

  def full_name
    "#{@first_name} #{@last_name}"
  end
end

このような前提がコンテキストです。同じコンテキストでは短い記述で書くことができ、違うコンテキストでは記述が長くなります。

alice = Person.new("Alice", "Liddell")
puts "#{alice.first_name} #{alice.last_name}"

メソッドの中では@first_nameと@last_nameでアクセスできたものがトップレベルではalice.first_nameとalice.last_nameでアクセスすることになります。これはコンテキストが異なるため、単に「インスタンス変数」ということができずに「aliceのインスタンス変数」という必要があるためです。

遠くのオブジェクトというのは離れたコンテキストにあるオブジェクトのことです。遠くのオブジェクトにアクセスするには以下のようにコンテキストをたどっていく必要があります。

bookstore.fairy_stories.find {|story| story.title == "Alice's Adventures in Wonderland"}.characters.find {|character| character.first_name == "Alice"}.full_name

コンテキストをたどっていくとコードが横に長くなります。

それでは、どうして遠くのオブジェクトにさわるのが問題なのでしょうか。それは、抽象度が低くなるからです。また抽象度です。縦に長いメソッドのところでも触れましたが、抽象度が低いと多くのことを把握しないといけなくなります。しかし、多くのことを把握することは大変です。大きなソフトウェアや久しぶりにさわるソフトウェアでは特に大変です。そのため、できるだけ必要なことだけを把握した状態でプログラムを書けるようにしたいのです。遠くのオブジェクトにさわるコードではそれが難しくなることが問題です。

サンプル

logaling-commandや近くにあったtDiaryを見てみましたが、あまりこのケースに該当するコードはありませんでした。しかし、無理やり引っ張りだしてきたのが以下のコードです。これは用語を検索するために用語集のインデックスを更新するLogaling::Repository#indexというメソッドです。

def index
  project_glossaries = Dir[File.join(@path, "projects", "*")].map do |project|
    Dir.glob(get_all_glossary_sources(File.join(project, "glossary")))
  end
  imported_glossaries = Dir.glob(get_all_glossary_sources(cache_path))
  all_glossaries = project_glossaries.flatten + imported_glossaries

  Logaling::GlossaryDB.open(logaling_db_home, "utf8") do |db|
    db.recreate_table
    all_glossaries.each do |glossary_source|
      indexed_at = File.mtime(glossary_source)
      unless db.glossary_source_exist?(glossary_source, indexed_at)
        glossary_name, source_language, target_language = get_glossary(glossary_source)
        puts "now index #{glossary_name}..."
        db.index_glossary(Glossary.load(glossary_source), glossary_name, glossary_source, source_language, target_language, indexed_at)
      end
    end
    (db.get_all_glossary_source - all_glossaries).each do |glossary_source|
      glossary_name, source_language, target_language = get_glossary(glossary_source)
      puts "now deindex #{glossary_name}..."
      db.deindex_glossary(glossary_name, glossary_source)
    end
  end
end

気になるのはこのあたりです。

all_glossaries.each do |glossary_source|
  indexed_at = File.mtime(glossary_source)
  unless db.glossary_source_exist?(glossary_source, indexed_at)
    glossary_name, source_language, target_language = get_glossary(glossary_source)
    puts "now index #{glossary_name}..."
    db.index_glossary(Glossary.load(glossary_source), glossary_name, glossary_source, source_language, target_language, indexed_at)
  end
end

glossary_nameやsource_languageなどをバラバラにdb.index_glossaryに渡さないでGlossaryオブジェクトを渡すというのはどうでしょうか。

all_glossaries.each do |glossary_source|
  Glossary.open(glossary_source) do |glossary|
    unless db.glossary_source_exist?(glossary)
      puts "now index #{glossary.name}..."
      db.index_glossary(glossary)
    end
  end
end

だいぶすっきりしました。これでこのメソッドはGlossaryがどのような情報を持っているかの詳細を知らずに済みます。単に「用語集として必要な情報を持っているはず」ということだけ把握していればよいのです。

まとめ

メソッドの長さは視覚的にわかるので、パッと見てキレイなコードかそうでないかをざっくりと判断しやすくて便利です。あなたのコードはパッと見てキレイですか？

すでに修正済みです。 ↩