C# と VB.NET の質問掲示板

ASP.NET、C++/CLI、Java 何でもどうぞ

C# と VB.NET の入門サイト

Re[4]: コメントのいい書き方について

分類:[C#]　

開発環境
Windows7 Home
使用言語
2010 C# Professional

morimoriです。
いつもソースでコメントを書いているのですが、
どうしてもクラス名やメソッド名などを見たままのコメントを書いてしまいます。

このようなコメントはあまり意味がないと聞きますが、
どのように書けばいいコメントになるのでしょうか。
それでは失礼します。

補足：
※特にコメントの中でもクラスやメソッドにつけるドキュメンテーションコメントに
　皆様の回答をお聞きしたいです。

■No68124 (morimori さん) に返信
あくまでも自分流の場合ですが.....
　コメントってそれが何であるかを自分または他人が見たときに理解しやすくするためのものだと思いますので、動作内容を箇条書きしています。
　//----------二重起動禁止
　//----------データベースデータ取得
などのようにしています。

コメントの書き方ってのは「議論のテーマ」としてはたぶん永遠に続く難しい代物だ。

コメント＝マニュアル　という立場では、
C# の場合はまず XML Documentation コメントを書くことから始めるべきだろう。
他の言語であれば javadoc であるとか doxygen とか。

マニュアルにしないコメントの書き方なら、俺とほぼ同意見なので紹介しておこう
http://ufcpp.net/study/csharp/st_comment.html

・コード（コメントでない部分）を見れば「何をしているか」はわかる（はず）
　わからないようなコードを書いているということは、技量が低いということ。
・コメントを入れたくなる＝リファクタリングの対象としていい、ってこと。
・コメントでないと書けないのは Why であるから「なぜ」を書くべし。
　なぜの中には設計意図（実装上の注意）を含む（＝マニュアルを書け）

んで、聞きたいのはドキュメンテーションコメントってことだったから

俺は C# 本業ぢゃないので C/C++ でのコメント例になるが

char* strcat(char* dst, const char* src); にドキュメンテーションコメントをつけるなら
/// @param [in, out] dst : 結合先文字列（領域は呼び出し側が用意する）
/// @param [in] src : 結合する文字列（この文字列が後ろに付加される）（領域は呼び出し側が用意する）
/// @warning dst には「元の文字列」に「結合する文字列」を付加するに十分なサイズが必要。
/// 十分なサイズとは strlen(dst) + strlen(src) + 1 バイト以上のことである。
/// 領域サイズが不足すると、メモリを破壊する。

char* strdup(const char* src); にドキュメンテーションコメントをつけるなら
/// @param [in] src : 元文字列（領域は呼び出し側が用意する）
/// @retval nullptr のとき、失敗（空きメモリが不足など）
/// @retval nullptr でないとき、複写した文字列（領域は strdup が malloc を使って確保する）
/// @note nullptr でない返却値は、領域が不要になったら free() に渡して処分すること。
/// @warning 処分を忘れるとメモリリークを起こす。
/// @note nullptr を返却したときに限り errno が ENOMEM に更新される。

自分が作った関数やクラスを、他人に使ってもらうための使い方マニュアルを書くといい。
よく言われるが、１年後の自分は他人であるわけで・・・
・使う前提条件
・使い方の注意事項
・使い終わったらどうなるか
・内部で発生する副作用
などなど。

C# だと引数の修飾子 out や ref が必須で、呼び出しの際の注意事項がソース自体で表現できてしまうので
「呼び出し側が用意する」なんてコメントは書かなくてよくなっている。
manage 領域では「処分」の話も書かなくていいし、そういう意味ではコメントは最小限でいいかもしれない。

お二人ともありがとうございます。
イメージをつかむために、
お二方のドキュメンテーションコメント
をつけました。このようなものでいかがでしょうか。

/// <summary>
/// メモリマップドファイルを作成します。
/// ①空のメモリマップドファイルを作成
/// ②メンバのMemoryMapedAccsessインターフェースが設定されている場合、保存する
/// ③作成に失敗した場合は保存エラーを表示する
/// </summary>
/// <param name="strFileName">メモリマップドファイル名</param>
/// <param name="lnMaxCapacity">容量</param>
/// <returns>true:作成完了, false 作成失敗</returns>
public bool CreateMemoryMapedFile(string strFileName, long lnMaxCapacity)
{
try
{
MemoryMappedFile mmf = MemoryMappedFile.CreateOrOpen(strFileName, lnMaxCapacity);
using (var accessor = mmf.CreateViewAccessor())
{
if (MemoryMapedAccsess != null)
{
MemoryMapedAccsess.SaveObject(accessor);
MemoryMappedFileSave = mmf;
}
}
}
catch (Exception exp)
{
ErrorMessage = strFileName + ERROR_SAVE_MESSAGE;
TraceMessage = exp.ToString();

return false;
}
return true;
}

良い悪いは各読者の主観によるものであろうから言及しないが・・・

提供された Documentation で読者が満足するだろうか？を自分自身に問いかけるといい。
その関数・クラスを使うユーザーが読むであろうマニュアルを書いている＝
その関数・クラスを使うユーザがわざわざソースコードの詳細を調べなくていいようにすべきだ。
という観点からは内容が不足していると思う。

・既に同じ名前のメモリマップドファイルがあるとき、どう挙動するのか？
・容量って単位は何？
・ディスク装置に指定容量がない場合、どう挙動するのか？
・例外は投げられるのか、投げられないのか？
・表示する、って、何をどこに？
ってあたりはこの「ドキュメント」には書かれていない＝不明事項である。

この関数の作者は、この関数の使用者に対して何をどこまで期待するのか？
・使用者が、中で MemoryMappedFile を使っているということは知らなくていい。
・使用者が、中で MemoryMappedFile を使っているということは知っておかないといけない。
によって、ドキュメンテーションコメントに書くべきレベルは変化するだろう。

・前者なら、そういう細かい解説まで全部書く　か
・後者なら、挙動は System.IO.MemoryMappedFiles.MemoryMappedFile.CreateOrOpen に準ずる　と書くか
そういうことになるんぢゃないかな。

回答ありがとうございます
自分が理解しているのかを知りたいがために載せてみましたが、
やはりまだ理解が足りていなかったようです。

ということで指摘があった以下の内容を盛り込んで修正してみます。（比較のためメソッド内容は同じ）
・既に同じ名前のメモリマップドファイルがあるとき、どう挙動するのか？
・容量って単位は何？
・ディスク装置に指定容量がない場合、どう挙動するのか？
・例外は投げられるのか、投げられないのか？
・表示する、って、何をどこに？

/// <summary>
/// メモリマップドファイルを作成します。すでに同じファイル名があった場合は上書きします。
/// ①空のメモリマップドファイルを作成
/// ②メンバのMemoryMapedAccsessインターフェースが設定されている場合、保存する
/// ③作成に失敗した場合、例外を出力せずにErrorMessageプロパティにエラー内容を書き込む
/// </summary>
/// <param name="strFileName">メモリマップドファイル名</param>
/// <param name="lnMaxCapacity">容量(byte)</param>
/// <returns>true:作成完了, false 作成失敗</returns>

ひとことで言うなら「目的」をコメントにする。

実際のコードが以下のようなものだと仮定します。
1) ファイルを開く
2) １行読み込む
3) 先頭が ;文字だったら読み飛ばす
4) 先頭が ;文字以外だったら文字列リストに追加
5) ファイルをクローズ
さて、この関数は何をしているでしょう？

