July 25, 2007.

記述マナー

昨日からのバグ調査は難航。ここを直せばよいはず、という箇所を直すとなぜか計算された値が倍になってしまう。散々悩んだ挙句ソースを眺めていて違和感。なんとCのソース中にこんなコードが書いてありました(単純化しています)。

int xxxFlg = -1;

//途中の処理

if (xxxFlg = -1) {

　　xxxFlg = 1;

　　// その後の処理

}

if (xxxFlg = -1) {

　　xxxFlg = 2;

　　// その後の処理

}

「if(xxxFlg = -1)」が問題。VBなら「=」は代入以外に右辺と左辺の比較の意味を持つが、CやJavaでは代入の意味しか持たない。Javaだとコンパイルエラーとなるが、Cの場合は代入に「成功した」って事はtrueだな、とtrue扱いされてしまうため、常に処理が行なわれてしまうのです。ってこんな基本かよ...。コラ前任者出て来い！SQLを真剣に読んでいた俺の時間を返せ！

というか、そもそも処理を分けるint型のフラグがあるなら以下のように書くべきだろう。「処理を分岐させる」行為と「処理内容」を並列に書くべきではない。

int mode = DO_NOTING;

if (.....) {

　　mode = INSERT;

} else if (....) {

mode = UPDATE;

　　：

　　：

}

switch (mode) {

　　case INSERT:

　　　　// インサート処理

　　　　break;

　　case UPDATE:

　　　　// アップデート処理

　　　　break;

　　　　：

　　default :

　　　　// 何もしない

}

上記のようであれば、「//xxx処理」と書いた部分を別メソッドに切り分けることで、非常に分かりやすいコンポーネント化が可能である。こんなのプログラマが１年目に学ぶことだと思うのだが...。

他にも気に入らないところは沢山あって、例えば処理結果をそのままif文に突っ込むのはマナー違反だと思う。以下の例だと２番目は個人的に許せない。

// 許される例

if (isClosed()) {

　　writeLog("作業完了");

}

// 許されない例

if (connection.close()) {

　　writeLog("作業完了");

}

// 許されない例の改良版

boolean closed = connection.close();

if (closed) {

　　writeLog("作業完了");

}

ifやforのカッコ内に処理を書くのは良くない。可読性が下がるからだ。例えば「closeの結果にかかわらずログを出すようにして」といわれ、作成者以外が改変した場合、1行目と3行目をそのままコメントアウトする可能性がある。結果、そもそもclose()処理が呼ばれなくなってしまう。上記のようにCなら見てメソッドだと分かるからまだ良いが、VBだとメソッド呼び出しが「close」とかでできてしまうので「if (close) Then」とか書かれた日には確実にコメントアウトしてしまうはず。昨日書いた「メソッドだか変数だか分からない命名法」の話と被ると、もはや暗号に近いのです。

なので、ifに入れてよいのは「条件を判定する処理のみ」であってisClosed()のように条件を取得するだけのメソッドなら良いが、内容のあるメソッドなんて呼んではいけないのです。と声を大にしていいたいが、作った人は社内にいません。昔いたパートナと辞めた社員とオフショアチームなのです。だぁぁぁぁ...。

Copyright 2007 barista. All rights reserved.