あなたの Ruby コードを添削します 【第 5 回】 Miyako + MidoreDayBook + Lispもどき

はじめに

秋です。締め切りを守れない季節です。

この冒頭の一言も、五回目ともなるとさすがにネタがなくなってきます。 やむをえず第一回の文句を変形してみたのですが、キレに欠ける面は否定できません。 そもそもわたしはなんでこんな謎の文章を毎回入れているのでしょうか。 第一回を書いたとき睡眠不足のうえ酒を飲んでいたのがまずかったのかもしれません。

今回のお題

これまで毎回一つのプログラムを添削してきたこの連載ですが、 今回は趣向を変えて、三つのプログラムを一気に添削してみることにしました。

趣向を変えた理由は二つあります。

まず、日記でプログラムを募集したら三つ来たこと。 せっかく応募が来たんだから全部使わなければ損です。

次に、これまでの添削記事が読み切れないという声が多かったこと。 既存の添削はどれも一つ一つが長く、 しかも全体で話がつながっているため、全部読み切るのが大変でした。 そこで今回は別々のプログラム三つを添削して、 それぞれ独立して読めるようにしてみたわけです。

ソースコード

では、添削対象のソースコードを示します。 ファイルが複数ある場合は、主なファイルだけをピックアップしました。 CodeReview-0017.tar.gz には全ファイルを含めてあるので、 詳しくはそちらをダウンロードして眺めてください。

ちなみに、今回は添削後のソースコードがありません。 Miyako の 5000 行を筆頭として今回はとにかく分量が多いので、 改善ポイントを指摘するに留めます。

ゲーム作成フレームワーク Miyako

最初のプログラムはゲームやプレゼンテーションのフレームワーク、「Miyako」です。 このプログラムはサイロス誠さんに提供していただきました。 ライセンスは LGPL 2.1 です。

主なファイルは以下の二つです。

ファイル構成

まず、ファイル構成について話しましょう。 Miyako は miyako.rb と miyako_ext.rb の二つのファイルで構成されています。 サイズはこんなもんです。

$ wc miyako.rb miyako_ext.rb
  5137  13190 133706 miyako.rb
  1177  2982   29260 miyako_ext.rb

とりあえず、いくらなんでも miyako.rb がデカすぎます。 中を見ると Yuki と Miyako の二つのモジュールが入っているので、 せめて yuki.rb と miyako.rb くらいには分けたいところです。

ファイルが増えるとインストールは多少面倒になりますが、 すでに miyako.rb と miyako_ext.rb でファイルが二つあるのですから、 ディレクトリが一つ増えるくらいは我慢してもらえるのではないでしょうか。

あるいは、miyako_ext.rb を miyako/extension.rb に変更してもいいかもしれません。 それならインストール対象はファイル miyako.rb とディレクトリ miyako/ で、 今と同じ 2 つのままで済みます。

「Ruby の protected は Java や C++ の protected ではない」

まず、例によって protected について指摘します。

$ grep protected miyako.rb
    protected :setup
    protected :add, :compile
    protected :process_default
    protected :createList
    protected :init_height
    protected :_drawText, :_drawTextMild
    protected :setup
    protected :layout_left_inside, :layout_right_inside, :layout_center_inside
    protected :layout_top_inside, :layout_middle_inside, :layout_bottom_inside
    protected :layout_left_outside, :layout_right_outside, :layout_center_outside
    protected :layout_top_outside, :layout_middle_outside, :layout_bottom_outside
    protected :get_base_width, :get_base_height
    protected :get_base_x, :get_base_y
    protected :createWindow, :setMargin
    protected :setPauseLocate
    protected :createWindow, :setMargin
      protected :copyRect, :draw, :cdraw
$ grep private miyako.rb
$

protected と指定されているメソッドはどれも外から呼ばれていないようですし、 さらに private がまったく使われていません。 となると、たぶんこれはすべて private であるべきメソッドだと考えられます。

Ruby の protected は Java や C++ の protected とは働きが違い、 むしろ friend と同じような目的で使われます。 Ruby で Java, C++ の protected に相当するのは private です。 以下に Ruby の public, protected, private の効果をまとめておくので、 再確認してください。

可視性指定 効果
public 制限なく呼び出せる
protected 同じクラスまたはサブクラスのインスタンスからのみ呼び出せる
private 同じインスタンスからのみ呼び出せる

それから、Ruby のメソッド可視性とは、 「違うインスタンスからメソッドを呼び出せるかどうか」を制御する属性です。 同じインスタンスの、 違うクラスで定義されたメソッドからの呼び出しは制御できません。 つまり、C++ の private に相当する機能はありません。 それは以下の例を見るとわかります。

# スーパークラスで定義された private メソッドを呼び出す例
class A
  def print_ok
    puts "OK"
  end
  private :print_ok
end

class B < A
  def m
    print_ok   # スーパークラスで定義された private メソッドを呼ぶ
  end
end

B.new.m    # エラーにならない!
# include したモジュールで定義されている private メソッドを呼び出す例
module M
  def print_ok
    puts "OK"
  end
  private :print_ok
end

class C
  include M

  def m
    print_ok   # include したモジュールで定義されている private メソッドを呼ぶ
  end
end

C.new.m   # やっぱりエラーにならない

Ruby で protected を使うべき場面とは、例えば、 「==」を実装するために非公開のインスタンス変数にアクセスしたい場合です。

 class Person
   def initialize(name, age)
     @name = name     # この変数は公開する
     @age = age       # この変数は公開したくない
   end

   # ……しかし、Person#name も Person#age も別のオブジェクトからアクセスしたい
   def ==(other)
     @name == other.name and    # ← Person#name を外から呼んでる
     @age == other.age          # ← Person#age を外から呼んでる
   end

   attr_reader :name  # Person#name は public で問題なし
   attr_reader :age   # でも Person#age は無制限に公開したくない
   protected :age     # ……ので、protected にする
 end

Ruby で protected が適切な場面はたいして多くありません。 自分のプログラムで protected を使った覚えのある人は用途を確認してみてください。

これまでにも (Ruby の) private のつもりで protected を使うミスが 何度か登場していたことを考えると、 この点は相当に強調しておく価値がありそうです。 この項のキャプションにも明示しておきましたので、絶対に忘れないでください。

条件文

次の行を見てください。

        while f.eof? == false

この点もこの連載では何度か書きましたが、 if や while の条件文で true や false と比較するのは冗長すぎます。 特に、メソッド名に「?」が付いていれば返り値が真偽値であるのは自明ですから、 そのうえさらに true や false と比較すべきではありません。 上記の行は、次のように書けば十分です。

        until f.eof?

return

細かいことを言えば (細かいことばっか言うのがこの連載なわけですが)、 return が付いているところと 付いていないところが混在している点も気になります。

