[C++]
パッと見ても分からない
投稿者からのアピールポイント
こんなコードを見つけたので、場合分けを書きだして書き直したらこのようになりました。ちなみに、元々のコメントは何も無し。
でも、何か意図があるようで怖かったので、結局元に戻しました。(チキン)
※変数名や関数名は変えていますが、元々のコードでは一応それなりに意味のある名前になっています。
/* 注：flag2はBOOL型のメンバ変数だった。 */
BOOL flag1 = hogehogeFunction();
if (flag1) {
flag2 = (flag2==TRUE) ? FALSE : TRUE;
}
if (flag2) {
flag1 = TRUE;
}
/* 書き直したものが↓ */
BOOL flag1 = hogehogeFunction();
flag2 = flag1 ^ flag2;
flag1 = flag1 || flag2;
使い方ヒント: 「これは臭う」という行を見付けたら、各行の
をクリックしてマーキングしておきましょう(要Twitter OAuth認証)
コメント(12)
書き直し後のコードは果たして「パッと見て分かる」んだろうか。
可読性やら保守性の観点で言うなら、元コードの方がナンボかマシに見えてしまうなぁ。確かに汚いけど、設計書をそのままコードに落としたんだろうなあ、というニュアンスは伝わってくるので、まだ読む気になる。
少なくとも、書き直し後のコードは投稿者さん自身が書き間違えてるくらいだから、ね。。
>keiichirohさん、InternalServerEさん
テストはしていませんし、仰る通り保守性のことを考えたら手を加えるとまずい気がしたので、実際のコードでは弄ってはいません。
一見後者だとシンプルには思ったのですが、結局どっちをとっても「パッと見ても分からなかった」ですし、後者はコメントが足しにくいと感じたことも事実です。
あと誤植に関して、ご指摘ありがとうございます。恥ずかしながら、修正しました。
BOOL型と言っているのが実はint型のdefineで、関数の戻り値がTRUE,FALSE以外の何かになることがあったりしたらウンコだなと思いました。
第三案。これがどんな要件や仕様を満たそうとしているのかともかく、やってることは単刀直入に伝わるかと。
1つの変数を複数の用途に使いまわすこともなくなるし。
BOOL flag1;
if (hogehogeFunction()) {
flag1 = TRUE; //常に真
flag2 = !flag2; //真偽を逆転
} else {
flag1 = flag2; //flag2の値を代入
}
ifで切って「手を付けない」のが複雑にしてる気が。hogehogeFunctionの中や、このコード以前の部分でのflag2の扱いまで踏み込まないとリファクタリングはできないような。
書き換えの方が網羅が容易になるかも…と思いました。
C++ならboolにしたい
自分の場合書き直し後のほうがいいと思う。コメントは必須だけどね。
真は0以外
ビット演算子と論理演算子を混ぜないのが好みです。
コメント投稿には、twitter認証が必要です。
Twitter認証
書き直しはテストしたのかと。