まぁ、このぐらいの処理なら想像がつくかと思いますが、
テキストファイルからコメント行(;文字で始まるものとする)を除いて読み込み
文字列リストに格納です。

でも、コメントが上記そのままだったら、ざっとコメントを流し読みしないと
何をしているのか理解できません。

しかし、以下のようなコメントだったらどうでしょう？
・テキストファイルを読み込みます
Path(in) : テキストファイルへのパス
List(out): テキストファイルの1行づつの文字列リスト
(;文字で始まる行はコメントとみなして省く)

一目で理解できるわけです。

もう一つの例。

以下のようなコメントがあったとします。
・ソートします

いわゆる汎用的なソート関数を作るのが仕事なら、コメントはこれでいいわけ
です。

でも、通常はなんらかの目的を持ってソートしているわけです。
そして、目的はソースコードを読んだだけではわからないのです。

なので、目的をコメントにしましょう。

・商品を売り上げ価格順にソートします
・メールを差出人毎にソートします
・リストビューにおいてクリックされた列でソートします

など。。。

上記例なら、なんのためにメモリマップドファイルを作るの？

別にプロセス間通信したいだけなら、
方法はメモリマップドファイル以外にもいくらでもあるわけです。
1) ソケット通信
2) レジストリ
3) INIファイル
4) 専用の設定ファイル
5) パイプ
:
どれを選んでもソフトウェアの機能を満たす上では問題ないわけです。

