Rubocop を使ってみた
Rubocop というコードのスタイルをチェックするツールがあることを知りました。rubocop とすればディレクトリの Ruby ファイルについていろいろ教えてくれます。完璧ではないものの、rubocop --auto-correct とすれば自動で変更までしてくれます。
これを自分のコードでやってみました。
歓迎のこと
trailing whitespace を削除したり、改行コードを統一してくれるのは大歓迎です。あとクラスやメソッドが長すぎるのを指摘してくれるのも便利だと思います。
使ってなかった Ruby の機能に気付いた
Ruby でも C のように隣接する文字列リテラルが連結されるのですね。長い文字列を複数行に分けたいときに + を使っていたので、こちらのほうがスマートだなと思いました。
以下はバージョン情報メッセージを作っているところ。
comments = ["GTK+ #{Gtk::VERSION.join('.')}", - "Ruby/GTK #{Gtk::BINDING_VERSION.join('.')}" + + "Ruby/GTK #{Gtk::BINDING_VERSION.join('.')}" \ "(built for #{Gtk::BUILD_VERSION.join('.')})", "Ruby #{RUBY_VERSION} [#{RUBY_PLATFORM}]", "Nokogiri #{Nokogiri::VERSION}", "Mechanize #{Mechanize::VERSION}"].join("\n")
シングルクォートとダブルクォート
式展開をするのでなければ、シングルクォートで十分というやつですね。自分も require の後はシングルクォートなのですが、その他の場所では基本ダブルクォートを使うのでたくさん警告が出ました。
シングルクォートが良いとされる理由は、その文字列が「定数」であることを表わせるという点と、目にやさしいというところでしょうか。聞くところによるとシングルクォートのほうがちょっと速いらしいですが、真偽のところは知りません。
自分が基本ダブルクォートにしている理由は、式展開をしたくなった時に簡単に変更できるということが一点*1。もうひとつは、意味が C でも同じ*2だからということでしょうか。特に C 由来の関数への指示として文字列を使うときにシングルクォートだと違和感があります。
- renderer.text = sprintf("%2d:%02d", hour, min) + renderer.text = sprintf('%2d:%02d', hour, min)
- File.open("outlog.txt", "w") do |f| + File.open('outlog.txt', 'w') do |f|
nil との比較は冗長？
if の条件式のように暗黙的に true/false に変換されるような文脈でないと != nil は省けませんね。
- finished = @checking.group_by { |checker| (checker.state =~ /finished/) != nil } + finished = @checking.group_by { |checker| (checker.state =~ /finished/) }
finished[true] などとして後から参照しているのでこの場合は NG です。
冗長な self
- self.vbox.add @progress_bar + vbox.add @progress_bar
ダイアログの子ウィジェットなのですが、vbox というローカル変数を作ることはよくあって、その場合子ウィジェットの vbox を隠してしまうので、あえて冗長に self と書いて子ウィジェットだということを示している感じです。
- print "#{self.name}: failed to parse time #{@fields[15].inspect}\n" + print "#{name}: failed to parse time #{@fields[15].inspect}\n"
この例も name がローカル変数にありそうな名前なので self を書いたのだと思います。次の例は self と other が同じ構造を持つことを強調しています。
def ==(other) - self.channel_id == other.channel_id + channel_id == other.channel_id end
その他に、単なるリーダメソッドではなく、レシーバーがメソッド名である動詞の目的語になっているような場合は、self を書きたいです。以下は「自分に」メッセージを送る例。
def update message, *args if self.respond_to? message Gtk.queue do - self.__send__(message, *args) + __send__(message, *args) end end end
inspect のレシーバーを省略するのも同様に変な感じがします。
- raise TypeError, "#{self.inspect} is not of the expected type #{constraint}" + fail TypeError, "#{inspect} is not of the expected type #{constraint}"
正規表現
if @fields[15] =~ /^(\d+):(\d+)$/ - $1.to_i * 60 + $2.to_i + Regexp.last_match[1].to_i * 60 + Regexp.last_match[2].to_i
$1, $2 … はスクリプト言語に馴染みがない人には暗号的だからやめたほうがよいとも思うのですが、単純に Regexp.last_match[1] 等と置き換えると長くなって、かえって読み難くなってしまうようです。
この場合は hour = Regexp.last_match[1] などとして説明的変数を導入したほうが良いかもしれませんね*3。
演算子の周りのスペース
無用に式を詰めて書くのは読みにくいのですが、式が込み入ってくると内側の式を詰めて書いたほうが、意味を取りやすいと思います。
このくらいならどちらでもよいのですが、
- "%d時間%d分経過" % [minutes/60, minutes%60] + '%d時間%d分経過' % [minutes / 60, minutes % 60]
下の例は詰めたほうが引数の数がわかりやすいかと思います。
- table.attach_defaults(item, x, x+1, y, y+1) + table.attach_defaults(item, x, x + 1, y, y + 1)
きちんと覚えてない演算子の優先順位のメモであることもありますｗ
- MIN_POSITIVE_BIGNUM = 1 << 1.size*8-2 + MIN_POSITIVE_BIGNUM = 1 << 1.size * 8 - 2
かっこの内側のスペース
式が入り組むと息苦しいので内側にスペースを入れてやりたくなります。
rows = [ - [ get_framed_favicon_image(ch), cell(ch.name) ], - [ head("リスナー数/リレー数"), cell("#{ch.listener}/#{ch.relay}") ], - [ head("TIP"), cell("#{ch.tip}#{get_tip_info(ch.tip)}") ], - [ head("コンタクトURL"), cell(ch.contact_url) ], - [ head("ID"), cell(ch.id) ], - [ head("タイプ"), cell(ch.type) ], - [ head("詳細"), cell(ch.detail) ], - [ head("ジャンル"), cell(ch.genre) ], - [ head("コメント"), cell(ch.comment) ], + [get_framed_favicon_image(ch), cell(ch.name)], + [head('リスナー数/リレー数'), cell("#{ch.listener}/#{ch.relay}")], + [head('TIP'), cell("#{ch.tip}#{get_tip_info(ch.tip)}")], + [head('コンタクトURL'), cell(ch.contact_url)], + [head('ID'), cell(ch.id)], + [head('タイプ'), cell(ch.type)], + [head('詳細'), cell(ch.detail)], + [head('ジャンル'), cell(ch.genre)], + [head('コメント'), cell(ch.comment)], ]
複数行の {} は避けよ
……とはいうものの、かっこの内側で do end はどうでしょうか。do end と {} の択一は難しい問題ですね。
del_button = create(Button, '削除', width_request: 60, - on_clicked: proc { + on_clicked: proc do path, column = @treeview.cursor if path iter = @treeview.model.get_iter(path) @list.delete(iter[0]) @treeview.model.remove(iter) end - }) + end)
空のリテラル
Array.new 等よりもリテラルを書いたほうがよいのだそうです。自分はなぜか、破壊的に値を詰めこんでいく場合は new にしたがるようです。
- options = Hash.new
+ options = {}
- @chlist = Array.new + @chlist = []
if not よりも unless を使うべし
raise ~ unless はイディオムとして多用しているからいいのですが、
def interpolate(channel) @template.gsub(VAR_PATTERN) { |var| fn = VAR_DEFINITION[var] - raise "unknown variable #{var}" if not fn + fail "unknown variable #{var}" unless fn fn.call(channel).shellescape } end
次の unless の使い方には大いに違和感があります。
- if not result + unless result self.notification = "#{watcher} に接続できません。(#{watcher.error_reason})" end
自分は unless は単純に if not の意味ではなくて「[例外的状況]でもない限り[本来の動作]を行う」という意味に使いたいのですね。だから unless には else も使いません。
