少し前に読んだ「情熱プログラマ」では、プログラマの必須能力の一つとして作文技術を挙げており、とても納得しました。

優れたプログラムは、無駄が少ないためにわかりやすく、そして見た目にも美しいものです。
これに対し、良くないコードは読みにくく、把握が困難なために見ているだけでイライラしてきます。

そして、コードの良さはコメント一つからでも分かるものです。

今回はアンチパターンとして私が「 ないわー 」と思ったコメントの付け方をピックアップしてみます。

ノイズになっているコメント

良くないコメントの9割はノイズである
tomita@atuweb

コードをそのまま自然言語に直しただけのコメント

// Bad: コードをそのまま自然言語に直しただけのコメント
<?php
var $_item_id;//アイテムID
var $_loginid;//ログインID
var $_pass;//パスワード

こういったものはコードを自然言語に直しただけのコメント という言い方をされますね。
これが本当に多いこと多いこと。

上のコードは、コメントをすべて削除しても問題ありません。

// Good: 好きなコード
<?php
var $_item_id;
var $_loginid;
var $_pass;

コメントをなくしたほうが、無駄な記述がなくなることでコードがすっきりします。
これがコードのノイズです。

処理や変数名を日本語で書き直したからといって、コードに対する理解が深まるわけでは ありません。