要するに、メモリマップドファイルは手段であって目的ではない。
目的はメモリマップドファイルを使ってどんなデータをやりとりするのか？
なんのためにそのデータをやりとりするのか？

そこにあるわけです。

スコール様回答ありがとうございます。

なるほど、目的ですか。
ここでの目的はコンストラクタでメンバにあるMemoryMapedAccsessインターフェースを使って
メモリマップドファイルを作成するということですから

/// <summary>
/// コンストラクタで指定したMemoryMapedAccsessインターフェースを使って
/// メモリマップドファイルを作成します。すでに同じファイル名があった場合は上書きします。
/// ①空のメモリマップドファイルを作成
/// ②メンバのMemoryMapedAccsessインターフェースが設定されている場合、保存する
/// ③作成に失敗した場合、例外を出力せずにErrorMessageプロパティにエラー内容を書き込む
/// </summary>
/// <param name="strFileName">メモリマップドファイル名</param>
/// <param name="lnMaxCapacity">容量(byte)</param>
/// <returns>true:作成完了, false 作成失敗</returns>

こうなるのでしょうか。

■No68144 (morimori さん) に返信

> /// <summary>
> /// コンストラクタで指定したMemoryMapedAccsessインターフェースを使って
> /// メモリマップドファイルを作成します。すでに同じファイル名があった場合は上書きします。
> /// ①空のメモリマップドファイルを作成
> /// ②メンバのMemoryMapedAccsessインターフェースが設定されている場合、保存する
> /// ③作成に失敗した場合、例外を出力せずにErrorMessageプロパティにエラー内容を書き込む
> /// </summary>

　summary は「要約」なので、全部は書かないかなぁ。remark というのもあるし。（remarks　だったかな？）
あと、「maped」じゃなくて「mapped」じゃない？

> /// メモリマップドファイルを作成します。すでに同じファイル名があった場合は上書きします。
> /// ②メンバのMemoryMapedAccsessインターフェースが設定されている場合、保存する

　この二つは、同じメソッドにないといけないのかな？
「作る」ことと「保存する」ことは、違いますよね？

> /// <param name="strFileName">メモリマップドファイル名</param>
> /// <param name="lnMaxCapacity">容量(byte)</param>

　システムハンガリアンは、使わないことが推奨されています。
fineName, maxCapacity で十分。
たとえば、「maxCapacityInByte」というのはアプリケーションハンガリアンになるので、こちらは ok。

> /// <returns>true:作成完了, false 作成失敗</returns>

　「保存」に失敗しても、false が返ると思います。

自分の場合、Java なら JavaDoc、.NET なら XML コメントしか書かないです。
つまりメソッドの入り口にはリファレンス的な内容だけ書き、メソッド内はまったくコメントがないです。
そもそもあれこれ何行にも渡るような説明が必要なメソッドって設計がおかしいですよ。仕様というかコード設計。

あ、一応フィールドにも書きますが、JavaDoc、XML コメントだけです。
private メンバなら、ほぼお飾りですねぇ。

