きれいなコードとは？(3)
さてさて第三弾です。
シリーズ化は想定していませんでした。
今回のテーマは「コードとコメント」について書いてみようと思います。
その前に前回、前々回を読んでいない方はこちらから（全部1回で完結していますが）
では本題に入ります。
コーディングをしていると、メソッドの説明や、処理の説明などでコメントをする機会があると思います。
メソッドの説明は可能な限り、処理概要、引数、戻り値について書くべきだと思います。
コメントで補うならコードを変えるべし
しかしながら、以下のコード例のように関数コールの時に何の処理をコールしたかをコメントする必要はないと思います。
void main(void) {
// 標準出力に関数に入ったことを出力レベル1で出力する <== このコメントをする前に呼び出す関数を修正すべき
out("main()",1,1);
// ～中略～
// 標準出力に関数から出ることを出力レベル1で出力する <== このコメントをする前に呼び出す関数を修正すべき
out("main()",2,1);
} 関数コールでどんな処理を呼んでいるのかをコメントする場合があるのはライブラリで提供されている関数やメソッドでわかりにくい場合に限るでしょう。自分やチームで作った関数やメソッドの場合は、名前や引数を変える（リファクタリングする）ことで対応する必要のある場面だと思います。
上記のコード例はout()という関数が何をする関数で引数が何であるのかわかりにくいからコメントをしていると考えられます。out()という関数を分かりやすく、そして引数を分かりやすく定数化することで解決できるでしょう。ちょっとやってみましょう。
#define FUNC_IN 1 // 関数に入るとき
#define FUNC_OUT 2 // 関数から出るとき
#define LOG_VERVOSE 0 // 詳細ログ
#define LOG_DEBUG 1 // デバッグログ
#define LOG_INFO 2 // 情報ログ
#define LOG_WARN 3 // 警告ログ
#define LOG_ERROR 4 // エラーログ
#define LOG_LEVEL LOG_DEBUG // このレベル以上のログを出力対象とする
void func_log(char* name, int inout, int level);
void write_log(char* msg, int level);
/**
* 関数の入出力をログ出力する関数
* name: 関数名
* inout: 入口or出口(FUNC_IN, FUNC_OUTで指定)
* level: ログ出力レベル(0～4 LOG_VERVOSE～LOG_ERRORで指定)
*/
void func_log(char* name, int inout, int level) {
char out[256];
if(inout == FUNC_IN) {
sprintf(out, "%s, FUNC_IN", name);
write_log(out, level);
}
else if(inout == FUNC_OUT) {
sprintf(out, "%s, FUNC_OUT", name);
write_log(out, level);
}
else {
sprintf(out, "%s, %s%d%s", name, "wrong parameter: inout(", inout, ")");
write_log(out, LOG_ERROR);
}
}
/**
* ログ出力する関数
* msg: 出力ログメッセージ
* level: ログ出力レベル(0～4 LOG_VERVOSE～LOG_ERRORで指定)
*/
void write_log(char* msg, int level) {
char out[256];
if(level < LOG_VERVOSE || level > LOG_ERROR) {
sprintf(out, "%s%d%s", "wrong parameter: level(", level, ")");
write_log(out, LOG_ERROR);
return;
}
if(level < LOG_LEVEL) return;
printf(msg);
printf("\n");
}
int main() {
func_log("main()", FUNC_IN, LOG_DEBUG); // <== out("main()",1,1);の改良
write_log("This is test!(V)", LOG_VERVOSE); // <== これは出力されない
write_log("This is test!(D)", LOG_DEBUG);
write_log("This is test!(I)", LOG_INFO);
write_log("This is test!(W)", LOG_WARN);
write_log("This is test!(E)", LOG_ERROR);
write_log("This is test!", -1); // <== 引数エラー
write_log("This is test!", 6); // <== 引数エラー
func_log("main()", 3, LOG_DEBUG); // <== 引数エラー
func_log("main()", FUNC_OUT, LOG_DEBUG); // <== out("main()",2,1);の改良
return 0;
}ここまで書くとコメントは不要ですよね。
（※関数コールの右に書いているコメントはnote読者のためのコメントです^^）
出力結果はこうなります。Android開発でよく用いるlogcatのまねごとですね。
main(), FUNC_IN
This is test!(D)
This is test!(I)
This is test!(W)
This is test!(E)
wrong parameter: level(-1)
wrong parameter: level(6)
main(), wrong parameter: inout(3)
main(), FUNC_OUTテストは以下のC codeというオンラインの実行環境で行いましたので、気になる方はそちらで値を変えてテストをしてみてください。
ちょっと大げさなコードになりましたが、何をするかわからないout()という関数が分かりやすい関数名func_log()になり、引数も定数化（今回はマクロ定義を用いた）することで使いやすくなりました。ここまでするとコメントはなくても理解できますね。
マジックナンバーの扱い
マジックナンバーという言葉は聞いたことあるでしょうか？
コード上に突然現れる数値を指します。
「この数字の意味はわからないが、とにかくプログラムは正しく動く。まるで魔法の数字だ」
Wikipediaより引用
ナンバーという文言が付いていますが、数値だけでなく文字列もそれに含みます。例えば、開くフォルダのパスも呼び出すときに直接書いてあったらマジックナンバーと言えます。
でも以下のコードはマジックナンバーでしょうか？
if(msec >= 1000) { // 1msが1000で1秒 <== 本当に必要なコメント？
sec++;
msec = 0;
}
if(sec >= 60) { // 1秒が60で1分 <== 本当に必要なコメント？
min++;
sec = 0;
}
if(min >= 60) { // 1分が60で1時間 <== 本当に必要なコメント？
hour++;
min = 0;
}
if(hour >= 24) { // 1時間が24で1日 <== 本当に必要なコメント？
day++;
hour = 0;
}上記のコードに出てくる1000, 60, 24という値は一緒に出てくる変数名とセットで見れば、ミリ秒を秒に変換、秒を分に変換、分を時間に変換、時間を日に変換ということが見えてくると思います。この1000,60,24に対して定数化する必要はあるでしょうか？そして定数化しない場合コメントは必要でしょうか？
時間の単位変換はよほどの大変革がない限り変わることはないので、敢えて定数化する必要はないと思います。そして60や24という値とsec,min,hourという値を見れば、一般的に検討は付くのでコメントも不要でしょう。
では、次の場合はどうでしょう？
int total_price(int unit_price, int quantity) {
double price;
price = unit_price * quantity;
price += price * 0.10;
return (int)price;
}単価と個数を引数として与えると税込み価格を出力する簡易のプログラムです。もちろん"0.10"が消費税率であることはこの処理を見れば誰でも理解できるでしょう。だからコメントは必要ないと思います。しかしながら、この値は今後も変わらないと言えるでしょうか？
いいえ、税率は時代によって変わります。
よってこのような値は直接書き込んではいけません。税率の他に、測定誤差などを調整するために入れるオフセット値や比率もハードウェア部品によって変わることもあります。こういった調整の値もコード上に直接書くことは推奨されません。
ただ次のような場合はコメントで対応しても問題ないと思われます。
public void onConnectionStateChange(final BluetoothGatt gatt, final int status, final int newState) {
super.onConnectionStateChange(gatt, status, newState);
if (BluetoothGatt.GATT_SUCCESS != status) {
if (status == 8 || status == 133) {
// 8はタイムアウトの発生（ ==> 再接続する ）
// 133はGattエラー（ ==> 再接続する ）
Log.e(TAG, "status( " + status + " )");
～ 省略(再接続処理など) ～
}
}
}上記のコードはAndroidでBLEの開発をした方なら見たことがあると思います。133で定義されているGATTエラーはよくわからないステータスの代表格です。
この場合、この133というステータスコードはここにしか登場しないので、直接条件式に入れてコメントで補っても問題ありません。ただしコメントは必須です。後から見て何かわからなければ、たとえそこにしか出ない値であっても混乱を招く可能性はありますので。
ハードコードを使っていいのは、
絶対に変わらない値と確実にそこにしか現れない値のみ
ただし、後から見たときに、別の人が見たときに理解できるように必要に応じてコメントを残すべし
まとめ
今回の内容をまとめると以下の4つの項目に集約できます。
１．コードを直してコメントが不要にならないか検討する
２．誰が見てもわかる不変な値はハードコーディングでも問題はない
３．変わる可能性のある値は定数化する
４．そこにしか現れない値は直接書き込んでもよいがコメントで補う
すべてのルールに共通して言えるのは
「数年後の自分が見てわかること」
「開発者以外の人間が見て、直感的に理解できること」
「後のメンテナンスを考慮すること」
この3つです。
長くなったので、この辺で締めます。
おまけ．．．（2021.2.28 21:00 追記）
ログの出力のコードにもうちょっとこだわってみた。
出力時にどのレベルの出力かを表示するように「create_log_msg()」でログメッセージを整形するようにしました。
コメントはしていませんが、読み取ってください。
#define FUNC_IN 1 // 関数に入るとき
#define FUNC_OUT 2 // 関数から出るとき
#define LOG_VERVOSE 0 // 詳細ログ
#define LOG_DEBUG 1 // デバッグログ
#define LOG_INFO 2 // 情報ログ
#define LOG_WARN 3 // 警告ログ
#define LOG_ERROR 4 // エラーログ
#define LOG_LEVEL LOG_DEBUG // このレベル以上のログを出力対象とする
void func_log(const char* name, int inout, int level);
void write_log(const char* msg, int level);
char* create_log_msg(const char* msg, int level);
void func_log(const char* name, int inout, int level) {
const char out[256];
if(inout == FUNC_IN) {
sprintf(out, "%s, FUNC_IN", name);
write_log(out, level);
}
else if(inout == FUNC_OUT) {
sprintf(out, "%s, FUNC_OUT", name);
write_log(out, level);
}
else {
sprintf(out, "%s, %s%d%s", name, "wrong parameter: inout(", inout, ")");
write_log(out, LOG_ERROR);
}
}
void write_log(const char* msg, int level) {
char out[256];
if(level < LOG_VERVOSE || level > LOG_ERROR) {
sprintf(out, "%s%d%s", "wrong parameter: level(", level, ")");
write_log(out, LOG_ERROR);
return;
}
if(level < LOG_LEVEL) return;
printf(create_log_msg(msg, level));
printf("\n");
}
char* create_log_msg(const char* msg, int level) {
static const char out[256]; // <== ここはstaticにしているのがポイント
switch(level) {
case LOG_VERVOSE:
sprintf(out, "[%-7s] %s", "VERVOSE", msg);
break;
case LOG_DEBUG:
sprintf(out, "[%-7s] %s", "DEBUG", msg);
break;
case LOG_INFO:
sprintf(out, "[%-7s] %s", "INFO", msg);
break;
case LOG_WARN:
sprintf(out, "[%-7s] %s", "WARN", msg);
break;
case LOG_ERROR:
sprintf(out, "[%-7s] %s", "ERROR", msg);
break;
}
return out;
}
int main() {
func_log("main()", FUNC_IN, LOG_DEBUG);
write_log("This is test!(V)", LOG_VERVOSE);
write_log("This is test!(D)", LOG_DEBUG);
write_log("This is test!(I)", LOG_INFO);
write_log("This is test!(W)", LOG_WARN);
write_log("This is test!(E)", LOG_ERROR);
write_log("This is test!", -1);
write_log("This is test!", 6);
func_log("main()", 3, LOG_DEBUG);
func_log("main()", FUNC_OUT, LOG_DEBUG);
return 0;
}static修飾子を付けているところのstaticを取り除くと、出力がバグります。
この関数を超えたときに値が解放されてしまうようです。
staticを付けない場合、文字列格納のこのchar配列を関数外に出すとよいと思います（検証はしていない）
出力結果は以下の通りです。
[DEBUG ] main(), FUNC_IN
[DEBUG ] This is test!(D)
[INFO ] This is test!(I)
[WARN ] This is test!(W)
[ERROR ] This is test!(E)
[ERROR ] wrong parameter: level(-1)
[ERROR ] wrong parameter: level(6)
[ERROR ] main(), wrong parameter: inout(3)
[DEBUG ] main(), FUNC_OUT普段C言語は組込でしか使わないので文字列の処理はほぼ初心者です(笑)
sprintfなんて使ったの初めてな気がする。
この記事が気に入ったらサポートをしてみませんか？