// Bad: 見苦しいコメントを端に追いやった例
<?php
public static function getList($top_flg = false, $type = false, $os_type = self::OS_TYPE_ALL) {
    global $db;                                         // DB接続管理
    global $sess;                                       // Session管理
    global $memc;                                       // memcache管理

このように、画面の端に追いやってもダメです。
こちらのほうが視界の端にチラチラ見えてしまうことで余計にストレスを感じます。

if文にノイズがついているケースも非常に多いですね。

// Bad: ifの処理を説明しただけのコメント
<?php
// もしも月初だったら
if ($date->day() == 1) {
    :

だったら?、だったら何?

せっかくコメントをつけるなら、なぜ、なにを、どうする を記録するようにしましょう。

// Good: if文を上位から説明
<?php
// 月初だけカウンタをリセットする処理を呼び出す
// これは月間稼働数をKPI参照するため
if ($date->day() == 1) {
    :

処理を逐一説明するコメント

// Bad: 処理を逐一説明するコメント
<?php
function grantReward($user_id, $reward_id)
{
    // 日付取得
    $obj_today = new MyDateTime();
    $today = $obj_today->ymdhis();

    // ロック処理
    DbUsserAccssor::getInstance()->lock($user_id);
    // ユーザDBトランザクション開始
    UserOrm::beginTransaction($user_id);

    try
    {
        // 報酬データを検索する
        $user_reward_box_data = UserPresentOrm::getUserRewardList($user_id, $reward_id);
        // 報酬があれば
        if (empty($user_reward_box_data) === false)
        {
            // 報酬を付与する
            PresentService::grant($user_id, $user_reward_box_data);
            // 報酬ボックスの該当データを削除する
            UserPresentOrm::getInstance($user_id)->delete($user_id,$user_reward_box_data['id']);

            // 履歴データ生成
            $value_columns = array(
                'user_id' => $user_id,
                'present_id' => $user_reward_box_data['data_id'],
            );
            // 履歴を挿入する
            $result = UserPresentHistoryOrm::getInstance($user_id)->insert($value_columns);
        }
    }
    catch(Exception $e)
    {
        throw $e;
    }
    // ユーザDBトランザクション終了
    UserOrm::commit($user_id);
    // ロック処理
    DbUsserAccssor::getInstance()->unlock();
}

「実装サンプルにそのまま足していった」という感じのコードですね。

意味のあるコメントが全くなく、全体的に無駄も多いです。
無駄が多ければそれだけコードの理解に時間がかかってしまいます。

コメントとしては、一言「リクエストされた報酬を付与し、データを履歴に移す」といった記述があれば十分ではないでしょうか。

このように「数を減らし、ちょっと上の視点で書く」と無駄が減り、コードの理解を促すコメントにすることができると思います。
同時に「処理が推測できる変数名、関数名」をつけるようにするともっと良いですね。
(趣旨から外れるため、変数名、関数名については別の機会に。)

目立たせるために壮大なゴミを生み出しているコメント

処理を把握しやすくするためにブロッキングすることはコードの手すりとなりうる良いプラクティスです。

しかし、コメントで画面に仕切りを作るのは、残念ながら目を痛くするだけのゴミであることがほとんどです。

// Bad: 目立たせるために壮大なゴミを生み出しているコメント
<?php
public static function getActivedHash() {
    //------------------------------------------------------------------------------------------
    // memcacheまたはDBからマスタデータを全件数取得する
    //------------------------------------------------------------------------------------------
    $manages				=	self::getAllControlCache();
    // マスタデータが取得できない場合はエラー
    if ($manages === false) {
        return false;
    }
    //------------------------------------------------------------------------------------------
    // 有効期限をチェックし該当データを返却する
    //------------------------------------------------------------------------------------------
    $results				=	array();
    foreach ($manages as $manage) {
        // 有効期限チェック
        if ($manage['disp_flag'] == 1 &&
            (new DateTimeEx($manage["pre_start_at"]))->timestamp() <= (new DateTimeEx())->timestamp() )
        {
            $results[$manage["event_id"]] 		=	$manage;
        }
    }
    return $results;
}

この小さな関数で、こんなに強烈なコメントが必要でしょうか。
ここまで書かなければ字が見えないのなら、エディタのフォントサイズを大きくすれば良いと教えてあげてください。

仕切りのためにコメントを入れるのは、そもそもクラスや関数の サイズが大きくなりすぎてしまっている ため、ということがほとんどです。
クラス分けに失敗している場合も、このような仕切りが入れてごまかすケースが多いように感じます。

パッと見て把握しやすいボリュームに分割して、役割分担を明確にしましょう。

大量のコメントアウト

コメントの中に必要な処理が紛れてしまっているケース

// Bad: コメントの中に必要な処理が紛れてしまっているケース
<?php
// original-transaction-id抽出
//$target_string				= '"original-transaction-id"';
//$original_transaction_id	= ProductManager::getExtractionPlistData($purchaseinfo, $target_string);
$original_transaction_id = time();//TODO
/*
 // product-id抽出
$target_string	= '"product-id"';
$product_id		= ProductManager::getExtractionPlistData($purchaseinfo, $target_string);

// original-purchase-date抽出
$target_string			= '"original-purchase-date"';
$original_purchase_date	= ProductManager::getExtractionPlistData($purchaseinfo, $target_string);

if(empty($original_transaction_id) || empty($product_id) || empty($original_purchase_date))
{
//$string	= 'code:'.$failed_code_other;	// 不正なレシート
//
//print XmlTagLogger::getCryptZipText($string);
//exit;
}
*/
// TODO デバッグ設定
//$product_id='apple_product_code.9';//TODO:

実装中に試行錯誤したものがそのままデプロイされてしまったようなコードですね。
シンタックスハイライトしなければ、何が必要で何がそうでないか全くわかりません!

いつか使うかもしれないというコメント

改修によって使用しなくなったコードを いつか復活させるかもしれない と考えて残してしまうパターンも多いです。

// Bad: いつか使うかもしれないというコメント
<?php
while ($db->next(array('guild_name'))) {
    $data = $db->getArray();
    if ($data['status'] == '1') {
        // ↓プラットフォームAPI対応
        /*if ($data['guild_name_id'] != '') {
            if (($text_data = AppModule::getTextDataName($data['guild_name_id'])) !== false) {
                $data['guild_name'] = $text_data;
            }
        }
        if ($data['description_id'] != '') {
            if (($text_data = AppModule::getTextDataProfile($data['description_id'])) !== false) {
                $data['description'] = $text_data;
            }
        }*/
        // ↑プラットフォームAPI対応
        self::$cache[$data['guild_id']] = array('guild_name'=>$data['guild_name'], 'description_id'=>$data['description_id']);
    }

この行為は「いつか使うだろうと、大量に紙袋を取っておくおかん 」と一緒です。

コードなら物理スペースを消費しないのでOKだろうという考え方はNo Goodです。
リアルとと同じで、「いつか使うだろう」の いつかが来ることはありません

あなたが忘れても、バージョン管理システムは覚えてくれていますから、不要になったものは即消しましょう。

あいまいな表現のコメント

真実を伝えないコメントは有害である
tomita@atuweb

コメントが、?

// Bad: あいまいな表現のコメント
<?php
/**
 * ?
 *  @param int @id
 *  @return void
 */
public void fuga(Integer id) {
    :

これは本当にあったコードですが、コメントが?しかありませんでした。
見るたびに不安になるコードですね。

きっと後から実装者と別の人が解読に試みたのでしょうが、善戦むなしくタイムオーバーになったのでしょう。

情報が不足しているコメント

// Bad: 情報が不足しているコメント
<?php
$original_transaction_id = time();//TODO

TODOが残っていますが、ここから 「何をどうする」ということを一切読み取ることができません 。 そのため、不完全な実装という印象だけが残ってしまいますね。

メンテナンスされていないコメント

クラスや関数のコメントが実際の挙動と相違していることがよくあります。

// Bad: メンテナンスされておらず、コメントが実際の挙動と相違しているコメント
<?php
/**
 * 報酬BOXの報酬を受け取る
 *
 * @param int $trn_user_id ユーザーID
 * @param int $_reward_id 報酬ID
 */
public static function getReward($trn_user_id,$user_reward_box_data,$today)
{
    :

引数と@paramが一致していませんね。
コメントを見ても引数が分からないため、そのままではこの関数を使うことができません。

引数の相違ならまだ良いほうで、返り値や実際の挙動に相違がある場合は、 不具合に発展 してしまうことがあります。

クラスや関数のコメントを折りたたんだまま気付かず、コメントだけ変更が漏れてしまうのでしょう。
コメントの見直しもしっかり行いましょう。

良いコメントをを書くために

コードは3回、書き直せ

何かのビジネス書で「 新入社員の報告書は、読まずに3回突き返す 」というものがありました。

その理由は「趣旨が明確になっていなかったり、結論が弱かったりと改善の余地が多分にあるため」だそうです。
2回、3回とNGを出すことによって、「何をどうすればよいか」を 自分の頭で考えて改善していく ことを促すことにつながるのですね。

プログラムも同じです。
「とりあえず動けばOK」という気持ちで書かれたコードはまるで独り言を書きなぐったようであり、後から見直すととても理解できたものではありません。

さらっとコードを書いた後には3回程度見直したほうが、間違いなくよいコードになります。

「せっかく書いたコード」かもしれませんが、自分のコードを壊し書き直しすというスクラッチ&ビルドをひたすら繰り返すことでレベルアップしていきますよ。

アンチパターンコメントの特徴

最後にCodeIQさんのコーディング記事から、アンチパターンコメントの特徴を引用いたします。

量の公理に違反: コメントを書きすぎている/コメントが説明不足
質の公理に違反: コメントが間違っている
関係性の公理に違反: コードと関係のないコメント
様式の公理に違反: ストレートではなく、まわりくどい、曖昧なコメント

コメントの9割は無駄!~アンチプラクティスから学ぶ洗練されたコメントの書き方~ #code #コード https://codeiq.jp/magazine/2013/12/3700/

コメントはコードに書ききれないWhyを残すのがベター言われます。

同時に 変数名、関数名が適切かどうか ということも、しっかり考えてコードを書きましょう。

おわりに

時間があっても良いプログラムが書けるわけではありません。
限りある時間の中で、適切なコードを書き進めていくことは難しいです。

しかし、私が書いたコードと、ベテランプログラマが書いたコードは明らかに異なります。
この差は、ひとえに 良いコードを書こう という気持ちから生まれてくるものではないでしょうか。

良いコードは一日にしてならず です。

チームメンバに「なんも考えてないわー」と言われてしまわないように、良いコードを書きましょう。

この記事はtomita@atuwebがお届けしました。



2016年07月25日:タイトルを『[プログラミング]そのコメントの付け方は間違っている』から変更
2016年05月10日:SNSでの指摘を受けて、一部記事を修正しました。