だから「書き方」と言われると、もう上記の形式どおりにしか書けないってわけです。

# っていうか、774RR さんの意見とまったく同じですね。

みなさんありがとうございます。
なるほど、必要ないのならあえて書かないことも手なのですね。

＞Java なら JavaDoc、.NET なら XML コメントしか書かないです。
それをドキュメンテーションコメントというのではないですか？

＞あれこれ何行にも渡るような説明が必要なメソッドって設計がおかしいですよ。仕様というかコード設計。
そこまでおっしゃるのでしたらその正しいコード設計をぜひ知りたいものです。
じゃんぬさんなら今回の場合どう対応されます？

■No68153 (morimori さん) に返信
> ＞あれこれ何行にも渡るような説明が必要なメソッドって設計がおかしいですよ。仕様というかコード設計。
> そこまでおっしゃるのでしたらその正しいコード設計をぜひ知りたいものです。

　Microsoft の「手本」を参考にしましょう。
msdn.microsoft.com/ja-jp/library/system.io.fileinfo.create(v=vs.100).aspx
FileInfo.Create メソッドです。

summary は、「ファイルを作成します。」だけです。
ここの例での

> すでに同じファイル名があった場合は上書きします。
> ①空のメモリマップドファイルを作成
> ②メンバのMemoryMapedAccsessインターフェースが設定されている場合、保存する
> ③作成に失敗した場合、例外を出力せずにErrorMessageプロパティにエラー内容を書き込む

に相当するものは、「解説」に書かれています。remarks かなぁ、やっぱり。
msdn.microsoft.com/ja-jp/library/3zw4z1ys(v=vs.110).aspx

/// <summary>
/// メモリマップドファイルを作成します。
/// </summary>
/// <param name="fileName">メモリマップドファイルのファイル名を指定します。</param>
/// <param name="maxCapacity">ファイルの容量を、バイト単位で指定します。</param>
/// <returns>作成が成功した場合に true、それ以外の場合は false を返します。</returns>
/// <remarks>
/// <para>すでに同じファイル名があった場合は上書きします。</para>
/// <para>コンストラクタで MemoryMappedAccsess インターフェースが指定されていた場合、
/// そのインスタンスを使用して（？？？）を書き出します。</para>
/// <para>何らかの失敗が発生した場合、ErrorMessage プロパティにメッセージを設定します。</para>
/// </remarks>

なお、.NET Framework の名前付けに関するガイドラインは、次の URL にあります。
msdn.microsoft.com/ja-jp/library/vstudio/ms229045(v=vs.100).aspx

■No68153 (morimori さん) に返信
> それをドキュメンテーションコメントというのではないですか？

トピックのレスに記載があったのを見ておりませんでした。
申し訳ありませんでした。

> ＞あれこれ何行にも渡るような説明が必要なメソッドって設計がおかしいですよ。仕様というかコード設計。
> そこまでおっしゃるのでしたらその正しいコード設計をぜひ知りたいものです。
> じゃんぬさんなら今回の場合どう対応されます？

# 久しぶりに「じゃんぬさん」と呼ばれた気がする... まあ、それはさておき。

コード設計がおかしいという物言いは乱暴でした。
きちんと読みもせずに、えらそうな書き込みをしてしまった点、お詫び申し上げます。

コード自体はおかしいかどうかで言うなら、「好み」の域を出ないかもしれないですね。
また当該クラスがどんな設計が求められているか、そのセオリーはあるかもしれませんが、
携わっている業務において、そのセオリーが最適かどうかはイコールではない場合もあります。

たとえば、私は Util 系のメソッドにはあまり条件分岐や例外を見逃すようには設計しない人です。
条件分岐は業務ロジック側のフローに記載します。今回のクラスは業務ロジックとは切り離された存在だと考えています。
例外については、インスタンスの状態として実行にふさわしくない場合は素直にスローした方がバグの発見に繋がると考えています。
ですので「好み」の域を出ないと結論づけ、先のレスの乱暴な書き込みについて謝罪致します。

