dmpコンバータ

プログラミング

いや、dmpコンバータって名前じゃないけどね。
dmpのa2iについて。

長くなりそうなのでトラバコメントにする。
実行はマンドクセなのでやってない(スマン)
というわけでコードレビューだけ。
といっても気づいた点だけで。

重要度高

deleteがない！

mainのappのdeleteが無いよ！
もしもコイツがデバイスを占有していたらやばいよ！
(してないけど。でもディスプレイ系デバイスならXPでも多分落ちるんじゃね？　DirectXとか。)
こういうことがあるのでnew/deleteは必要が無ければ使わないようにしましょう。


exit(1)はやめようよ！

非常にヤバイことに、exit関数でスコープを抜けた場合、ローカル変数のデストラクタは呼ばれない(はず)。
これによって、finの初期化に失敗した場合、foutがリークする。
これは初期化後チェックに書き換えしても、
今度はfoutの初期化に失敗したときにfinがリークする。
ゆえに、C++でexitを使うのは多分相当ヤバイ。


ヘッダファイルに実体を書いちゃだめだー！

a2iapp.hには以下の３つの実体が存在する。

  • static string ATOK[]
  • static string IME[]
  • staic string IMEhead

これが何故ダメかといえば、ヘッダファイルに実体を書くと、インクルード先全てに同じ内容の異なる実態が生成されるからだ。
しかも、staticが付いているのでこいつはコンパイル単位に閉じ込められて外部に見えないため、リンクエラーも出ない！
これは激しくリソースの無駄遣いだろう。
というわけで、ヘッダファイルには宣言のみで、実体は書かないようにしよう。
一つの実体を多くのファイルが参照しているなら、externをつけることで達成できるが、これはそういう場合でもないようだ。
大人しくa2iapp.cppに書くとよろしい感じ。


重要度中

a2iappの多くの関数の参照引数

変更しない参照にはconstをつけたほうが良い。

void a2iapp::init(char* ifn, char* ofn)
↓
void a2iapp::init(const char* ifn, const char* ofn)

など。コンパイラの最適化を助けたり、思わぬバグを防いだりしてくれる。


fstream::closeは書かないほうがむしろ良し。

C++では、ローカルオブジェクトはスコープを外れると自動で解体される。
だからopen相当の処理はコンストラクタで、close相当の処理はデストラクタで行うのがいい。
そうすれば、解体忘れ・二重解体の悪魔から逃れられる。
fin.close() fout.close()はばっさり切ってしまえー。


無用な引数コピー

stringなどの「重たい」オブジェクトは極力無駄なコピーを避けるべき。
ゆえに、こういうときは決まってconst参照を使う。
さらに言えば、get_hashがオブジェクトの状態を変えないことを保証するためにconst関数にできる。
これは最適化に効果があったり、デバッグに効果があったりする。

unsigned char a2iapp::get_hash(string str)
↓
unsigned char a2iapp::get_hash(const string& str) const

重要度低

無用な一時変数

a2iapp::conv_headのsubstrを使っている部分。

tmp = line.substr(11,line.size() );
date = conv_date(tmp);
↓
date = conv_date( line.substr(11,line.size() ) );

これでtmpというstd::stringが不要になる。


a2iapp::initはコンストラクタで処理しても良いのでは

コンストラクタなら、ifname/ofnameの初期化が、代入ではなく初期化構文が使える。
好みかもしれないので重要度低。

void a2iapp::init(char* ifn, char* ofn){
    ifname = ifn;
    ofname = ofn;
}
↓
void a2iapp::a2iapp(char* ifn, char* ofn)
    :ifname(ifn),ofname(ofn)
{}
a2iapp::termはデストラクタで(略

上と同じ。しかも処理が無いので更にいらない子感が。


ifname/ofnameって実は無駄なのでは

わざわざstd::stringにコピーしておいて、c_str()で再び呼び出しているけど、その場でファイルストリームを開いてしまっても問題ないのではないか。
std::fstreamとstd::stringのどちらが重いかという問題になるかもしれないけど。


a2iapp::convertで、std::ifstreamの初期化エラーのチェックがすぐに行われていない

C++は宣言がどこでも出来るので、コンストラクトのエラーチェックをするなら宣言の直後が良いと思う。


a2iapp::conv_hinshiの長いスイッチ文

こういうスイッチ文は、容量的に余裕があるならテーブルを引いたほうが良いと思う。


重要度僅少

ワイド文字

たいていの場合は平気なんだが、日本語を扱うならwstring/wchar_tを使ったほうがよさげ。
エンコードの対応とかの実装は不安だから、重要度ゼロ。

getlineって

に入ってなかったっけ。
出来うる限り標準ライブラリに入っている関数を使おう。
(っていうかどっかから持ってきたのかもしかして。)


ifの論理構造に気を配る

最初のifではlineを一切書き換えていないので、
二つ目のifは前に持ってくることが出来る。

line = "";
getline_skipspace(fin,line);
if(line.compare(0,11,"!!出力日時;") == 0){
   ...
}
if(line == "") break;
↓
line = "";
getline_skipspace(fin,line);
if(line == "") break;
else if(line.compare(0,11,"!!出力日時;") == 0){
   ...
}

lineがヌル文字列だった場合、この書き換えで、ifを通る回数が一回減る。
正直どうでもいい最適化。


distCharCodeにて

無駄な処理はっけーん。
まぁ最近のコンパイラだったら、最適化で消してしまえる処理だけど。

code = error;
cerr << "Can't read" << endl;
return code;
↓
cerr << "Can't read" << endl;
return error;
この処理はいったい。

a2iapp::conv_hinshiのswitch内。

        case 0xc3: code = 10; break;
            if(str.compare("名詞形動")==0) code = 10;
            else code = 15;
            break;

いや、どう考えてもそこ通らんがな。



きょ、今日はこのぐらいで許してあげるんだから！