あなたの Ruby コードを添削します 【第 5 回】 Miyako + MidoreDayBook + Lispもどき
初稿:2006-11-26
著者: 青木峰郎
編集: ささだ
はじめに
秋です。締め切りを守れない季節です。
この冒頭の一言も、五回目ともなるとさすがにネタがなくなってきます。
やむをえず第一回の文句を変形してみたのですが、キレに欠ける面は否定できません。
そもそもわたしはなんでこんな謎の文章を毎回入れているのでしょうか。
第一回を書いたとき睡眠不足のうえ酒を飲んでいたのがまずかったのかもしれません。
今回のお題
これまで毎回一つのプログラムを添削してきたこの連載ですが、
今回は趣向を変えて、三つのプログラムを一気に添削してみることにしました。
趣向を変えた理由は二つあります。
まず、日記でプログラムを募集したら三つ来たこと。
せっかく応募が来たんだから全部使わなければ損です。
次に、これまでの添削記事が読み切れないという声が多かったこと。
既存の添削はどれも一つ一つが長く、
しかも全体で話がつながっているため、全部読み切るのが大変でした。
そこで今回は別々のプログラム三つを添削して、
それぞれ独立して読めるようにしてみたわけです。
ソースコード
では、添削対象のソースコードを示します。
ファイルが複数ある場合は、主なファイルだけをピックアップしました。
CodeReview-0017.tar.gz には全ファイルを含めてあるので、
詳しくはそちらをダウンロードして眺めてください。
ちなみに、今回は添削後のソースコードがありません。
Miyako の 5000 行を筆頭として今回はとにかく分量が多いので、
改善ポイントを指摘するに留めます。
ゲーム作成フレームワーク Miyako
最初のプログラムはゲームやプレゼンテーションのフレームワーク、「Miyako」です。
このプログラムはサイロス誠さんに提供していただきました。
ライセンスは LGPL 2.1 です。
主なファイルは以下の二つです。
ファイル構成
まず、ファイル構成について話しましょう。
Miyako は miyako.rb と 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 について指摘します。
protected と指定されているメソッドはどれも外から呼ばれていないようですし、
さらに private がまったく使われていません。
となると、たぶんこれはすべて private であるべきメソッドだと考えられます。
Ruby の protected は Java や C++ の protected とは働きが違い、
むしろ friend と同じような目的で使われます。
Ruby で Java, C++ の protected に相当するのは private です。
以下に Ruby の public, protected, private の効果をまとめておくので、
再確認してください。
可視性指定 |
効果 |
public |
制限なく呼び出せる |
protected |
同じクラスまたはサブクラスのインスタンスからのみ呼び出せる |
private |
同じインスタンスからのみ呼び出せる |
それから、Ruby のメソッド可視性とは、
「違うインスタンスからメソッドを呼び出せるかどうか」を制御する属性です。
同じインスタンスの、
違うクラスで定義されたメソッドからの呼び出しは制御できません。
つまり、C++ の private に相当する機能はありません。
それは以下の例を見るとわかります。
Ruby で protected を使うべき場面とは、例えば、
「==」を実装するために非公開のインスタンス変数にアクセスしたい場合です。
Ruby で protected が適切な場面はたいして多くありません。
自分のプログラムで protected を使った覚えのある人は用途を確認してみてください。
これまでにも (Ruby の) private のつもりで protected を使うミスが
何度か登場していたことを考えると、
この点は相当に強調しておく価値がありそうです。
この項のキャプションにも明示しておきましたので、絶対に忘れないでください。
条件文
次の行を見てください。
この点もこの連載では何度か書きましたが、
if や while の条件文で true や false と比較するのは冗長すぎます。
特に、メソッド名に「?」が付いていれば返り値が真偽値であるのは自明ですから、
そのうえさらに true や false と比較すべきではありません。
上記の行は、次のように書けば十分です。
return
細かいことを言えば (細かいことばっか言うのがこの連載なわけですが)、
return が付いているところと
付いていないところが混在している点も気になります。
例えば Yuki::Direction.cr には return が付いています。
しかし Miyako::Rect#to_s には return が付いていません。
return を付けるか付けないかは趣味の問題ですが、
一つのライブラリではどちらかに統一しましょう。
ついでに言えば、
文字列の埋め込み式 (#{…}) を使うと値に対して自動的に #to_s を呼んで文字列化してくれるので、
上記の Miyako::Rect#to_s は次のように書けます。
例外
なんだかよくわからない文を発見しました。
「,」はたぶん「.」の打ち間違いでしょう。
また、例外を投げるなら raise ExceptionClass, “message”
の形を使うほうが一般的でもあります。できるだけ引数二つの形式を使いましょう。
ついでに言えば、「〜〜 if label == nil」より
「〜〜 unless label」がわたし好みです。
以上の二つの指摘をまとめて改善すると、次のようになります。
クラスによる分岐
続いて、クラスによる分岐について話します。
以下は Yuki::Plot#[] のコードですが、
類似のコードは全体に渡って散見されました。
原則として、値のクラスを見て分岐するのは避けるべきです。
上記のコードなら、単にシンボルと文字列以外は無視してしまえばよいでしょう。
明示的なクラスチェックをやめるとエラーメッセージは多少不親切になりますが、
name が文字列でもシンボルでもなければ
どうせ @scnr[name] が nil になってエラーになりますから、
たいした問題ではありません。
さらに、もしできることなら、
シンボルと文字列の両方を受け入れないほうがよいと思います。
コードの意味も考えると、
引数の name はゲームのシナリオファイルの名前だと想像できます。
それならば文字列だけ受け付ければ十分でしょう。
File.open にシンボルを渡すことがまずないように、
ファイル名だとかそれに類するものに無理してシンボルを使っても不便なだけです。
以上の方針に従って書き直すと、Yuki::Plot#[] は以下のように単純化できます。
元のコードより意図がはっきりしたのではないでしょうか。
ついでに言えば (今回このフレーズ多いなあ)、
インスタンス変数名は変に略さず、@scenarios などとしたほうがよいと思います。
if の連鎖
次に Yuki::Compiler.compile で以下のようなコードに遭遇しました。
このあとも 30 回近く、com に対する比較が続きます。
これはもう、どう考えても case 文を使う場面でしょう。
これで多少よくなりました。
when の連鎖
しかし、ここで安心してはいけません。
同じような when 節が並ぶ case 文を見たら「これはヤバい」と考えるべきです。
いわゆる「不吉なコードのにおい」というやつです。
上記の case 文は、例えば次のように変形できます。
つまり、各 when 節で違う部分だけを Proc オブジェクトにくくりだして、
when 節 (コード) のリストをデータのリストに変換するわけです。
正規表現の問題
それから、Yuki::Compiler.compile はコードの意味的にも問題があります。
例えば color コマンドを次のような正規表現で解析していますが、
この正規表現は「”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」の形式のコマンドがあるようなので、まずは各コマンドをこの二つに分類します。
そのうえで「xxxx = yyyy」コマンドについては yyyy をコマンドごとに解析します。
このように解析すればより正確に、厳密に解析できるはずです。
あとは定数 Direction.new(Direction::XXXX, …) という字面がウザったいので
Direction::XXXX ごとにクラスを作るとか、なにかしら改善したいところですね。
しかし、そこまで行くと大改造になりそうなので、本稿ではこのへんでやめておきます。
クラスによる分岐 (2)
さきほど Miyako::Color.to_rgb についてちょっとふれたので、
このメソッドをもう少し見てみます。
すでに書いた通り、値のクラスで分岐するのはやめましょう。
このケースなら、そもそも、
いろいろな種類の値をまとめて受け取るような仕様にするのが間違いです。
Ruby には静的型がありませんが、
だからと言って型について考えなくていいわけではありません。
ある値がどんな型なのかわからない、あるいはわざと混同するようなコードは、
たとえ Ruby であっても問題があります。
また、クラスで分岐するにしても、せめて case 文で分岐したいものです。
case 文を使うと上記のメソッドは次のように書き直せます。
eval
次に、Miyako::Color.to_rgb のコード中に出てくる str2rgb も見てみましょう。
このメソッドにもいくつか問題があります。
まず第一に、eval を使いすぎです。
例えば本体一行目などは、わざわざ正規表現マッチをしているのだから、
そのマッチ結果を使って部分文字列を取り出せば
簡単に eval をなくせます。以下に書き換え例を示します。
元のコードだと配列が一重になったり二重になったりして不都合なので、
正規表現を二つに分けて対応しました。
定数に関するリフレクション
次に、もう一つの eval の行を見てください。
このコードは、色名と同名の定数 (RED とか) が
Miyako::Color に定義されていたら、その定数の値を返す、
という意味です。
const_str と str.upcase の使い分けがはっきりしない……という点は置いとくとしても、
ここで Module.constants を使うのは明らかに誤りです。
Module.constants は「その場でアクセスできる全定数の名前の配列」を返します。
まあ百聞は一見に如かずなので、実際に動かして見てみましょう。
このように、確かに “RED” も入ってはいますが、トップレベルの定数まで含まれてしまっています。
それに対して、Miyako::Color.str2rgb で欲しいのは
Miyako::Color の「直下」に定義されている定数名だけのリストでしょう。
いまの Miyako のコードでは、「ARGF」や「NIL」という色が存在することになってしまいます。
あるクラスに定数が定義されているか調べるのであれば、
Module#const_defined? か Module#constants を使うべきです。
……と、言いたいところなのですが、残念ながら、それでも間違いです。
次のように、Module#const_defined? を使ってもトップレベルの定数が見えてしまいます。
それでは Module#constants を使えばどうでしょうか。
次のような実験をする限り、大丈夫に見えます。
ところが、Module#constants を使う場合にもきっちり罠があります。
Miyako::Color は Object から直接継承しているから大丈夫ですが、
もし別のクラスを継承していると、次のように、そのクラスの定数も入ってしまうのです。
したがって、任意のクラス c について、c に定義されている定数だけを得るには、
c.constants から c のスーパークラス (c.ancestors) の定数をすべて取り除く必要があります。
ただし、その場合は逆に定数を「取り除きすぎてしまう」可能性があるので、
やはり完全な方法とは言えません。
つまり、「任意のクラス c について、c に定義されている定数だけを得る」
方法は「存在しない」が正解です。
まあ、ここまであからさまに書いておけば、
きっと誰かが Module#constants に引数を追加してくれるでしょう。
さて、元のコードに戻ると、
定数の値を得るためだけに eval を使うのもイケてません。
ここは Module#const_get を使うべきでしょう。
以上の変更をすべてまとめると、
Miyako::Color.str2rgb は次のように書き換えられます。
そもそも、定数を使うのをやめればこんな苦労はしなくて済みます。
例えば次のように色名から RGB 値への Hash を使えば簡単に検索できます。
Ruby だからと言って無理にリフレクションを使う必要はありません。
まずは素直に書きましょう。
プラットフォームの判別
さて、また別のコードに移ります。
プラットフォーム名を判別するために、
次のようなコードが書いてありました。
Config::CONFIG[‘target_os’] は RUBY_PLATFORM の OS 部分と同じで、
“linux” とか “mswin32” のような文字列です。
このコードで何がまずいかと言うと、「/win/」の部分です。
この正規表現は “darwin” にもマッチしてしまうので、
Mac OS X が Windows だと判定されてしまいます。
まあ、Readme.txt を読むと、
Miyako は Windows と Linux を想定していると書いてあるので
それ以外の OS なんてどーでもいいような気もしますが、
せっかくならもう少し細かくチェックしたほうがいいかなあと思います。
ちなみに、Windows かどうか判定するには次のような正規表現を使います。
この正規表現では mswince (Windows CE) を除外していますが、
含めたほうがいい場合もありえます。
また、interix (Services for Unix) も入れたほうがいいかもしれません。
それから、きっとそのうち mswin64 も使われるようになると思うので、
「mswin32」とは指定しないほうがよいでしょう。
それにしても、そもそも、
正規表現で名前をチェックしないといけないのがどうも嫌な感じがします。
Windows かどうか調べるなら、
例えば Win32API.so が require できるかどうかで
チェックするというのもアリかもしれません。
ウェブ日記システム MidoreDayBook
続いてはウェブ日記システム MidoreDayBook を添削します。
このプログラムは伊藤みどりさんに提供していただきました。
ライセンスは Ruby License です。
添削に登場する主なファイルは次の二つです。
この日記システムは日記データが XML で、
それを XSL で書き換えて HTML を生成しているところがポイントのようです。
ただし、この添削では XML 関係の話はまったく出てきません。
ファイル構成
まずファイル構成から見ましょう。
ソースコードの daybook/mycgi/daybook 以下がプログラムのディレクトリらしいので、
その下のファイルをリストアップします。
まず、ファイル名だけからは判別しにくいですが、
このリストにはコマンドとライブラリが入り混じっています。
daybook_*.rb がコマンドで、それ以外がライブラリです。
daybook_.rb がコマンドでそれ以外が
ライブラリという命名法は直感的ではありません。
わたしの感覚だと、むしろ daybook_.rb がライブラリで、
d_*.rb がコマンドだろうと感じられます。
どうも元の配置だと何が何だかよくわからなかったので、
ファイルを次のように並べなおしてみました。
ついでに、ファイル名も次に述べるルールに合わせて変えてあります。
ここではわかりやすくするために bin/ と lib/ を作りましたが、
必ずしも作らなくても構いません。
ウェブアプリケーションだとコマンドを置いた場所が
プロセスのカレントディレクトリになるの普通なので、
CGI を動かすディレクトリにいきなりコマンドを置いておくと便利です。
また、Ruby では $LOAD_PATH に「.」(カレントディレクトリ) が入っているため、
CGI ディレクトリにライブラリを置いておくと、
何も設定しなくとも require できて便利だという意見があります。
ちなみに、わたしがウェブアプリケーションを書く場合は、
CGI プログラムはその場に置きますが、
ライブラリは別の場所 (/usr/local/lib 以下など) に置くことにしています。
つまり、以下のようなレイアウトにします。
上記の配置では index.cgi か設定ファイルの中で $LOAD_PATH に
“/usr/local/lib/bitchannel/lib” を追加します。
ファイル名とクラス名
次にクラス名を見てみましょう。
例によって rdefs コマンドを使います。
この構成で問題なのは、
クラス名とファイル名 (ライブラリ名) に何の関連もないところです。
Ruby では、そのファイルの主要なクラス・モジュール名を downcase してファイル名を決めます。
例えば FileUtils モジュールのファイル名は fileutils.rb です。
CGI クラスのファイル名は cgi.rb です。
また、クラスやモジュールがネストしている場合は、
ネスト関係をディレクトリ構造に反映させます。
例えば Net::HTTP クラスのファイル名は net/http.rb です。
Test::Unit::TestCase クラスのファイル名は test/unit/testcase.rb です。
ですから、このプログラムで言えば、
MainBase クラスは mainbase.rb というファイルに入れるべきです。
また、自分のアプリケーションのクラスをトップレベルに定義してしまうと、
他のライブラリと名前が衝突する可能性があります。
アプリケーション用のモジュールを一つ決めて、その中にネストするようにしましょう。
例えば Daybook モジュールにネストさせるなら、次のように書きます。
このルールに従ってクラスとモジュールを再配置してみました。
#!
ファイルの中を眺めていると変なことに気付きました。
「#!」の入ってるファイルが異様に多いのです。
しかも、どう見てもライブラリにしか見えないファイルにも「#!」が入っています。
どうやらすべてのファイルに「#!」を入れているようです。
「#!」は、UNIX において、
インタプリタ経由で実行するコマンドを作るときに使います。
ですから、ライブラリにまで #! を入れる必要はありません。
また、ファイルに実行可能属性をつける必要もありません。
コードのレイアウト
以下は index/d_index.rb からの抜粋です。
スタイルについて三点指摘します。
まず、インデントが一貫していません。
2 でも 3 でも 4 でもいいですが、とにかくインデントはきっちり揃えましょう。
最初はタブの設定が違うのかとも思いましたが、
インデントはすべて空白なのでタブのせいではなさそうです。
第二に、空白の使いかたにも統一性がありません。
メソッド呼び出しのときに括弧の内側に空白を入れるのか入れないのか、
どちらかに揃えましょう。
第三に、空行に無頓着すぎます。
せめてメソッド定義の間くらいは空行を入れましょう。
別のファイルには、メソッドの真ん中で突然 2 行空いているところもありましたが、
これもよろしくありません。
それから、以下のようにセミコロンを多用する点も気になりました。
改行文字の出力だから同じ行に書いとこうという気持ちはわかりますが、
コードの密度が高くて見にくくなっています。
また、次のコードまで来ると明らかに詰め込みすぎです。
以下のように、素直に複数行に分けましょう。
private メソッドの命名規則
またまた rdefs コマンドを使って、定義されているメソッドを見てみます。
どうやら private メソッドにはプリフィクス「p_」を付ける規則があるようです。
C++ なんかではありがちですが、Ruby では初めて見ました。
絶対にメソッド名にプリフィクスを付けるなとまでは言いませんが、
お勧めはしません。
コードを読むときに private メソッドかどうか
区別しなければいけない理由がわかりませんし、
ちゃんと読める程度にクラスが小さければプリフィクスなんて付けなくても
private であることはすぐわかるでしょう。
require
require がどれもメソッド中に書いてある点も気になります。
この書きかたは一般的でもなければ、わかりやすいわけでもなく、
効率の面でもいいことはありません。
ファイル (ライブラリ) 同士の関係はファイル先頭だけ見てわかったほうが
全体の構造をつかむためには役立ちます。
require はファイル先頭にまとめて書くようにしましょう。
値の文字列化と、文字列の連結
さきほどのコードの、この行が非常にひっかかります。
まず、値を文字列化するためだけに文字列埋め込み式 (“#{..}”) を使うのはやめましょう。
文字列埋め込み式とは、あくまでも、文字列に式の値を埋め込むときに使うものです。
値を文字列化することが主眼になってはいけません。
ここは明示的に key.to_s, value.to_s と書くべきです。
また、Array#to_s を使って文字列配列を連結していますが、
これも非常によろしくありません。
通常は Array#to_s を文字列の連結のためには使いませんからコードの意図がはっきりしませんし、
Ruby 1.9 以降では Array#to_s の動作が変わっています。
配列に入った文字列を連結するなら、Array#join を使うべきです。
また、この場合は単に「+」で連結していっても問題ありません。
さらに言えば、文字列埋め込み式を使って次のように書けば済む話でもあります。
ついでに言えば、この式も無理に一行で書く必要はありませんね。
また、para_data という変数名も好きになれません。
わたしなら params にします。
SAFE
各コマンドの最初で、SAFE という定数に代入していました。
しかし、定数 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 を過信したプログラムを書かないように気をつけてください。
ファイルに関するレースコンディション
次の行を見てください。
実はここのコードをちゃんと読んでいないので確信は持てないのですが、
このコードにはレースコンディションがありそうです。
FileTest.exist? から File.atime までの間にファイルが削除されると
File.atime が失敗してしまいます。
この行のほかにも、FileTest.exist? を使って
ファイルの存在をチェックしている個所がいくつかありました。
しかし一般に言って、CGI プログラムのように複数のプロセスが同時に動く場面では、
一つのファイルに二つ以上のシステムコールでアクセスしてしまうと失敗する場合があります。
つまり、この場合なら、
- ファイルの存在をチェックする (stat(2) 一回目)
- ファイルのアクセス時刻を得る (stat(2) 二回目)
という二つの操作に分かれているのがまずいわけです。
このコードはいきなり atime を得て (stat して)、
例外 Errno::ENOENT を rescue したほうがよいと思います。
もちろん、あらかじめデータベースをロックして、
一つのプロセスしかファイルには触らないようになっているのであれば話は別です。
しかし、このコードではデータベースをロックしているような雰囲気がなかったので、
ロックするように変えるよりはファイルアクセスをアトミックにしたほうが楽だと考え、
上記のように指摘しておきます。
ファイルのロック
どうやってロックしているのかさらに調べてみると、
以下のようなコードが見付かりました。
このメソッドには以下の MakeXML#m_read_xml が対応します。
m_write_file では LOCK_EX でファイルをロックしていますが、
m_read_xml ではファイルをロックしていません。
このコードだと書き込み中にファイルを読み込んでしまう可能性があります。
読み込みのコードでは次のように共有ロックをかけるべきです。
クラス設計
最後に、クラス設計について指摘します。
クラス名の一覧を見ると、動詞っぽい名前が多すぎます。
例を挙げると……
- CommentMail
- CheckPath
- CheckURI
- HTMLRemake
- MakeXML
- MySearch
- GetParams
これは俗に言う「オブジェクトを関数の代わりに使っている」症状です。
どのクラスを見ても「このクラスは〜〜をする」という方針に基いて設計されており、
オブジェクトの分けかたとして不適切です。
例えば、ウェブ日記システムなら、日記を保存するデータベースだとかレポジトリだとか、
その手のクラスが必要になるであろうことは容易に想像できます。
しかしこのプログラムには database の文字すらありません。
いったいどこでデータにアクセスしているんだろうと思ってコードを見ていくと、
NewIndex クラスにデータベースの役割が埋め込まれていることがわかります。
データベースをデータベースとしてオブジェクトにするのではなく、
「データベースにアクセスして XML/HTML を作る」という役割をオブジェクト化してあるわけです。
以下のように各クラスの initialize の引数を見てみると、
設計のまずさがさらによくわかります。
このように、引数がことごとく myhash です。
この myhash はすべて同じオブジェクトで、
アプリケーションの設定がすべて入っています。
つまり全てのオブジェクトで同じデータが共有されているうえ、
そのデータは生データそのものであり、
オブジェクトによって抽象化されていないということです。
こんな状況は、言うまでもなく、絶対に避けるべきです。
ではどのように設計を改善すべきでしょうか。
まず、さきほど言ったように、データベースだとか記事だとか、
明らかにモノっぽいものをまずオブジェクトにすべきです。
さらに挙げるなら、「(記事のまとまりとしての) 月」「日」、
CGI リクエスト、CGI レスポンス、記事を指定するための日付などもオブジェクトにすべきでしょう。
それから、上に挙げたオブジェクトを実際に使う場面を考えてみて、
目的を達成するために足りない機能を抽出し、オブジェクト化します。
例えば URL と記事の対応を管理するオブジェクトがあってもいいでしょう。
XML を HTML に変形するときに使われるオブジェクトも必要かもしれません。
プログラムに必要なオブジェクトがすぐに思い付かなくても心配はありません。
既存のプログラムを見てみればよいのです。
例えばウェブアプリケーションをいくつか見てみれば、
「フツー」データベースオブジェクトがあるでしょう。
ならばとりあえずデータベースオブジェクトを作ることにしてコードを書きます。
コードを書いてみてうまくハマればそれで終わりです。
不都合があるなら、問題を特定して再構成します。
再構成するときも自分でゼロから考える必要などありません。
既存の設計から似たような問題を抱えているプログラムを探してみましょう。
あとは慣れと経験です。自分の能力を信じてコードを書きまくりましょう。
Lisp もどき
添削プログラムの三本目は Lisp インタプリタです。
このプログラムは石原博さんに提供していただきました。
ライセンスは Ruby ライセンスです。
石原さんによれば、このプログラムは
「出来るだけ ruby に扱いやすくし、小さくすることを主眼にしてい」るそうです。
また、以下の制限があります。
ソースコードは以下のファイル一つです。
メソッド名
このプログラムはファイル一つのアプリケーションなのでファイル構成の話は飛ばし、
いきなりプログラムの構造を見ていくことにしましょう。
なかなか指摘しづらい構成ですが、がんばって指摘していきたいと思います。
まず、getToken と findVar が CamelCase な点がやや気になります。
CamelCase かアンダースコア区切りか、
どちらかで統一されていれば問題ないような気もしますが、
Ruby でメソッド名が CamelCase だとどうも違和感があります。
pr と evlis については「名前を省略しすぎ」と指摘したいのですが、
Lisp というコンテキストを考えると指摘してはいけないような気がしないこともありません。
ちなみに pr はリストを文字列に戻すメソッドで、
evlis は eval list (リストを評価する) でしょうね。
eval メソッドは Ruby の Kernel#eval をオーバーライドしてしまう点が気になります。
モジュールに入れるか、あるいは「evaluate」などと名前を変えるか、
どちらかにすべきでしょう。
入力の読み込み (バックスペース編)
lisp.rb は標準入力から一行入力したあと、次のような文で行を処理しています。
“\010” はバックスペース (“\b”) ですね。
この文は、バックスペースの前の文字を消そうとしています。
String#gsub! は置換が起こると真を返すので、
上記のような式で、置換が起こらなくなるまで文字列置換を続けることができます。
しかし、そもそも、IO がバッファードモードのときに
バックスペースが入力に入っていることはあるのでしょうか。
もしかして Windows では入力文字列にバックスペースが
入っていることもあるのかと思い実験してみましたが、
特にそういうわけでもなさそうです。意図がよくわかりませんでした。
それから、本体が空の while を 1 行で書くのはやめましょう。
こんな姑息な方法で行数を減らしてもいいことはありません。
ちゃんと次のように複数行で書くべきです。
あと、「l」がどうしても「1」に見えるので、
ここは「line」などにしてほしいところです。
ときどき「line より l のほうが Ruby らしい」という意見をみかけますが、
わたしはその見解には同意しません。
入力の読み込み (コメント編)
バックスペースを除去したあとは、
次のようなコードでコメントを削除しています。
しかし、さらに次の行でもまたコメントを処理しているようです。
正規表現の二つめの選択肢 (/;.*$/) がコメントの処理です。
二ヶ所でコメントを処理すること自体は構いませんが、
二ヶ所での処理方法が違うために、出力が変化してしまいます。
次のように、コメントの前に空白があるかどうかだけで挙動が違うのです。
これはどちらかに揃っていたほうがよいでしょうね。
while の do、if の then
次のようなコードを見る限り、while には do を付ける方針のようです。
しかし、if には then が付いていません。
一般に言って、while の do と if の then は、
両方付けるか、両方省略するかのどちらかです。
どちらかに揃えるべきでしょう。
Array で Cons セル
続いて eval (評価) のコードに移ります。
こ、これはきつい……。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 で表現されているようです。
@fenv は [変数名, 変数値] という形式の Array のリストです。
また、関数を検索するときは次のように Array#assoc を使います。
しかし、どう考えてもここでは Hash を使ったほうが簡単で、高速です。
きっと Lisp だからリスト (Array) を使ってやろうという考えだと思うのですが、
ここでリストを使うのは無駄なこだわりと考えます。
Lisp の処理系だからと言って「いかにも Lisp ぽく」演出してやる必要はありません。
おわりに
いかがだったでしょうか。
今回はプログラム三本を一気に添削するという新機軸を打ち出してみました。
また、内容に立ち入った話よりは、一目見てわかる部分に重点を置いてみました。
今回の添削で指摘したポイントをまとめます。
- ファイルサイズが大きすぎるときは複数のファイルに分割する
- ファイル名は、そのファイルの主なクラスの名前を downcase して決める
- ファイルは Ruby の標準に合わせて配置する
- shebang 行 (#!) はコマンドにだけ付ける
- require はファイル先頭にまとめて書く
- 一つのプログラムに複数のクラス・モジュールがあるときはモジュールにネストさせる
- Ruby の protected は C++ とは意味が違うので注意する
- private メソッドだからと言って名前にプリフィクスとか付けない
- メソッド名が短すぎたら赤信号
- インデントはきっちりと
- 空白・空行にも気を配る
- return や then や do の有無はともかく、一つのライブラリで有無を統一する
- 複数の文を一行に詰め込まない
- if や while の条件式で true/false/nil と比較しない
- 同じオブジェクトとの比較には case 文を使う
- 同じような when 節が並んでいたら赤信号
- 値を文字列化するときは to_s を使う
- 文字列を連結するときは String#+ などを使う
- 文字列配列を連結するときは Array#join を使う
- 正規表現は細かいところにも注意する
- 値のクラスで分岐しない
- どうしても値のクラスで分岐したいときは case 文を使う
- 「何かをする」クラスを作らない
- リフレクションは奥が深い
- CGI プログラムでファイルにアクセスするときはレースコンディションに注意する
今回はえらくポイントがたくさんありますね。
これはこれでいいことかもしれません。
次回予告
例によって次回の予定は未定です。
添削してほしいプログラムをお持ちのかたは Subject に
「添削希望」と書いてるびま編集部にプログラムを送りつけてください。
ただし、添削するプログラムはオープンソースソフトウェアに限ります。
ではまた次回の添削でお会いしましょう。
著者について
青木峰郎 (あおき・みねろう)
ふつうの文系プログラマ。本業は哲学らしい。
最新刊『‘ふつうの Haskell プログラミング’』はおかげさまで大好評発売中です。
そのうち発売のるびま本もよろしく。
あなたの Ruby コードを添削します 連載一覧