どちらかというと「コメントがすごく丁寧になっているから」、複数行に渡っていると感じました。
いやホントにすごい丁寧ですね。

> /// <summary>
> /// コンストラクタで指定したMemoryMapedAccsessインターフェースを使って
> /// メモリマップドファイルを作成します。すでに同じファイル名があった場合は上書きします。
> /// (1)空のメモリマップドファイルを作成
> /// (2)メンバのMemoryMapedAccsessインターフェースが設定されている場合、保存する
> /// (3)作成に失敗した場合、例外を出力せずにErrorMessageプロパティにエラー内容を書き込む
> /// </summary>
> /// <param name="strFileName">メモリマップドファイル名</param>
> /// <param name="lnMaxCapacity">容量(byte)</param>
> /// <returns>true:作成完了, false 作成失敗</returns>

以下は私の場合の話です。これが正解という意味ではなく私の好みと言った方が良いかもしれません。

・繰り返しますが、業務ロジックでない Util 系のメソッドには条件分岐を設けないで実装します。
・そのメソッドの前提条件的なものは <summary> 要素には書かないです。前提条件がない設計が望ましいからです。
・「インスタンスの状態として、このメソッドを呼ぶのに適さない状態」ということがないようなクラス設計にします。
　わかりやすい例として、順次ファイルを読み込む (前方向ストリーム) 系のクラスなら、途中でエンコーディングは変更するようなクチを用意すべきでないと思います。
　つまり、エンコーディングの指定は setter は設けずにコンストラクタのみ、という設計にしたりしますよね。そういう意味です。
　ついでに、私以外の人間が保守するかもしれないので、private であってもフィールドも読み取り専用にしておき、そんなことは想定していないことを明示します。
・どうしても状態の不整合が起きるクラスの場合は、例外をスローでいいというか、そうすべきだと思っています。
　ただ、その不整合が起きるのが外部からによるものであれば、その setter は必要なのか検討する必要があるでしょう。
　難しい場合は、その setter で引数チェックを行い、不整合が起きる前にとっとと例外をスローします。目的のメソッド呼び出しまで実行させません。
・内部実装が変更になった時に、コメントも編集しなくてはいけないので内部実装のことには触れません ((1) ～ (3) の部分です)。
・"コンストラクタに指定した" の部分は、この「インスタンスが持つ状態」の位置づけになるので不要だと思います。
・すでに書かれているとおり「処理のやっていること」の詳細が気になる人はコードを見た方が安心するのでコメントは見ないです。
・"上書き" するかどうかの個所は引数で指定可能としておき、その引数の説明で記載します。また必要に応じてデフォルトのオーバーロードも用意します。
・ぶっちゃけ .NET の BLC と同じレベルのことしか書かないのが理想なんて思っています。
　JavaDoc の世界も <summary> にあたる部分は簡素ですね。その代わり <remarks> に当たる内容は長いのが多いですがw

まとめるとこんな感じにすると思います。

/// <summary>
/// 　　Memory を Map した内容を表すファイルを作成します。
/// </summary>
/// <param name="filePath">作成されるファイルパス</param>
/// <param name="maxCapacity">ファイルの最大サイズ</param>
/// <param name="overwrite">上書き可能な場合は true、それ以外は false</param>

戻り値を作らないので <return> は書かないつもりですが、どうしても例外を殺す場合は、
/// <return>作成に成功した場合は true、それ以外は false。</return>
としますが、なんか本当に BCL っぽいですね。

どうしても注記事項を記載したい場合は <remarks> を使いますが、ほとんど使用したことはないです。

> コンストラクタで指定したMemoryMapedAccsessインターフェースを使って

これを書きたくなる気持ちはわかります。
でもここに重要性があるなら、クラスの関係性を見直すべきかもしれません。

ちなみに「MemoryMappedAccess」は "インターフェース" と書かれていますが、これは型が interface という意味でしょうか?
.NET は先頭に「I」を付けることになっていますので、どちらかちょっとわからなかったです。
外部インターフェースという意味で "インターフェース" と記載しているのでしょうか?

