少し前に読んだ「情熱プログラマ」では、プログラマの必須能力のひとつとして 作文技術
を挙げており、とても納得しました。
優れたプログラムは、ムダが少ないためにわかりやすく、そのうえ見た目にも美しいものです。
これに対し、良くないコードは読みにくく、把握が困難なために見ているだけでイライラしてきます。
そして、コードの良さはコメントひとつにもあらわれるものです。
今回はアンチパターンとして私が「 ないわー 」と思ったコメントの付け方をピックアップしてみます。
ノイズになっているコメント
良くないコメントの 9割 はノイズである
by Atuweb
コードをそのまま自然言語に直しただけのコメント
1// Bad: コードをそのまま自然言語に直しただけのコメント
2var $_item_id;//アイテムID
3var $_loginid;//ログインID
4var $_pass;//パスワード
こういったものは コードを自然言語に直しただけのコメント という言い方をされますね。
これが本当に多いこと多いこと。
上のコードは、コメントをすべて削除しても問題ありません。
1// Good: 好きなコード
2var $_item_id;
3var $_loginid;
4var $_pass;
コメントをなくしたほうが、ムダな記述がなくなることでコードがすっきりします。
これが コードのノイズ
です。
処理や変数名を日本語で書き直したからといって、コードに対する理解が深まるわけでは ありません。
1// Bad: 見苦しいコメントを端に追いやった例
2public static function getList($top_flg = false, $type = false, $os_type = self::OS_TYPE_ALL) {
3 global $db; // DB接続管理
4 global $sess; // Session管理
5 global $memc; // memcache管理
6 :
7}
このように、画面の端に追いやってもダメです。
こちらのほうが視界の端にチラチラ見えてしまうことで余計にストレスを感じます。
if 文にノイズがついているケースも非常に多いですね。
1// Bad: ifの処理を説明しただけのコメント
2// もしも月初だったら
3if ($date->day() == 1) {
4 :
5}
だったら?、だったら何?
せっかくコメントをつけるなら、なぜ、なにを、どうする を記録するようにしましょう。
1// Good: if文を上位から説明
2// 月初だけカウンタをリセットする処理を呼び出す
3// これは月間稼働数をKPI参照するため
4if ($date->day() == 1) {
5 :
6}
処理を逐一説明するコメント
1// Bad: 処理を逐一説明するコメント
2function grantReward($user_id, $reward_id)
3{
4 // 日付取得
5 $obj_today = new MyDateTime();
6 $today = $obj_today->ymdhis();
7
8 // ロック処理
9 DbUsserAccssor::getInstance()->lock($user_id);
10 // ユーザDBトランザクション開始
11 UserOrm::beginTransaction($user_id);
12
13 try
14 {
15 // 報酬データを検索する
16 $user_reward_box_data = UserPresentOrm::getUserRewardList($user_id, $reward_id);
17 // 報酬があれば
18 if (empty($user_reward_box_data) === false)
19 {
20 // 報酬を付与する
21 PresentService::grant($user_id, $user_reward_box_data);
22 // 報酬ボックスの該当データを削除する
23 UserPresentOrm::getInstance($user_id)->delete($user_id,$user_reward_box_data['id']);
24
25 // 履歴データ生成
26 $value_columns = array(
27 'user_id' => $user_id,
28 'present_id' => $user_reward_box_data['data_id'],
29 );
30 // 履歴を挿入する
31 $result = UserPresentHistoryOrm::getInstance($user_id)->insert($value_columns);
32 }
33 }
34 catch(Exception $e)
35 {
36 throw $e;
37 }
38 // ユーザDBトランザクション終了
39 UserOrm::commit($user_id);
40 // ロック処理
41 DbUsserAccssor::getInstance()->unlock();
42}
「実装サンプルにそのまま足していった」という感じのコードですね。
中身のあるコメントではなく、全体的にムダが多いです。
ムダが多ければそれだけコードの理解に時間がかかってしまいます。
コメントとしては、一言「リクエストされた報酬を付与し、データを履歴に移す」といった記述があれば十分ではないでしょうか。
このように「数を減らし、ちょっと上の視点で書く」とムダが減り、コードの理解を促すコメントにすることができると思います。
同時に「処理が推測できる変数名、関数名」をつけるようにするともっと良いですね。
(趣旨から外れるため、変数名、関数名については別の機会に。)
目立たせるために壮大なゴミを生み出しているコメント
処理を把握しやすくするためにブロッキングすることはコードの手すりとなりうる良いプラクティスです。
しかし、コメントで画面に仕切りを作るのは、残念ながら目を痛くするだけのゴミであることがほとんどです。
1// Bad: 目立たせるために壮大なゴミを生み出しているコメント
2public static function getActivedHash() {
3 //------------------------------------------------------------------------------------------
4 // memcacheまたはDBからマスタデータを全件数取得する
5 //------------------------------------------------------------------------------------------
6 $manages = self::getAllControlCache();
7 // マスタデータが取得できない場合はエラー
8 if ($manages === false) {
9 return false;
10 }
11 //------------------------------------------------------------------------------------------
12 // 有効期限をチェックし該当データを返却する
13 //------------------------------------------------------------------------------------------
14 $results = array();
15 foreach ($manages as $manage) {
16 // 有効期限チェック
17 if ($manage['disp_flag'] == 1 &&
18 (new DateTimeEx($manage["pre_start_at"]))->timestamp() <= (new DateTimeEx())->timestamp() )
19 {
20 $results[$manage["event_id"]] = $manage;
21 }
22 }
23 return $results;
24}
この小さな関数で、こんなに強烈なコメントが必要でしょうか。
ここまで書かなければ字が見えないのなら、エディターのフォントサイズを大きくすれば良いと教えてあげてください。
仕切りのためにコメントを入れるのは、そもそもクラスや関数の サイズが大きくなりすぎてしまっている ため、ということがほとんどです。
クラス分けに失敗している場合も、このような仕切りが入れてごまかすケースが多いように感じます。
パッと見て把握しやすいボリュームに分割して、役割分担を明確にしましょう。
大量のコメントアウト
コメントの中に必要な処理が紛れてしまっているケース
1// Bad: コメントの中に必要な処理が紛れてしまっているケース
2// original-transaction-id抽出
3//$target_string = '"original-transaction-id"';
4//$original_transaction_id = ProductManager::getExtractionPlistData($purchaseinfo, $target_string);
5$original_transaction_id = time();//TODO
6/*
7 // product-id抽出
8$target_string = '"product-id"';
9$product_id = ProductManager::getExtractionPlistData($purchaseinfo, $target_string);
10
11// original-purchase-date抽出
12$target_string = '"original-purchase-date"';
13$original_purchase_date = ProductManager::getExtractionPlistData($purchaseinfo, $target_string);
14
15if(empty($original_transaction_id) || empty($product_id) || empty($original_purchase_date))
16{
17//$string = 'code:'.$failed_code_other; // 不正なレシート
18//
19//print XmlTagLogger::getCryptZipText($string);
20//exit;
21}
22*/
23// TODO デバッグ設定
24//$product_id='apple_product_code.9';//TODO:
実装中に試行錯誤したものがそのままデプロイされてしまったようなコードですね。
シンタックスハイライトしなければ、何が必要で、何が不要なのか読み解くのは苦難です !
いつか使うかもしれないというコメント
改修によって使用しなくなったコードを いつか復活させるかもしれない と考えて残してしまうパターンも多いです。
1// Bad: いつか使うかもしれないというコメント
2while ($db->next(array('guild_name'))) {
3 $data = $db->getArray();
4 if ($data['status'] == '1') {
5 // ↓プラットフォームAPI対応
6 /*if ($data['guild_name_id'] != '') {
7 if (($text_data = AppModule::getTextDataName($data['guild_name_id'])) !== false) {
8 $data['guild_name'] = $text_data;
9 }
10 }
11 if ($data['description_id'] != '') {
12 if (($text_data = AppModule::getTextDataProfile($data['description_id'])) !== false) {
13 $data['description'] = $text_data;
14 }
15 }*/
16 // ↑プラットフォームAPI対応
17 self::$cache[$data['guild_id']] = array('guild_name'=>$data['guild_name'], 'description_id'=>$data['description_id']);
18 }
19}
この行為は「いつか使うだろうと、大量に紙袋を取っておくおかん 」と一緒です。
コードなら物理スペースを消費しないのでOKだろうという考え方は No Good です。
リアルとと同じで、「いつか使うだろう」の いつかが来ることはありません 。
あなたが忘れても、バージョン管理システムは覚えてくれていますから、不要になったものは即消しましょう。
あいまいな表現のコメント
真実を伝えないコメントは有害である
by Atuweb
コメントが「 ? 」
1// Bad: あいまいな表現のコメント
2/**
3 * ?
4 * @param int @id
5 * @return void
6 */
7public void fuga(Integer id) {
8 :
9}
これは本当にあったコードですが、コメントが ?
しかありませんでした。
見るたび不安になるコードですね。
きっと後から実装者と別の人が解読に試みたのでしょうが、善戦むなしくタイムオーバーになったのでしょう。
情報が不足しているコメント
1// Bad: 情報が不足しているコメント
2$original_transaction_id = time();//TODO
TODO
が残っていますが、ここから 「何をどうする」ということを一切読み取ることができません 。
そのため、不完全な実装という印象だけが残ってしまいますね。
メンテナンスされていないコメント
クラスや関数のコメントが実際の挙動と相違していることがよくあります。
1// Bad: メンテナンスされておらず、コメントが実際の挙動と相違しているコメント
2/**
3 * 報酬BOXの報酬を受け取る
4 *
5 * @param int $trn_user_id ユーザーID
6 * @param int $_reward_id 報酬ID
7 */
8public static function getReward($trn_user_id,$user_reward_box_data,$today)
9{
10 :
11}
引数と @param が一致していませんね。
コメントを見ても引数が分からないため、そのままではこの関数を使うことができません。
引数の相違ならまだ良いほうで、返り値や実際の挙動に相違がある場合は、 不具合に発展 してしまうことがあります。
クラスや関数のコメントを折りたたんだまま気付かず、コメントだけ変更が漏れてしまうのでしょう。
コメントの見直しもしっかり行いましょう。
良いコメントをを書くために
コードは 3 回、書き直せ
新人プログラマさんはぜひ「自己レビュー」を行い、3 回はコードを書き直してみましょう。
自分のコードを壊し書き直しすという スクラッチ & ビルド
をひたすら繰り返すことでレベルアップしますよ。
アンチパターンコメントの特徴
最後に CodeIQ さんのコーディング記事から、アンチパターンコメントの特徴を引用いたします。
量の公理に違反: コメントを書きすぎている/コメントが説明不足
質の公理に違反: コメントが間違っている
関係性の公理に違反: コードと関係のないコメント
様式の公理に違反: ストレートではなく、まわりくどい、曖昧なコメントコメントの9割は無駄!~アンチプラクティスから学ぶ洗練されたコメントの書き方~ #code #コード
//codeiq.jp/magazine/2013/12/3700/
現在は閉鎖 ?
コメントはコードに書ききれないWhyを残すのがベター言われます。
同時に 変数名、関数名が適切かどうか ということも、しっかり考えてコードを書きましょう。
おわりに
時間があっても良いプログラムが書けるわけではありません。
限りある時間の中で、適切なコードを書き進めていくことは難しいです。
しかし、私が書いたコードと、ベテランプログラマが書いたコードは明らかに異なります。
この差は、ひとえに 良いコードを書こう という気持ちから生まれてくるものではないでしょうか。
良いコードは一日にしてならず です。
チームメンバーに「なんも考えてないわー」と言われてしまわないように、良いコードを書きましょう。