1週間に1回ずつコメントできるといいなぁと思っていたけど2週間に1回のペースになっている須藤です。
リーダブルなコードを目指して：コードへのコメント（1）の続きです。前回はWinMain()の最初の2行目まで読んでコメントしました。ここまではウィンドウモードの設定をしていただけです。さて、続きはどうなっているでしょうか。
リポジトリー： https://github.com/yu-chan/Mario
今回のコメントに関するやりとりをするissue： https://github.com/yu-chan/Mario/issues/2
メイン関数
ウィンドウモードの設定の次の処理は↓のようになっていました。
if(DxLib_Init() == -1) {
return -1;
}
DxLib_Init()という関数名からDXライブラリを初期化していることがわかります。また、おそらく初期化に失敗したら-1が返ってくるのだろうということもわかります。
失敗したらすぐにreturnしているのはいいですね。次のように「成功したらメインの処理を実行」というように書くこともできますが、そうするとインデントが深くなって見通しが悪くなってしまいます。また、「このプログラムで重要な処理はなんなのか」が伝わりにくいコードになりがちです。参考：ifとreturnの使い方
if(DxLib_Init() == 0) {
// メインの処理
}
次のようにreturn -1の代わりにEXIT_FAILUREを使うこともできます。
#include <stdlib.h>
// ...
if(DxLib_Init() == -1) {
return EXIT_FAILURE;
}
EXIT_FAILUREを使うと「失敗扱いで終了するんだなー」ということをコードで伝えやすくなります。が、0以外なら失敗というのはよく知られているので-1をreturnしても特段わかりにくいというわけではありません。
なお、プロセスの終了値でなにかを伝えたいプログラムの場合はEXIT_FAILUREを使わない方がいいです。たとえば、diff(1)は変更があったときは1を返し、エラーがあったときは2を返します。EXIT_FAILUREの値が0以外のなんなのか（1なのか8なのかとか）わからないので、「0以外の値」以上の意味がある場合以外はEXIT_FAILUREを使わない方がよいです。
ところで、Windowsでは負の数を返すのは普通なのでしょうか。Linuxでは正の数を返すことが多い気がしていたので少し意外でした。
あ、そうだ、失敗したときはなにかメッセージを出してから終了する方がいいです。たとえばこんな感じです。
if(DxLib_Init() == -1) {
std::cerr << "DXライブラリの初期化に失敗しました。" << std::endl;
return -1;
}
なお、「なにか」というのは「問題解決に役立つ情報」です。↑でもなにもないよりはヒントになるのですが、↑よりもたとえば「サウンドデバイスの初期化に失敗しました。」の方がより問題解決の役に立ちます。問題を解決するための次のアクション（サウンドデバイスがおかしくないか調べようとか）につながりやすいからです。参考：問題解決につながるエラーメッセージ
次の行を見てみましょう。
SetDrawScreen(DX_SCREEN_BACK);
バックのスクリーンに描画するように設定しているのでしょう。「バックのスクリーンに描画ってどういうこと？」と思ったのでSetDrawScreen()のドキュメントを読んでみました。ダブルバッファリングの設定のようです。読んでいたらtypoを見つけたので報告しておきました。
次の行を見てみましょう。
srand((unsigned)time(NULL));
現在時刻で疑似乱数のシードを設定しているのがわかります。どこかでrand()も使っているのでしょう。
C++なのでCスタイルのキャストよりもC++スタイルのキャストを使った方がよいでしょう。
srand(static_cast<unsigned>(time(NULL)));
C++スタイルのキャストの方が安全でないキャストに気づきやすいので、できるだけC++スタイルのキャストを使うのがよいでしょう。
インスタンス作成
次は似たようなコードが集まっていました。
//フレームレート制御
if(!Framerate::instance()) {
Framerate::create();
}
if(!SoundManager::instance()) {
SoundManager::create();
}
if(!Sequence::Parent::instance()) {
Sequence::Parent::create();
}
インスタンスが存在しなかったら作成する、というコードに見えます。create()の結果をどこにも代入していないのでシングルトンパターンなのでしょう。シングルトンパターンではinstanceという名前を使って実現することが多いので、instanceというクラスメソッドにしているのはリーダブルなコードにつながります。
メイン関数の最後の方には次のようなコードがあります。
Sequence::Parent::destroy();
SoundManager::destroy();
Framerate::destroy();
ここでcreate()で作ったインスタンスを解放しているのでしょう。
シングルトンパターンは少し行儀のよいグローバル変数のようなものなので、できるだけ使わない方がよいです。グローバル変数のようにどこからも参照できる（スコープが広い）ので、影響範囲を把握しにくくなります。これはリーダブルさを下げてしまいます。
今回のケースでは、メイン関数内のローカル変数としてFramerate、SoundManager、Sequence::Parentのインスタンスを作成してそれを適切なオブジェクトに渡すことでも実現できるんじゃないかなぁと思いました。
たとえば次のような感じです。
{
Framerate framerate;
SoundManager sound_manager;
Sequence::Parent parent;
while(!InputInterface::isOn(KEY_INPUT_Q)) { //Qを押したら終了
if(ProcessMessage() != 0) {
break;
}
InputInterface::updateKey();
framerate.update();
ClearDrawScreen();
//ゲーム開始
parent.update();
ScreenFlip();
framerate.wait();
}
}
ローカル変数で実現できるならそのスコープを抜けるときに自動でデストラクターが走るので、手動でdestroy()を呼ぶ必要はありません。そうすると解放処理の呼び忘れがなくなり、安全なプログラムになりやすいです。また、読む側もスコープが小さいほど覚えておくことが少なくなって理解しやすくなります。
後処理
メインループ（while() {...}）は次回以降見ていくとして、今回は後処理部分を見て終わりにしましょう。
DxLib_End();
return 0;
DXライブラリの終了処理をしてプログラムを正常終了していることがすぐにわかりますね。0の代わりにEXIT_SUCCESSを使ってもよいでしょう。
まとめ
リーダブルコードの解説を読んで「自分が書いたコードにコメントして欲しい」という連絡があったのでコメントしています。今回はメイン関数の全体を読んでコメントしました。次回はメインループの中に入ります。
「リーダブルなコードはどんなコードか」を一緒に考えていきたい人はぜひ一緒にコメントして考えていきましょう。なお、コメントするときは「悪いところ探しではない」、「自分お考えを押し付けることは大事ではない」点に注意しましょう。詳細はリーダブルなコードを目指して：コードへのコメント（1）を参照してください。
