ハッカーやクラッカー達を描いた漫画『王様達のヴァイキング』がめちゃくちゃ面白いです。ITエンジニアであれば所々に共感する所があってハマると思います。
例えば「汚いソースコード」が出てくるシーンがあります。 主人公はATMへのクラッキングの調査をするため、ATMのシステムを作っている会社に赴きます。そこでソースコードを見ます。
王様達のヴァイキング 4巻 {CODE 6:CONVERSATION PIECE} #3
世のITエンジニア達は「うへぇ」ってなっているシーンだと思います。おそらく言語はVisual C++です。そして、作者は現場をすごく分かってるなーっと思う「汚いソースコード」です。
このソースコードのどこが汚いのか…面白そうだったので問題と思っている箇所をまとめました。IT業界初心者に捧げたいので、分かりやすさをモットーにツッコミまくりました。
問題のソースコード
画像のソースコードをテキストに落としました。想像も一部含んでいます。
/* * 引数３：kouza_bangou * 口座番号 * 引数４：anshou_bangou * 暗証番号 * * 残高を銀行とやりとりする。 * 成功した場合、TRUE を返す。 * 失敗した場合、グローバル変数 error_koudo に、エラーコードを代入してFALSE を返す。 */ BOOL TANAKA_0312() { int a, b, c, d, e, f, g, h, i, j; int buffer[1024]; int ginkou_error_koudo; SSL_CTX ssl_ctx; TANAKA_0109(&ssl_ctx); /* 接続。 */ if ( !YAMADA_0102(&ssl_ctx, XXX)) { error_koudo = ERROR_SETS; return FALSE; } /* 何でか接続できたはずなXXXXXる。 そのための対策 */ if ( !TANAKA_0309(&ssl_ctx, XXX)) { error_koudo = ERROR_SETS; return FALSE; } /* 2007/01/04 田中：大事なログなので勝手に直すな。 */ /* /* 2006/12/25 山田：この printf があるとなぜか動かなくなる */ */ }
ソースコードの特徴
過去にこんな記事を書きました。
これに照らし合わせると、このコードはインフラ系システムに多い典型的な「品質重視のシステム」の汚いコードです。独自の製造・開発ルールでソースコードを読みにくくする特徴があります。
コメント欄（関数の説明）
/* */で囲った部分がコメントのブロックです。最初のコメントは関数「BOOL TANAKA_0312()」の説明が書かれています。
引数の説明
* 引数３：kouza_bangou * 口座番号 * 引数４：anshou_bangou * 暗証番号
変数名が日本語って…の議論がありますが、まぁ命名規則（スネークケース）に従っているのでよしとします。しかしお気づきだろうか、この関数に引数が定義されていないことに…
単にコメントの修正忘れであって欲しいですが、もし引数を使う関数であればいったいどうやって引数を渡しているのだろうか。いくつか考えられますが、可能性が大きいのはグローバル変数でしょう。
グローバル変数は（有効なスコープの中で）どんな関数からでも参照可能な変数です。つまり、グローバル変数の値を更新するということは、それを利用する他の関数に影響を与える可能性があります。そんなわけで、関数に値を渡す場合は、よっぽどのことでない限り、引数経由で渡した方が良いです。
残高を銀行とやりとりする
「やりとり」とは何だろう？銀行に対して残高を取得する処理だろうか？それとも残高を更新する処理だろうか？処理説明のコメントが曖昧過ぎます。
コメントを見ただけでは何をするのか分からないため、ソースコードを追跡するはめになります。また、関数の処理（責務）が曖昧なために、追加改修する場合はこの関数が肥大化していくと予想されます。コメントの曖昧さは出来るだけ排除しよう。
戻り値のコメント
* 成功した場合、TRUE を返す。
* 失敗した場合、グローバル変数 error_koudo に、エラーコードを代入してFALSE を返す。
グローバル変数 error_koudo …この変数名を書いたやつ、および、これをそのままにしておいたチームメンバーにグーパンしたいですよね。コードをcodeではなくkoudoと書くところに気持ち悪さを感じます。
「変数に英語と日本語を混ぜるのはありか？」の論争があります。個人的には、日本語を英語にした時に長くなって読みにくくなるとか、そのままの日本語の方が意味が通じるなどの理由があれば、日本語を使っても良いと思います。しかしkoudoの場合はcodeより長いですし、そして意味が通じないです。（error_という修飾子が付いてやっとcodeの意味だと分かります）
そして、エラーコードもグローバル変数で渡しているんですよね…呼び出し元でエラーハンドリングするのであれば、例外をスローするとか、戻り値は構造体にして複数の項目を返すようにするとかしたいです。
関数定義
BOOL TANAKA_0312() { … } の部分です。
関数名 TANAKA_0312
この関数名だけを見て、この関数が何をするか分かるでしょうか？分かるとしたらそれはコードに関わった人か神です。おそらく開発管理の関係で「関数名=開発担当者_連番」のような独自ルールがあったのでしょう。。
関数名だけでこの関数が何をするのかが分かるとコードを読むのが早くなります。「TANAKA_0312」と書かれると、処理内容を把握するためにはコメントを読んだり、または処理内容を追いかけたり、あるいは「関数一覧表」みたいなExcelで管理している資料と突き合わせることが必要になってきます。
仮にこの処理の内容が「銀行に問い合わせて残高（Account balance）を取得する」のであれば関数名は「get_account_balance」とするのが分かりやすいと思います。「どこに」に相当する（銀行などの）情報は引数の定義で分かります。
ローカル変数の宣言
int a, b, c, d, e, f, g, h, i, j;
絶対使ってない変数あるよこれ…おそらくコピペしてきたコードです。使っていない変数は混乱の元なので削除すべきです。
そしてもし全ての変数を使っているとしたら、それはそれで恐ろしいです。変数名を見て、その変数が何を意味しているかパッと見て分からないためです。aに口座番号を入れて、bに金額を入れて…というコード、読みたくないです。
int buffer[1024];
「なんのバッファだよ」という指摘ももちろんありますが、サイズに1024という定数が指定されているのも注目したいところです。
バッファサイズが変更になった時、ソースコードでバッファを使っている箇所を探して修正する必要があります。こういったシステムの要件によって変わりうる値はグローバル変数、環境変数、プロパティなどで持たせるべきです。そうすれば１箇所修正するだけで良くなります。
バッファがその関数内でしか使わないのであれば、コメントに1024の根拠を残してもらえれば、コメントしだいで納得はできます。
int ginkou_error_koudo;
和名_英名_意味不明。
SSL_CTX ssl_ctx;
これはこのままでも良いと思います。
メイン処理
TANAKA_0109(&ssl_ctx);
何をやっているかわからない関数を呼び出しています。おそらくssl_ctxを初期化する関数でしょう。
/* 接続。 */ if ( !YAMADA_0102(&ssl_ctx, XXX)) { error_koudo = ERROR_SETS; return FALSE; }
YAMADA_0102がおそらく銀行と接続し、そのコネクション（接続状態）をssl_ctxに保持する関数と推測できます。んー…と思いますが、エラーハンドリングにはいろんな思想があるのでここでは触れないようにします。
また、コードをよく見ると、return FALSE; の後ろに空白文字が書かれています。ソースコードを整形する運用がされていない気がします。整形は、必須ではないですけど、あれば書き方に統一が出ます。
例えば「{」を最後につけるか改行してつけるか、開発者によって好みがあります。インデントをTabにするか半角スペースにするかも好みがあります。整形の運用がないと、各々の開発者が好き勝手な書き方をします。
/* 何でか接続できたはずなXXXXXる。 そのための対策 */ if ( !TANAKA_0309(&ssl_ctx, XXX)) { error_koudo = ERROR_SETS; return FALSE; }
「何でか接続できたはず…」という怖い文章がありますね。こういう注意書きをコメントに残すのは大いに賛成です。問題点がすぐ分かりますし、バグ改修の時にも役立ちます。
/* 2007/01/04 田中：大事なログなので勝手に直すな。 */ /* /* 2006/12/25 山田：この printf があるとなぜか動かなくなる */ */
修正履歴をソースコードに書くという運用をしているプロジェクトもあります。こういったコメントは、開発者個人の裁量で消せないため、結果として改修が入るたびにコードが肥大化していきます。書かない選択肢があるのであれば書かないのが良いです。
まとめ
典型的な「汚いソースコード」を指摘しまくりました。こういうコードを世の中から無くしていきたいという想いがあったためか、思ったよりも長文になりました。製造・開発現場で少しでも参考になればと思います。
