この記事は、インテル® デベロッパー・ゾーンに公開されている「The Ultimate Question of Programming, Refactoring, and Everything」の日本語参考訳です。
16. 能力の誇示はプログラミングでは容認されない
前述の「11. 1 行にできるだけ多くの操作を詰め込もうとしない」に似ていますが、ここでは別の問題に注目したいと思います。ときどき、自分の能力を誇示するためできるだけ短いコードを記述しようとするプログラマーがいます。
複雑なテンプレートについて話しているのではありません。複雑なテンプレートは、良い場合も悪い場合もあり、その線引きが難しいため、全く別の話です。私が言いたいのは、C と C++ プログラマーのどちらにも当てはまるもっと単純な状況です。「自分ができることをひけらかすため」構造をより複雑にすることです。
KDE*4 プロジェクトから抜粋した以下のコードについて考えてみます。このエラーは、次の PVS-Studio 診断によって検出されます。
V593 Consider reviewing the expression of the ‘A = B == C’ kind. The expression is calculated as following: ‘A = (B == C)’. (V593 ‘A = B == C’ のような表現を再確認してください。このような表現は、’A = (B == C)’ として計算されます。)
void LDAPProtocol::del( const KUrl &_url, bool )
{
....
if ( (id = mOp.del( usrc.dn() ) == -1) ) {
LDAPErr();
return;
}
ret = mOp.waitForResult( id, -1 );
....
}
説明
このコードを見ると、私はいつも次のような疑問を持ちます: どうしてこんなことをするのだろう? 行数を抑えたいのか? 複数の操作を 1 つの表現にまとめられることを見せびらかしたいのか?
このようなコードは、”if (A = Foo() == Error)” のような典型的なエラーパターン (http://www.viva64.com/en/examples/V593/) になります。
比較操作は、代入操作よりも優先されます。そのため、”mOp.del( usrc.dn() ) == -1″ が先に実行され、その後 true (1) または false (0) 値が id 変数に代入されます。
mOp.del() が ‘-1′ を返す場合、関数は終了します。そうでない場合、関数は実行を継続し、’id’ 変数に不正な値が代入されます。つまり、常に 0 になります。
正しいコード
括弧を追加しても問題の解決にはなりません。エラーは排除されますが、これは正しい解決法ではありません。
コードには複数の括弧があります。よく見てください。それぞれの意味を理解するのは困難です。プログラマーは、コンパイラーによる警告を排除したかったのかもしれません。あるいは、操作が正しい優先順位で行われるようにしようとし失敗したのかもしれません。どちらにせよ、これらの括弧は役に立っていません。
このコード例には、より深刻な問題が潜んでいます。可能な場合は、コードを複雑にすべきではありません。次のように記述したほうが良いでしょう。
id = mOp.del(usrc.dn());
if ( id == -1 ) {
推奨事項
面倒でも追加のコード行を記述すべきです。複雑な表現は読むのが困難です。先に代入を行い、その後比較を行います。そうすることで、後でコードを保守するプログラマーの作業を容易にし、ミスの可能性を減らすことができます。
つまり、能力を誇示しようとすべきではない、ということです。
些細なことですが、このヒントが皆さんの役に立つことを願っています。常に明確で簡潔なコードのほうが、能力を誇示するようなコードよりも推奨されます。
コンパイラーの最適化に関する詳細は、最適化に関する注意事項を参照してください。
