最近、以下のようなコードを何度か見ました¹。

FileUtils.mkdir_p assets_path unless FileTest.exist? assets_path

このコードを元に、「余計なコードを書かない」ことがどうして大事かを説明します。

余計なコード

まずは、どこが余計なコードなのかを考えてみましょう。このコードではFileUtils.mkdir_pとFileTest.exist?メソッドを使っています。

FileUtils.mkdir_pは引数で指定されたディレクトリがなかったら親ディレクトリも含めて作成するメソッドです。すでにディレクトリが存在した場合は何もしませんし、エラーにもなりません。mkdir_pというメソッド名はmkdir -pコマンドが由来でしょう。

FileTest.exist?は引数で指定されたファイルが存在したら真を返すメソッドです。

このコードではunless FileTest.exist?のときだけFileUtils.mkdir_pをしていますが、FileUtils.mkdir_pはすでにないときだけディレクトリを作るという動作なのでunless FileTest.exist?というチェックは必要ありません。つまり、このunless FileTest.exist?が余計なコードだということです。

余計なコードを読むときに考えること

それでは、余計なコード入りのコードがどうしてよくないかを説明します。

余計なコードが入っているとコードを読むときに「この一見余計なコードには実はなにか深い意味があるんじゃないか？」と勘ぐってしまいます。そうするとコードをすーっと読めずにつかえてしまいます。読むときにたくさんつかえてしまうコードは理解しづらいためよくありません。

今回の例ではどのように勘ぐってしまうかを説明します。実際は一気にまとめて考えますが、わかりやすくするために順を追って1つずつ説明します。

まず、以下の部分で「この場所で確実にassets_pathを準備しておくんだな。この後のコードではassets_pathが必ず存在すると仮定して大丈夫だな。」と読みます。シェルスクリプトでもmkdir -pがあったら「これ以降の処理ではこのディレクトリは必ず存在するんだな。」と読むのと一緒ですね。

FileUtils.mkdir_p assets_path

続いてFileUtils.mkdir_pが特定のときだけ実行されることに気づきます。ここで違和感を覚えます。「あれ？FileUtils.mkdir_pはすでにディレクトリがあってもエラーにしないからいつも実行しても問題ないはずだけど…あ、ディレクトリが存在するときはエラーにならないけどパーミッションがなくてディレクトリを作れないときはエラーになるからそれを防ごうとしているのかな？」

FileUtils.mkdir_p assets_path unless # ...

しかし、FileTest.exist?でファイル（ディレクトリを含む）が存在するかどうかをチェックしています。ここから深読みが始まります。「ディレクトリがすでに存在するかどうかはFileUtils.mkdir_pでチェックしているからこのFileTest.exist?にはなにか別の意図があるのではないか。もしかしたら、assets_pathはディレクトリではなくてファイルでもよいのではないか。あるいは、ここは実行効率が気になる場面でFileUtils.mkdir_pよりもFileText.exist?の方が実行効率がよいのではないか。」

FileUtils.mkdir_p assets_path unless FileTest.exist? assets_path

気になる点が2つでてきました。

1. assets_pathはディレクトリではなくてファイルでもよいのではないか。

2. ここは実行効率が求められるのではないか。

確認してみましょう²。

余計なコードかどうかの確認

まず、ファイルでも問題ないかを確認します。FileUtils.mkdir_pは「これ以降はこのディレクトリは確実に存在するよ！」という意図を持ったコードなので、これ以降の処理を確認します。するとこんな感じになっています。

FileUtils.mkdir_p assets_path unless FileTest.exist? assets_path

FileList['{js,theme}/*'].each do |file|
	FileUtils.cp_r(file, "#{assets_path}/#{Pathname.new(file).basename}")
end

"#{assets_path}/#{Pathname.new(file).basename}"としているのでassets_pathがファイルの場合はエラーになります。つまり、unless FileTest.exist?があることで問題の発見が遅れてしまっています。もし、assets_pathが存在する場合はFileUtils.mkdir_pはエラーになるからです。多くの場合、問題の原因を早めに報告するコードの方が問題を直しやすいのでよいコードです³。自分でfree()を呼んだときにクラッシュする問題と、GCでたまにクラッシュする問題は前者の方が直しやすい⁴です。