[追記]
改めてソースを見ると、MemoryMappedAccess というアクセッサ自体が Util 系のクラスに見えるので、CreateMemoryMappedFile メソッドは業務ロジックとしてのメソッドの位置づけですね...
であれば、コンストラクタでこの MemoryMapped のアクセッサーを指定するということは私はしないです。
そのアクセッサーを「生み出したところ」で完結しないと、業務ロジックが分散しているだけになってしまうからです。
業務ロジックはフレームワークではないですから、別のクラスに委ねるようなパターンにはしないです。
背景の理解が遅くて大変申し訳ないです。
[/追記]

先に謝罪したとおり、このあたりまで考えて前回の回答をつけているわけではないですが、
かといって、自分を正当化するためにでたらめを書いているというわけでもないです。
ただ、正解とかそういうつもりはなく、好みの域を出ないのかなと思っています。

# 予防線っぽくてくどい言い回しですかね...

こんな回答が参考になるのか、自信がありませんが...
少しでも参考になったら嬉しいです。
長文すぎて自分でも疲れてしまいました。ごめんなさい。

返事が遅れました。

// メモリマップドファイルを作成
public bool CreateMemoryMapedFile(string strFileName, long lnMaxCapacity)
{
try
{
// 空のメモリマップドファイルを作成
MemoryMappedFile mmf = MemoryMappedFile.CreateOrOpen(strFileName, lnMaxCapacity);
using (var accessor = mmf.CreateViewAccessor())
{
// MemoryMapedAccsessインターフェースが設定されている場合、保存
if (MemoryMapedAccsess != null)
{
MemoryMapedAccsess.SaveObject(accessor);
MemoryMappedFileSave = mmf;
}
}
}
catch (Exception exp)
{
// 作成に失敗した場合は保存エラーを表示
ErrorMessage = strFileName + ERROR_SAVE_MESSAGE;
TraceMessage = exp.ToString();
return false;
}
return true;
}

分かりますか？
あくまで、関数の目的は
「メモリマップドファイルを作成」
この一点につきます。それ以上でもそれ以下でもありません。

「空のメモリマップドファイルを作成」
「MemoryMapedAccsessインターフェースが設定されている場合、保存」
「作成に失敗した場合は保存エラーを表示」
は内部処理であり、目的ではなく手段です。
手段をコメントに書くなら、関数内部のコメントです。

そうすると、ものすごくすっきりすると思いませんか？

関数全体のコメントにすると、仕様変更のたびに実装とかけ離れた場所にあるコメント
を修正せねばならなくなります。
そうすると、コメントの修正を忘れがちになりますし、忘れてしまうとソースコードと
コメントは食い違います。
食い違ってしまえば、コメントの価値は下がります。

だから、場所に応じて、必要十分(多すぎず少なすぎず)なコメントを心がけるべきなのです。

回答ありがとうございます。
実はこの前のコメントを書いている私も
こんなに長いコメントメソッドやクラスに全部書いていたらプログラム完成しないだろうな。
と思ってましたが、やっぱり何事もほどほどが一番なのですね。

＞「処理のやっていること」の詳細が気になる人はコードを見た方が安心するのでコメントは見ないです。
言われると確かに。コメントは案内板みたいなものですし。
コメントがプログラムを動かしているわけではないからそりゃ知ってる人から見たらコメントは
信用するには値しませんね。

ずいぶんいろんな方を巻き込んでしまいまして申し訳ありませんでした。
「いいコメントを考える前にいいコメントが打てるようなソースを作るべし」
という結論で手を打とうと思います。

特にじゃんぬ様詳細な回答をありがとうございます。
これからはユーティリティソースには例外や条件分岐を含ませずに
使用者側でエラー処理させるように設計します。
皆様ありがとうございました。

C# と VB.NET の質問掲示板

ASP.NET、C++/CLI、Java 何でもどうぞ

C# と VB.NET の入門サイト

Re[4]: コメントのいい書き方について

過去ログには書き込み不可