例えば Yuki::Direction.cr には return が付いています。

    def Direction.cr
      return @@cr
    end

しかし Miyako::Rect#to_s には return が付いていません。

    def to_s
      @p.to_s+","+@s.to_s
    end

return を付けるか付けないかは趣味の問題ですが、 一つのライブラリではどちらかに統一しましょう。

ついでに言えば、 文字列の埋め込み式 (#{...}) を使うと値に対して自動的に #to_s を呼んで文字列化してくれるので、 上記の Miyako::Rect#to_s は次のように書けます。

    def to_s
      "#{@p},#{@s}"
    end

例外

なんだかよくわからない文を発見しました。

      raise YukiError,new("Command Label is Empty!") if label == nil

「,」はたぶん「.」の打ち間違いでしょう。 また、例外を投げるなら raise ExceptionClass, "message" の形を使うほうが一般的でもあります。できるだけ引数二つの形式を使いましょう。

ついでに言えば、「〜〜 if label == nil」より 「〜〜 unless label」がわたし好みです。

以上の二つの指摘をまとめて改善すると、次のようになります。

      raise YukiError, "command label is empty" unless label

クラスによる分岐

続いて、クラスによる分岐について話します。 以下は Yuki::Plot#[] のコードですが、 類似のコードは全体に渡って散見されました。

    def [](name)
      raise YukiError.new("name is Not String or Symbol!") unless name.kind_of?(String) || name.kind_of?(Symbol)
      name = name.intern if name.class == String
      raise YukiError.new("Illegal scenario name! : "+ name.to_s) unless @scnr.include?(name)
      @scnr[name].reset(false)
      return @scnr[name]
    end

原則として、値のクラスを見て分岐するのは避けるべきです。 上記のコードなら、単にシンボルと文字列以外は無視してしまえばよいでしょう。 明示的なクラスチェックをやめるとエラーメッセージは多少不親切になりますが、 name が文字列でもシンボルでもなければ どうせ @scnr[name] が nil になってエラーになりますから、 たいした問題ではありません。

さらに、もしできることなら、 シンボルと文字列の両方を受け入れないほうがよいと思います。 コードの意味も考えると、 引数の name はゲームのシナリオファイルの名前だと想像できます。 それならば文字列だけ受け付ければ十分でしょう。 File.open にシンボルを渡すことがまずないように、 ファイル名だとかそれに類するものに無理してシンボルを使っても不便なだけです。

以上の方針に従って書き直すと、Yuki::Plot#[] は以下のように単純化できます。

    def [](name)
      scenario = @scnr[name] or raise YukiError, "unknown scenario: #{name.inspect}"
      scenario.reset(false)
      scenario
    end

元のコードより意図がはっきりしたのではないでしょうか。

ついでに言えば (今回このフレーズ多いなあ)、 インスタンス変数名は変に略さず、@scenarios などとしたほうがよいと思います。

if の連鎖

次に Yuki::Compiler.compile で以下のようなコードに遭遇しました。

            if com =~ /^color[=\s\t]+(.+)$/
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::FONTCOLOR, Miyako::Color.to_rgb($1), :setting))
            elsif com =~ /^size[=\s\t]+(\d+)$/
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::FONTSIZE, $1.to_i, :setting))
            elsif com == "pause"
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::PAUSE))
            elsif com == "clear"
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::CLEAR))
            elsif com == "cr"
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::CR))
            elsif com =~ /^message_wait[=\s\t]+(\d*\.?\d+)$/
                       (以下略)

このあとも 30 回近く、com に対する比較が続きます。 これはもう、どう考えても case 文を使う場面でしょう。

            case com
            when /^color[=\s\t]+(.+)$/
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::FONTCOLOR, Miyako::Color.to_rgb($1), :setting))
            when /^size[=\s\t]+(\d+)$/
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::FONTSIZE, $1.to_i, :setting))
            when "pause"
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::PAUSE))
            when "clear"
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::CLEAR))
            when "cr"
              cl = add(cl, tmc + tm, spr)
              tmc = ""
              cl.push(Direction.new(Direction::CR))
            when /^message_wait[=\s\t]+(\d*\.?\d+)$/
                       (以下略)

これで多少よくなりました。

when の連鎖

しかし、ここで安心してはいけません。 同じような when 節が並ぶ case 文を見たら「これはヤバい」と考えるべきです。 いわゆる「不吉なコードのにおい」というやつです。

上記の case 文は、例えば次のように変形できます。

COMMAND_DESCRIPTORS = [
  [/^color[=\s\t]+(.+)$/,
   lambda {|m| Direction.new(Direction::FONTCOLOR, Miyako::Color.to_rgb(m[1]), :setting) }],
  [/^size[=\s\t]+(\d+)$/,
   lambda {|m| Direction.new(Direction::FONTSIZE, m[1].to_i, :setting) }],
  ["pause",
   lambda {|m| Direction.new(Direction::PAUSE) }],
  ["clear",
   lambda {|m| Direction.new(Direction::CLEAR) }],
  ["cr",
   lambda {|m| Direction.new(Direction::CR) }],
         :
         :
]

class Compiler
  def Compiler.compile(c, spr)
         :
         :
        pattern, action = COMMAND_DESCRIPTORS.detect {|re, | re =~ com }
        if pattern
          m = pattern.match(com)
          cl = add(cl, tmp + tm, spr)
          tmp = ""
          cl.push action.call(m)
        else
          # どの正規表現にもマッチしなかった場合のコード
        end
      end

つまり、各 when 節で違う部分だけを Proc オブジェクトにくくりだして、 when 節 (コード) のリストをデータのリストに変換するわけです。

正規表現の問題

それから、Yuki::Compiler.compile はコードの意味的にも問題があります。 例えば color コマンドを次のような正規表現で解析していますが、

/^color[=\s\t]+(.+)$/

この正規表現は「"color = === = #FFFFFF"」にもマッチしてしまいます。 もしかするとそういう文法のスクリプトファイルなのかもしれませんが、 ちょっとありえない感じがします。きっと間違いでしょう。

それから、上記の表現だと "color = #FFFFFF RED" とマッチさせた場合に $1 が "#FFFFFF RED" になります。この文字列を渡す先のコード (Miyako::Color.to_rgb) は末尾に関係ない文字列が付いていても解析できるようなので、 問題ないと言えばないのですが、 Miyako::Color.to_rgb("#FFFFFF RED") が問題なく通ってしまうのはそれ自体まずいでしょう。 Yuki::Compiler.compile と Miyako::Color.to_rgb の両方でもう少し厳密にパースするべきです。