よって、unless FileTest.exist?はassets_pathがファイルでもディレクトリでもよいから、という理由ではなさそうです。

それでは、実行効率が求められるからでしょうか。まず、FileUtils.mkdir_pよりもFileTest.exist?の方が実行効率がよいことを確認します。もし、FileTest.exist?の方が遅ければunless FileTest.exist?を書かないほうが実行効率がよくなります。実行時間を測る場合はbenchmarkが便利です。

require "benchmark"
require "fileutils"

assets_path = "/tmp/asserts"
FileUtils.mkdir_p(assets_path)

Benchmark.bmbm do |benchmark|
  benchmark.report("FileUtils.mkdir_p") do
    100.times do
      FileUtils.mkdir_p(assets_path)
    end
  end

  benchmark.report("FileTest.exist?") do
    100.times do
      FileTest.exist?(assets_path)
    end
  end
end

実行してみましょう。

% ruby1.9.1 -v /tmp/bench_fileutils.rb
ruby 1.9.3p0 (2011-10-30 revision 33570) [x86_64-linux]
Rehearsal -----------------------------------------------------
FileUtils.mkdir_p   0.000000   0.000000   0.000000 (  0.002940)
FileTest.exist?     0.000000   0.000000   0.000000 (  0.000104)
-------------------------------------------- total: 0.000000sec

                        user     system      total        real
FileUtils.mkdir_p   0.000000   0.000000   0.000000 (  0.002047)
FileTest.exist?     0.000000   0.000000   0.000000 (  0.000105)

たしかにFileTest.exist?の方が速いので、ディレクトリがすでに存在するケースがほとんどと考えるならunless FileTest.exist?があった方が速そうです。もちろん、ディレクトリが存在しない場合はunless FileTest.exist?がない方が⁵速いでしょう。

それでは、この処理が実行効率を求められているかを確認しましょう。ベンチマークの結果、1回あたり0.00002秒ほど違いがでるようです。数回しか実行しない処理ならあまり気にならないでしょう。1万回くらい実行するなら気になりそうです。このコードがどのくらい実行されるかを確認すれば実行効率を求められているかどうかがわかりそうです。このコードはRakeのタスクの中にあります。

namespace :assets do
	task :copy do
		require 'fileutils'
		assets_path = File.dirname(__FILE__) + '/public/assets'
		FileUtils.mkdir_p assets_path unless FileTest.exist? assets_path

		FileList['{js,theme}/*'].each do |file|
			FileUtils.cp_r(file, "#{assets_path}/#{Pathname.new(file).basename}")
		end
	end
end

Rakeのタスクは1回しか実行されないので、ここで実行効率を考える必要はなさそうです。よって、実行効率を求めてunless FileTest.exist?を使ったわけではないのでしょう。

以上から、コードから深読みした範囲ではコードを書いた人の意図はわかりませんでした。そのため、このコードからはわからない何か別の意図があるか、このコードに単に余計なコードが入っているだけではないかと考えます。

もし、本当にunless FileTest.exist?が余計なコードだったとしたら、このコードがない方が上記のことを考えないで済むため、読む人にやさしいよいコードと言えます。

まとめ

最近見たコードを例にして、余計なコードがある場合は読む人が大変ということを説明しました。自分がコードを書くときは余計なコードを入れず、どういう意図でこのコードを書いたかが読めばすぐにわかるコードを書きましょう。

余談ですが、もし、実行効率を求めている場合は以下のようにそれがわかる名前のメソッドを定義してそれを使うのがよいでしょう。そうすれば、読む人は「実行効率を求めてこういうコードにしているんだな。」ということがすぐにわかります⁶。

def fast_mkdir_p(directory)
	FileUtils.mkdir_p(directory) unless FileTest.exist?(directory)
end

fast_mkdir_p(assets_path)

もうひとつ余談をすると、Rakefileの中ではFileUtils.mkdir_pと書くよりも以下のようにmkdir_pだけ書くほうが好ましいです。

task :default do
	assets_path = File.dirname(__FILE__) + '/public/assets'
	mkdir_p(assets_path)
end

このように書くと、実行したときに以下のようにどのような操作を実行したかを表示してくれます。これはmkdir_pに限らずcpなどFileUtilsが提供しているコマンド全部で有効です。

% rake
mkdir -p /tmp/public/assets

ツイートする

フォローする