さらに言えば、正規表現で「^」や「$」を気軽に使うのはお勧めできません。 「^」は「行頭」、「$」は「行末」を表すメタ文字です。 しかし、Yuki::Compiler.compile で表現したいのは、 本当に「行頭」や「行末」なのでしょうか。 表現したいことは「文字列先頭」「文字列末尾」ではないでしょうか。 もしそうなら、「文字列先頭」は「\A」、「文字列末尾」は「\z」とちゃんと書くべきです。 一文字で短いからいいやーとか言って「^」「$」で代替するのは「ダメな」サボりです。

共通構文の導入

このメソッドをより根本的に改善するには、 コマンドごとにバラバラの正規表現を使うのをやめて、共通の構文を導入すべきです。 ざっと見た感じでは「xxxx = yyyy」の形式のコマンドと 「xxxx」の形式のコマンドがあるようなので、まずは各コマンドをこの二つに分類します。

 case com
 when /\A\s*(\w+)\s*=/m
   name = $1
   value = $'.strip
   # 略
 when /\A\s*\w+\s*\z/
   name = com.strip
 else
   raise CompileError, "syntax error: #{com.inspect}"
 end

そのうえで「xxxx = yyyy」コマンドについては yyyy をコマンドごとに解析します。

 VALUE_PARSER = {
   'color' => lambda {|str| Direction.new(Direction::FONTCOLOR, Miyako::Color.to_rgb(str) },
   'size'  => lambda {|str| Direction.new(Direction::FONTSIZE, Integer(str), :setting) },
      :
      :
 }

 def Compiler.compile(c, spr)
      :
      :
     case com
     when /\A\s*(\w+)\s*=\s*(.*)\z/m
       name = $1
       value = $2.strip
       parser = VALUE_PARSER[name] or
           raise CompileError, "unknown command: #{name.inspect}"
       cl.push parser.call(value)
       # 以下略

このように解析すればより正確に、厳密に解析できるはずです。

あとは定数 Direction.new(Direction::XXXX, ...) という字面がウザったいので Direction::XXXX ごとにクラスを作るとか、なにかしら改善したいところですね。 しかし、そこまで行くと大改造になりそうなので、本稿ではこのへんでやめておきます。

クラスによる分岐 (2)

さきほど Miyako::Color.to_rgb についてちょっとふれたので、 このメソッドをもう少し見てみます。

    def Color::to_rgb(v)
      return nil unless v
      if v.kind_of?(Array)
        raise MiyakoError.new("Illegal color array!") unless v.length == 3
        return v
      end
      return cc2rgb(v) if v.kind_of?(Integer)
      return str2rgb(v.to_s) if v.kind_of?(Symbol)
      return str2rgb(v) if v.kind_of?(String)
      raise MiyakoError.new("Illegal parameter")
    end

すでに書いた通り、値のクラスで分岐するのはやめましょう。 このケースなら、そもそも、 いろいろな種類の値をまとめて受け取るような仕様にするのが間違いです。 Ruby には静的型がありませんが、 だからと言って型について考えなくていいわけではありません。 ある値がどんな型なのかわからない、あるいはわざと混同するようなコードは、 たとえ Ruby であっても問題があります。

また、クラスで分岐するにしても、せめて case 文で分岐したいものです。 case 文を使うと上記のメソッドは次のように書き直せます。

    def Color::to_rgb(v)
      return nil unless v
      case v
      when Array
        raise MiyakoError, "size of color array must be 3" unless v.size == 3
        v
      when Integer then cc2rgb(v)
      when Symbol  then str2rgb(v.to_s)
      when String  then str2rgb(v)
      else
        raise MiyakoError, "wrong type of argument: #{v.inspect} (#{v.class})"
      end
    end

eval

次に、Miyako::Color.to_rgb のコード中に出てくる str2rgb も見てみましょう。

    def Color::str2rgb(str)
      return eval("["+str+"]") if str =~ /^\[?(\s*\d+\s*,\s*\d+\s*,\s*\d+\s*)\]?$/
      return hex2rgb(str) if str =~ /^\#[\da-fA-F]{6}/
      const_str = str.upcase
      return eval("Miyako::Color::"+str.upcase) if Module.constants.include?(const_str)
      raise MiyakoError.new("Illegal parameter")
    end

このメソッドにもいくつか問題があります。

まず第一に、eval を使いすぎです。 例えば本体一行目などは、わざわざ正規表現マッチをしているのだから、 そのマッチ結果を使って部分文字列を取り出せば 簡単に eval をなくせます。以下に書き換え例を示します。

    def Color::str2rgb(str)
      case str
      when /\A\[\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\]\z/
        [$1.to_i, $2.to_i, $3.to_i]
      when /\A\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\z/
        [$1.to_i, $2.to_i, $3.to_i]

元のコードだと配列が一重になったり二重になったりして不都合なので、 正規表現を二つに分けて対応しました。

定数に関するリフレクション

次に、もう一つの eval の行を見てください。

      const_str = str.upcase
      return eval("Miyako::Color::"+str.upcase) if Module.constants.include?(const_str)

このコードは、色名と同名の定数 (RED とか) が Miyako::Color に定義されていたら、その定数の値を返す、 という意味です。

const_str と str.upcase の使い分けがはっきりしない……という点は置いとくとしても、 ここで Module.constants を使うのは明らかに誤りです。 Module.constants は「その場でアクセスできる全定数の名前の配列」を返します。 まあ百聞は一見に如かずなので、実際に動かして見てみましょう。

$ ruby-1.8.5 -e '
  module Miyako
    class Color
      RED = 1
      p Module.constants.sort
    end
  end
'
["ARGF", "ARGV", "ArgumentError", "Array", "Bignum", "Binding",
"Class", "Color", "Comparable", "Continuation", "Data", "Dir", "ENV",
"EOFError", "Enumerable", "Errno", "Exception", "FALSE", "FalseClass",
"File", "FileTest", "Fixnum", "Float", "FloatDomainError", "GC",
"Hash", "IO", "IOError", "IndexError", "Integer", "Interrupt",
"Kernel", "LoadError", "LocalJumpError", "Marshal", "MatchData",
"MatchingData", "Math", "Method", "Miyako", "Module", "NIL",
"NameError", "NilClass", "NoMemoryError", "NoMethodError",
"NotImplementedError", "Numeric", "Object", "ObjectSpace", "PLATFORM",
"Precision", "Proc", "Process", "RED", "RELEASE_DATE",
"RUBY_PLATFORM", "RUBY_RELEASE_DATE", "RUBY_VERSION", "Range",
"RangeError", "Regexp", "RegexpError", "RuntimeError", "STDERR",
"STDIN", "STDOUT", "ScriptError", "SecurityError", "Signal",
"SignalException", "StandardError", "String", "Struct", "Symbol",
"SyntaxError", "SystemCallError", "SystemExit", "SystemStackError",
"TOPLEVEL_BINDING", "TRUE", "Thread", "ThreadError", "ThreadGroup",
"Time", "TrueClass", "TypeError", "UnboundMethod", "VERSION",
"ZeroDivisionError"]

このように、確かに "RED" も入ってはいますが、トップレベルの定数まで含まれてしまっています。 それに対して、Miyako::Color.str2rgb で欲しいのは Miyako::Color の「直下」に定義されている定数名だけのリストでしょう。 いまの Miyako のコードでは、「ARGF」や「NIL」という色が存在することになってしまいます。 あるクラスに定数が定義されているか調べるのであれば、 Module#const_defined? か Module#constants を使うべきです。

……と、言いたいところなのですが、残念ながら、それでも間違いです。 次のように、Module#const_defined? を使ってもトップレベルの定数が見えてしまいます。

module Miyako
  class Color
    RED = 1
  end
end

p Miyako::Color.const_defined?(:ARGF)   # => true

それでは Module#constants を使えばどうでしょうか。 次のような実験をする限り、大丈夫に見えます。

module Miyako
  class Color
    RED = 1
  end
end
p Miyako::Color.constants   # => ["RED"]

ところが、Module#constants を使う場合にもきっちり罠があります。 Miyako::Color は Object から直接継承しているから大丈夫ですが、 もし別のクラスを継承していると、次のように、そのクラスの定数も入ってしまうのです。

module Miyako
  class AbstractColor
    CONST = "全然関係ない定数"
  end

  class Color < AbstractColor
    RED = 1
  end
end

p Miyako::Color.constants   # => ["RED", "CONST"]

したがって、任意のクラス c について、c に定義されている定数だけを得るには、 c.constants から c のスーパークラス (c.ancestors) の定数をすべて取り除く必要があります。 ただし、その場合は逆に定数を「取り除きすぎてしまう」可能性があるので、 やはり完全な方法とは言えません。 つまり、「任意のクラス c について、c に定義されている定数だけを得る」 方法は「存在しない」が正解です。

まあ、ここまであからさまに書いておけば、 きっと誰かが Module#constants に引数を追加してくれるでしょう。

さて、元のコードに戻ると、 定数の値を得るためだけに eval を使うのもイケてません。 ここは Module#const_get を使うべきでしょう。

以上の変更をすべてまとめると、 Miyako::Color.str2rgb は次のように書き換えられます。

    def Color.str2rgb(str)
      case str
      when /\A\[?\s*(\d+)\s*,\s*(\d+)\s*,\s*(\d+)\s*\]?\z/
        [$1.to_i, $2.to_i, $3.to_i]
      when /\A\#[\da-fA-F]{6}\z/
        hex2rgb(str)
      else
        if constants().include?(str.upcase)
          const_get(str.upcase)
        else
          raise MiyakoError, "wrong color expression: #{str.inspect}"
        end
      end
    end

そもそも、定数を使うのをやめればこんな苦労はしなくて済みます。 例えば次のように色名から RGB 値への Hash を使えば簡単に検索できます。

 COLORS = {
   'BLACK' => [  0,   0,   0],
   'WHITE' => [255, 255, 255],
   'RED'   => [255,   0,   0],
     :
     :
 }

Ruby だからと言って無理にリフレクションを使う必要はありません。 まずは素直に書きましょう。

プラットフォームの判別

さて、また別のコードに移ります。

プラットフォーム名を判別するために、 次のようなコードが書いてありました。

  osn = Config::CONFIG["target_os"].downcase
  @@osName = osn =~ /win/ ? "win" : (osn =~ /linux/ ? "linux" : "other")

Config::CONFIG['target_os'] は RUBY_PLATFORM の OS 部分と同じで、 "linux" とか "mswin32" のような文字列です。

このコードで何がまずいかと言うと、「/win/」の部分です。 この正規表現は "darwin" にもマッチしてしまうので、 Mac OS X が Windows だと判定されてしまいます。

まあ、Readme.txt を読むと、 Miyako は Windows と Linux を想定していると書いてあるので それ以外の OS なんてどーでもいいような気もしますが、 せっかくならもう少し細かくチェックしたほうがいいかなあと思います。

ちなみに、Windows かどうか判定するには次のような正規表現を使います。

/mswin(?!ce)|mingw|cygwin|bccwin/

この正規表現では mswince (Windows CE) を除外していますが、 含めたほうがいい場合もありえます。 また、interix (Services for Unix) も入れたほうがいいかもしれません。 それから、きっとそのうち mswin64 も使われるようになると思うので、 「mswin32」とは指定しないほうがよいでしょう。

それにしても、そもそも、 正規表現で名前をチェックしないといけないのがどうも嫌な感じがします。 Windows かどうか調べるなら、 例えば Win32API.so が require できるかどうかで チェックするというのもアリかもしれません。

ウェブ日記システム MidoreDayBook

続いてはウェブ日記システム MidoreDayBook を添削します。 このプログラムは伊藤みどりさんに提供していただきました。 ライセンスは Ruby License です。

添削に登場する主なファイルは次の二つです。

この日記システムは日記データが XML で、 それを XSL で書き換えて HTML を生成しているところがポイントのようです。 ただし、この添削では XML 関係の話はまったく出てきません。

ファイル構成

まずファイル構成から見ましょう。 ソースコードの daybook/mycgi/daybook 以下がプログラムのディレクトリらしいので、 その下のファイルをリストアップします。

$ ls daybook/**/*.rb
daybook/base/daybook_lib.rb
daybook/base/daybook_mod.rb
daybook/base/daybook_print.rb
daybook/daybook.rb
daybook/daybook_admin_review.rb
daybook/daybook_comment.rb
daybook/daybook_cont.rb
daybook/daybook_google.rb
daybook/daybook_logmail.rb
daybook/daybook_new.rb
daybook/daybook_search.rb
daybook/daybook_viewindex.rb
daybook/index/d_index.rb
daybook/newpost/d_check.rb
daybook/newpost/d_commentmail.rb
daybook/newpost/d_new.rb
daybook/newpost/d_params.rb
daybook/owner/d_google.rb
daybook/owner/d_remake.rb
daybook/search/d_search.rb

まず、ファイル名だけからは判別しにくいですが、 このリストにはコマンドとライブラリが入り混じっています。 daybook_*.rb がコマンドで、それ以外がライブラリです。

daybook_*.rb がコマンドでそれ以外が ライブラリという命名法は直感的ではありません。 わたしの感覚だと、むしろ daybook_*.rb がライブラリで、 d_*.rb がコマンドだろうと感じられます。

どうも元の配置だと何が何だかよくわからなかったので、 ファイルを次のように並べなおしてみました。 ついでに、ファイル名も次に述べるルールに合わせて変えてあります。

$ find daybook
daybook
daybook/bin
daybook/bin/cont.rb
daybook/bin/logmail.rb
daybook/bin/admin_review.rb
daybook/bin/new.rb
daybook/bin/google.rb
daybook/bin/search.rb
daybook/bin/viewindex.rb
daybook/bin/comment.rb
daybook/lib
daybook/lib/daybook
daybook/lib/daybook/htmlremake.rb
daybook/lib/daybook/makexml.rb
daybook/lib/daybook/getparams.rb
daybook/lib/daybook/mainbase.rb
daybook/lib/daybook/checkpath.rb
daybook/lib/daybook/checkuri.rb
daybook/lib/daybook/newpost.rb
daybook/lib/daybook/googlesitemap.rb
daybook/lib/daybook/readwrite.rb
daybook/lib/daybook/print.rb
daybook/lib/daybook/commentmail.rb
daybook/lib/daybook/mysearch.rb
daybook/lib/daybook/newindex.rb
daybook/lib/daybook.rb
daybook/config

ここではわかりやすくするために bin/ と lib/ を作りましたが、 必ずしも作らなくても構いません。

ウェブアプリケーションだとコマンドを置いた場所が プロセスのカレントディレクトリになるの普通なので、 CGI を動かすディレクトリにいきなりコマンドを置いておくと便利です。 また、Ruby では $LOAD_PATH に「.」(カレントディレクトリ) が入っているため、 CGI ディレクトリにライブラリを置いておくと、 何も設定しなくとも require できて便利だという意見があります。

ちなみに、わたしがウェブアプリケーションを書く場合は、 CGI プログラムはその場に置きますが、 ライブラリは別の場所 (/usr/local/lib 以下など) に置くことにしています。 つまり、以下のようなレイアウトにします。

 /var/www/tree/                    DocumentRoot
 /var/www/tree/wiki/               CGI を動かすディレクトリ
 /var/www/tree/wiki/index.cgi      CGI プログラム (実行可能ファイル)
 /var/www/tree/wiki/config         設定ファイル
 /usr/local/lib/bitchannel/        ソースコード
 /usr/local/lib/bitchannel/lib/    ライブラリ (*.rb)
 /var/bitchannel/                  データ

上記の配置では index.cgi か設定ファイルの中で $LOAD_PATH に "/usr/local/lib/bitchannel/lib" を追加します。

ファイル名とクラス名

次にクラス名を見てみましょう。 例によって rdefs コマンドを使います。

$ for i (*/*.rb) {
>   echo "----- $i"
>   rdefs --class $i
>   echo
> }
----- base/daybook_lib.rb
class MainBase

----- base/daybook_mod.rb
module CheckPath
module ReadWrite
module MakeXML
  include REXML

----- base/daybook_print.rb
module PrintMessage
class DaybookPrint
include PrintMessage

----- index/d_index.rb
class NewIndex
  include MakeXML
  include ReadWrite

----- newpost/d_check.rb
class CheckURI

----- newpost/d_commentmail.rb
class DaybookCommentMail
  include ReadWrite

----- newpost/d_new.rb
class NewPost
  include MakeXML
  include ReadWrite

----- newpost/d_params.rb
class GetParams

----- owner/d_google.rb
class GoogleSiteMap
  include MakeXML
  include ReadWrite

----- owner/d_remake.rb
class HTMLRemake
  include CheckPath
  include MakeXML
  include ReadWrite

----- search/d_search.rb
class MySearch
  include MakeXML
  include ReadWrite

この構成で問題なのは、 クラス名とファイル名 (ライブラリ名) に何の関連もないところです。

Ruby では、そのファイルの主要なクラス・モジュール名を downcase してファイル名を決めます。 例えば FileUtils モジュールのファイル名は fileutils.rb です。 CGI クラスのファイル名は cgi.rb です。

また、クラスやモジュールがネストしている場合は、 ネスト関係をディレクトリ構造に反映させます。 例えば Net::HTTP クラスのファイル名は net/http.rb です。 Test::Unit::TestCase クラスのファイル名は test/unit/testcase.rb です。

ですから、このプログラムで言えば、 MainBase クラスは mainbase.rb というファイルに入れるべきです。

また、自分のアプリケーションのクラスをトップレベルに定義してしまうと、 他のライブラリと名前が衝突する可能性があります。 アプリケーション用のモジュールを一つ決めて、その中にネストするようにしましょう。

例えば Daybook モジュールにネストさせるなら、次のように書きます。

module Daybook
  class MainBase
  end
end

p Daybook::MainBase.new   # モジュールの外からアクセスするときは「::」を使う

このルールに従ってクラスとモジュールを再配置してみました。

$ rdefs --class lib/daybook/*.rb
module Daybook
  module CheckPath
module Daybook
  class CheckURI
module Daybook
  class CommentMail
    include ReadWrite
module Daybook
  class GetParams
module Daybook
  class GoogleSiteMap
    include MakeXML
    include ReadWrite
module Daybook
  class HTMLRemake
    include CheckPath
    include MakeXML
    include ReadWrite
module Daybook
  class MainBase
module Daybook
  module MakeXML
    include REXML
module Daybook
  class MySearch
    include MakeXML
    include ReadWrite
module Daybook
  class NewIndex
    include MakeXML
    include ReadWrite
module Daybook
  class NewPost
    include MakeXML
    include ReadWrite
module DayBook
  module PrintMessage
  class Print
    include PrintMessage
module Daybook
  module ReadWrite

#!

ファイルの中を眺めていると変なことに気付きました。 「#!」の入ってるファイルが異様に多いのです。

#!/usr/local/bin/ruby
#  encoding: utf-8
#  daybook.rb
#!/usr/local/bin/ruby
#  encoding: utf-8
#  daybook_lib.rb
#!/usr/local/bin/ruby
#  encoding: utf-8
#  d_index.rb

しかも、どう見てもライブラリにしか見えないファイルにも「#!」が入っています。 どうやらすべてのファイルに「#!」を入れているようです。

「#!」は、UNIX において、 インタプリタ経由で実行するコマンドを作るときに使います。 ですから、ライブラリにまで #! を入れる必要はありません。 また、ファイルに実行可能属性をつける必要もありません。

コードのレイアウト

以下は index/d_index.rb からの抜粋です。

    h_sort.each{|aray|

      item_comment = REXML::Element.new( "item_comment" )
      v = aray[1]
      v.each{|i| i_name = item_comment.add_element(i.name); i_name.add_text(i.text) }
      comments.add_element(item_comment)
      }
    #----------
    return comments
  end
  def p_admin_xml_make(archives_list_targetxml)
    h = Hash.new{|h, key| h[key] = []}
    archivesdoc = m_make_new_xml("archives")
    docroot = archivesdoc.root
    meta = docroot.add_element("metadate")
    meta.add_text(Time.now.to_s)
    docroot.add_text("\n")
    items = docroot.add_element("items")
    #----------
    archives_list_targetxml.each{|f|
     onefiledoc = Document.new(File.new( f ))
     entry = REXML::Element.new( "entry" )
     @list_element.each{|e| entry.add_element(onefiledoc.elements[e]) unless onefiledoc.elements[e].nil? }

     onefiledoc.root.get_elements('item_comment').each{|com|
       comments = REXML::Element.new( "comments" )
       comments.add_element(com.elements["date"])
       comments.add_element(com.elements["postno"])
       comments.add_element(com.elements["name"])
       entry.add_element(comments)
       }

     y = onefiledoc.root.elements['meta/metadate/d_y'].text

スタイルについて三点指摘します。

まず、インデントが一貫していません。 2 でも 3 でも 4 でもいいですが、とにかくインデントはきっちり揃えましょう。 最初はタブの設定が違うのかとも思いましたが、 インデントはすべて空白なのでタブのせいではなさそうです。

第二に、空白の使いかたにも統一性がありません。 メソッド呼び出しのときに括弧の内側に空白を入れるのか入れないのか、 どちらかに揃えましょう。

第三に、空行に無頓着すぎます。 せめてメソッド定義の間くらいは空行を入れましょう。 別のファイルには、メソッドの真ん中で突然 2 行空いているところもありましたが、 これもよろしくありません。

それから、以下のようにセミコロンを多用する点も気になりました。

  def p_index_xml_make(list_targetxml)
    indexdoc = m_make_new_xml("daybookindex")

    docroot = indexdoc.root; docroot.add_text("\n")
    meta = docroot.add_element("metadate")
    meta.add_text(Time.now.to_s); docroot.add_text("\n")

改行文字の出力だから同じ行に書いとこうという気持ちはわかりますが、 コードの密度が高くて見にくくなっています。

また、次のコードまで来ると明らかに詰め込みすぎです。

      v.each{|i| i_name = item_comment.add_element(i.name); i_name.add_text(i.text) }

以下のように、素直に複数行に分けましょう。

      v.each {|i|
        i_name = item_comment.add_element(i.name)
        i_name.add_text(i.text)
      }

private メソッドの命名規則

またまた rdefs コマンドを使って、定義されているメソッドを見てみます。

$ rdefs daybook/lib/daybook/checkuri.rb
module Daybook
  class CheckURI
    def initialize(myhash)
    def daybook_comment_check
    def daybook_newpage_check
    def daybook_search_check
    private
    def p_check_script_name(ref_buttonname, env_buttonname, error_list)
    def p_check_host(ref, uri, error_list)
    def p_check_path(ref, uri, error_list)
    def p_check_index_ref(ref, uri, error_list)

どうやら private メソッドにはプリフィクス「p_」を付ける規則があるようです。 C++ なんかではありがちですが、Ruby では初めて見ました。 絶対にメソッド名にプリフィクスを付けるなとまでは言いませんが、 お勧めはしません。

コードを読むときに private メソッドかどうか 区別しなければいけない理由がわかりませんし、 ちゃんと読める程度にクラスが小さければプリフィクスなんて付けなくても private であることはすぐわかるでしょう。

require

require がどれもメソッド中に書いてある点も気になります。

  def daybook_comment_check
    require "uri";error_list = Hash.new
 def comment_sendmail(para_data)
   body = ["\n\n",Time.now.to_s,"--\n"]
    para_data.each{|key ,value| body << ["[","#{key}", ",", "#{value}","]"].to_s }
    body << "\n"
    str_mail = p_make_body(body.to_s)

    require 'net/smtp'
 def p_make_body(body)
   require "nkf"

この書きかたは一般的でもなければ、わかりやすいわけでもなく、 効率の面でもいいことはありません。 ファイル (ライブラリ) 同士の関係はファイル先頭だけ見てわかったほうが 全体の構造をつかむためには役立ちます。 require はファイル先頭にまとめて書くようにしましょう。

値の文字列化と、文字列の連結

さきほどのコードの、この行が非常にひっかかります。

    para_data.each{|key ,value| body << ["[","#{key}", ",", "#{value}","]"].to_s }

まず、値を文字列化するためだけに文字列埋め込み式 ("#{..}") を使うのはやめましょう。 文字列埋め込み式とは、あくまでも、文字列に式の値を埋め込むときに使うものです。 値を文字列化することが主眼になってはいけません。 ここは明示的に key.to_s, value.to_s と書くべきです。

また、Array#to_s を使って文字列配列を連結していますが、 これも非常によろしくありません。 通常は Array#to_s を文字列の連結のためには使いませんからコードの意図がはっきりしませんし、 Ruby 1.9 以降では Array#to_s の動作が変わっています。 配列に入った文字列を連結するなら、Array#join を使うべきです。

また、この場合は単に「+」で連結していっても問題ありません。 さらに言えば、文字列埋め込み式を使って次のように書けば済む話でもあります。

    para_data.each{|key, value| body << "[#{key},#{value}]" }

ついでに言えば、この式も無理に一行で書く必要はありませんね。 また、para_data という変数名も好きになれません。 わたしなら params にします。

    params.each do |key, value|
      body << "[#{key},#{value}]"
    end

SAFE

各コマンドの最初で、SAFE という定数に代入していました。

  SAFE = 3

しかし、定数 SAFE を参照している場所はどこにもないので、 これは $SAFE の間違いのように思えます。 改めて考えると、確かに間違いやすそうな字面ではありますね。

ただ、このコードを $SAFE=3 に直してもいいものかどうか、確信が持てません。 $SAFE=3 はかなり制限がきついので、 CGI プログラムを動かすには適していません。 一般に、各 $SAFE レベルにはそれぞれ次のような場面が対応します。

$SAFE=0 セキュリティを考えなくてよいとき。
$SAFE=1 扱うデータが信用できないとき。CGI プログラムなど。
$SAFE=2 (存在価値がよくわからない)
$SAFE=3 $SAFE=4 でプログラムを動作させる準備をするとき。
$SAFE=4 コードが信用できないとき。安全かどうかわからないプラグインなど。

このプログラムは CGI プログラムなので、 $SAFE=1 くらいにしておいたほうがよいでしょう。

それはそれとして、個人的な意見としては、Ruby の $SAFE は信用できません。 $SAFE がらみのセキュリティホールはこれまでに嫌というほど見付かっていますし、 特に拡張ライブラリにはまだかなりの数の穴が残っていると予想されます。 あまり $SAFE を過信したプログラムを書かないように気をつけてください。

ファイルに関するレースコンディション

次の行を見てください。

      a_time = File.atime(html) if FileTest.exist?(html)

実はここのコードをちゃんと読んでいないので確信は持てないのですが、 このコードにはレースコンディションがありそうです。 FileTest.exist? から File.atime までの間にファイルが削除されると File.atime が失敗してしまいます。

この行のほかにも、FileTest.exist? を使って ファイルの存在をチェックしている個所がいくつかありました。 しかし一般に言って、CGI プログラムのように複数のプロセスが同時に動く場面では、 一つのファイルに二つ以上のシステムコールでアクセスしてしまうと失敗する場合があります。 つまり、この場合なら、

  1. ファイルの存在をチェックする (stat(2) 一回目)
  2. ファイルのアクセス時刻を得る (stat(2) 二回目)

という二つの操作に分かれているのがまずいわけです。 このコードはいきなり atime を得て (stat して)、 例外 Errno::ENOENT を rescue したほうがよいと思います。

もちろん、あらかじめデータベースをロックして、 一つのプロセスしかファイルには触らないようになっているのであれば話は別です。 しかし、このコードではデータベースをロックしているような雰囲気がなかったので、 ロックするように変えるよりはファイルアクセスをアトミックにしたほうが楽だと考え、 上記のように指摘しておきます。

ファイルのロック

どうやってロックしているのかさらに調べてみると、 以下のようなコードが見付かりました。

  def m_write_file(path_file, str)
    begin
      File.open(path_file, File::WRONLY | File::CREAT){|w|
      w.flock(File::LOCK_EX)
      w.print str
      }
    rescue SystemCallError
      $stderr.print "IO failes: " + $!
      raise
    end
  end

このメソッドには以下の MakeXML#m_read_xml が対応します。

 def m_read_xml(path_xml)
   doc = Document.new()
   doc << XMLDecl.new(version="1.0", encoding="UTF-8" )
   doc.add_text("\n")

   sourcedoc = Document.new(File.new( path_xml ))
   docroot = sourcedoc.root
   doc.add_element(docroot)

   return  doc

 end

m_write_file では LOCK_EX でファイルをロックしていますが、 m_read_xml ではファイルをロックしていません。 このコードだと書き込み中にファイルを読み込んでしまう可能性があります。 読み込みのコードでは次のように共有ロックをかけるべきです。

   sourcedoc = File.open(path_xml) {|f|
     f.flock(File::LOCK_SH)
     Document.new(f)
   }

クラス設計

最後に、クラス設計について指摘します。

クラス名の一覧を見ると、動詞っぽい名前が多すぎます。 例を挙げると……

  • CommentMail
  • CheckPath
  • CheckURI
  • HTMLRemake
  • MakeXML
  • MySearch
  • GetParams

これは俗に言う「オブジェクトを関数の代わりに使っている」症状です。 どのクラスを見ても「このクラスは〜〜をする」という方針に基いて設計されており、 オブジェクトの分けかたとして不適切です。

例えば、ウェブ日記システムなら、日記を保存するデータベースだとかレポジトリだとか、 その手のクラスが必要になるであろうことは容易に想像できます。 しかしこのプログラムには database の文字すらありません。 いったいどこでデータにアクセスしているんだろうと思ってコードを見ていくと、 NewIndex クラスにデータベースの役割が埋め込まれていることがわかります。 データベースをデータベースとしてオブジェクトにするのではなく、 「データベースにアクセスして XML/HTML を作る」という役割をオブジェクト化してあるわけです。

以下のように各クラスの initialize の引数を見てみると、 設計のまずさがさらによくわかります。

$ grep initialize daybook/*.rb
daybook/checkuri.rb:    def initialize(myhash)
daybook/commentmail.rb:    def initialize(myhash)
daybook/getparams.rb:    def initialize
daybook/googlesitemap.rb:    def initialize(myhash)
daybook/htmlremake.rb:    def initialize(myhash)
daybook/mainbase.rb:    def initialize(mod)
daybook/mysearch.rb:    def initialize(myhash)
daybook/newindex.rb:    def initialize(myhash)
daybook/newpost.rb:    def initialize(myhash)

このように、引数がことごとく myhash です。 この myhash はすべて同じオブジェクトで、 アプリケーションの設定がすべて入っています。 つまり全てのオブジェクトで同じデータが共有されているうえ、 そのデータは生データそのものであり、 オブジェクトによって抽象化されていないということです。 こんな状況は、言うまでもなく、絶対に避けるべきです。

ではどのように設計を改善すべきでしょうか。

まず、さきほど言ったように、データベースだとか記事だとか、 明らかにモノっぽいものをまずオブジェクトにすべきです。 さらに挙げるなら、「(記事のまとまりとしての) 月」「日」、 CGI リクエスト、CGI レスポンス、記事を指定するための日付などもオブジェクトにすべきでしょう。

それから、上に挙げたオブジェクトを実際に使う場面を考えてみて、 目的を達成するために足りない機能を抽出し、オブジェクト化します。 例えば URL と記事の対応を管理するオブジェクトがあってもいいでしょう。 XML を HTML に変形するときに使われるオブジェクトも必要かもしれません。

プログラムに必要なオブジェクトがすぐに思い付かなくても心配はありません。 既存のプログラムを見てみればよいのです。 例えばウェブアプリケーションをいくつか見てみれば、 「フツー」データベースオブジェクトがあるでしょう。 ならばとりあえずデータベースオブジェクトを作ることにしてコードを書きます。 コードを書いてみてうまくハマればそれで終わりです。 不都合があるなら、問題を特定して再構成します。 再構成するときも自分でゼロから考える必要などありません。 既存の設計から似たような問題を抱えているプログラムを探してみましょう。

あとは慣れと経験です。自分の能力を信じてコードを書きまくりましょう。

Lisp もどき

添削プログラムの三本目は Lisp インタプリタです。 このプログラムは石原博さんに提供していただきました。 ライセンスは Ruby ライセンスです。

石原さんによれば、このプログラムは 「出来るだけ ruby に扱いやすくし、小さくすることを主眼にしてい」るそうです。 また、以下の制限があります。

  • 文字列リテラルなし
  • ドット対なし
  • GC なし

ソースコードは以下のファイル一つです。

  • lisp.rb (オンラインで表示)

メソッド名

このプログラムはファイル一つのアプリケーションなのでファイル構成の話は飛ばし、 いきなりプログラムの構造を見ていくことにしましょう。

$ rdefs lisp.rb
def getToken
def parse0
def parse
def pr(x)
def define(x) #関数定義
def findVar(x, env)
def evlis(l, env)
def eval(x, env)

なかなか指摘しづらい構成ですが、がんばって指摘していきたいと思います。

まず、getToken と findVar が CamelCase な点がやや気になります。 CamelCase かアンダースコア区切りか、 どちらかで統一されていれば問題ないような気もしますが、 Ruby でメソッド名が CamelCase だとどうも違和感があります。

pr と evlis については「名前を省略しすぎ」と指摘したいのですが、 Lisp というコンテキストを考えると指摘してはいけないような気がしないこともありません。 ちなみに pr はリストを文字列に戻すメソッドで、 evlis は eval list (リストを評価する) でしょうね。

eval メソッドは Ruby の Kernel#eval をオーバーライドしてしまう点が気になります。 モジュールに入れるか、あるいは「evaluate」などと名前を変えるか、 どちらかにすべきでしょう。

入力の読み込み (バックスペース編)

lisp.rb は標準入力から一行入力したあと、次のような文で行を処理しています。

    while l.gsub!(/[^\010][\010]/,"") do end

"\010" はバックスペース ("\b") ですね。 この文は、バックスペースの前の文字を消そうとしています。 String#gsub! は置換が起こると真を返すので、 上記のような式で、置換が起こらなくなるまで文字列置換を続けることができます。

しかし、そもそも、IO がバッファードモードのときに バックスペースが入力に入っていることはあるのでしょうか。 もしかして Windows では入力文字列にバックスペースが 入っていることもあるのかと思い実験してみましたが、 特にそういうわけでもなさそうです。意図がよくわかりませんでした。

それから、本体が空の while を 1 行で書くのはやめましょう。 こんな姑息な方法で行数を減らしてもいいことはありません。 ちゃんと次のように複数行で書くべきです。

while l.gsub!(/[^\010][\010]/, '')
  ;
end

あと、「l」がどうしても「1」に見えるので、 ここは「line」などにしてほしいところです。 ときどき「line より l のほうが Ruby らしい」という意見をみかけますが、 わたしはその見解には同意しません。

入力の読み込み (コメント編)

バックスペースを除去したあとは、 次のようなコードでコメントを削除しています。

    if l =~ /^;.*$/ #コメント
      print "-"
      next
    end

しかし、さらに次の行でもまたコメントを処理しているようです。

    l.chomp.scan(/\s+|;.*$|([^()' ]+|.)/){|s,| @buf.push s if s}

正規表現の二つめの選択肢 (/;.*$/) がコメントの処理です。

二ヶ所でコメントを処理すること自体は構いませんが、 二ヶ所での処理方法が違うために、出力が変化してしまいます。 次のように、コメントの前に空白があるかどうかだけで挙動が違うのです。

$ ruby lisp.rb
-> ;                行頭からコメントを書くと次の行のプロンプトは「->」
->  ;               行頭に空白を入れると次の行のプロンプトは「>」
>

これはどちらかに揃っていたほうがよいでしょうね。

while の do、if の then

次のようなコードを見る限り、while には do を付ける方針のようです。

def pr(x)
  case x
  when Integer, String then x.to_s   #数字 アトム
  when Array #リスト
    res = "("
    if x != []
      res << pr(x[0])
      x = x[1]
      while x.class == Array && x != [] do
        res << " #{pr(x[0])}"
        x = x[1]
      end
      res << " . #{pr(x)}" if x != []
    end
    res << ")"
  end
end

しかし、if には then が付いていません。 一般に言って、while の do と if の then は、 両方付けるか、両方省略するかのどちらかです。 どちらかに揃えるべきでしょう。

Array で Cons セル

続いて eval (評価) のコードに移ります。

    when "if"
      if eval(x[1][0], env) != []
        eval(x[1][1][0], env)
      else
        eval(x[1][1][1][0], env)
      end
    when "while"
      while eval(x[1][0], env) != [] do
        eval(x[1][1][0], env)
      end

こ、これはきつい……。x[1][1][1][0] の類の式が嫌すぎます。 Array を Cons セルのように使っているためにこうなってしまうのですが、 少し工夫するだけでずっとよくなりそうです。

例えば car(x) と cdr(x) を定義するだけでも cdr(car(car(x))) と書けるので、かなりマシになるはずです。 もちろん、さらに caadr や cadr を定義してもいいでしょう。

また、いっそのこと Array に #car と #cdr を定義しても構わないのではないでしょうか。 その場合は x.car.car.cdr と書けます。

Array と Hash

以下の define は関数定義のときに使うメソッドです。 これを見ると、環境 (シンボルテーブル) は Array で表現されているようです。

def define(x) #関数定義
  @fenv = [[x[1][0], [x[1][1][0], x[1][1][1][0]]]].concat(@fenv)
  x[1][0]
end

@fenv は [変数名, 変数値] という形式の Array のリストです。 また、関数を検索するときは次のように Array#assoc を使います。

        if f = @fenv.assoc(x[0])

しかし、どう考えてもここでは Hash を使ったほうが簡単で、高速です。 きっと Lisp だからリスト (Array) を使ってやろうという考えだと思うのですが、 ここでリストを使うのは無駄なこだわりと考えます。 Lisp の処理系だからと言って「いかにも Lisp ぽく」演出してやる必要はありません。

おわりに

いかがだったでしょうか。 今回はプログラム三本を一気に添削するという新機軸を打ち出してみました。 また、内容に立ち入った話よりは、一目見てわかる部分に重点を置いてみました。

今回の添削で指摘したポイントをまとめます。

  1. ファイルサイズが大きすぎるときは複数のファイルに分割する
  2. ファイル名は、そのファイルの主なクラスの名前を downcase して決める
  3. ファイルは Ruby の標準に合わせて配置する
  4. shebang 行 (#!) はコマンドにだけ付ける
  5. require はファイル先頭にまとめて書く
  6. 一つのプログラムに複数のクラス・モジュールがあるときはモジュールにネストさせる
  7. Ruby の protected は C++ とは意味が違うので注意する
  8. private メソッドだからと言って名前にプリフィクスとか付けない
  9. メソッド名が短すぎたら赤信号
  10. インデントはきっちりと
  11. 空白・空行にも気を配る
  12. return や then や do の有無はともかく、一つのライブラリで有無を統一する
  13. 複数の文を一行に詰め込まない
  14. if や while の条件式で true/false/nil と比較しない
  15. 同じオブジェクトとの比較には case 文を使う
  16. 同じような when 節が並んでいたら赤信号
  17. 値を文字列化するときは to_s を使う
  18. 文字列を連結するときは String#+ などを使う
  19. 文字列配列を連結するときは Array#join を使う
  20. 正規表現は細かいところにも注意する
  21. 値のクラスで分岐しない
  22. どうしても値のクラスで分岐したいときは case 文を使う
  23. 「何かをする」クラスを作らない
  24. リフレクションは奥が深い
  25. CGI プログラムでファイルにアクセスするときはレースコンディションに注意する

今回はえらくポイントがたくさんありますね。 これはこれでいいことかもしれません。

次回予告

例によって次回の予定は未定です。 添削してほしいプログラムをお持ちのかたは Subject に 「添削希望」と書いてるびま編集部にプログラムを送りつけてください。 ただし、添削するプログラムはオープンソースソフトウェアに限ります。

ではまた次回の添削でお会いしましょう。

著者について

青木峰郎 (あおき・みねろう)

ふつうの文系プログラマ。本業は哲学らしい。 最新刊『ふつうの Haskell プログラミング』はおかげさまで大好評発売中です。 そのうち発売のるびま本もよろしく。