ククログ

株式会社クリアコード > ククログ > リーダブルなコードを目指して：コードへのコメント（3）

リーダブルなコードを目指して：コードへのコメント（3）

1週間に1回ずつコメントできるといいなぁと思っていたけど3週間に1回のペースになっている須藤です。

リーダブルなコードを目指して：コードへのコメント（2）の続きです。前回はメイン関数の全体を読んでコメントしました。これに対し、その後、「シングルトンパターンはどういうときに使うのがよいだろう」というのを一緒に考えていました。

リポジトリー： https://github.com/yu-chan/Mario

今回のコメントに関するやりとりをするissue： https://github.com/yu-chan/Mario/issues/3

メインループ

それではメインループの中を読んでいきましょう。

	while(!InputInterface::isOn(KEY_INPUT_Q)) { //Qを押したら終了
		if(ProcessMessage() != 0) {
			break;
		}
		InputInterface::updateKey();
		Framerate::instance()->update();
		ClearDrawScreen();

		//ゲーム開始
		Sequence::Parent::instance()->update();

		ScreenFlip();
		Framerate::instance()->wait();
	}

短くまとまっています。このくらいの粒度で抽象化されていると、全体の流れがわかりやすくて読みやすいですね。このコードからは1回のループでは次の処理をしていることがすぐにわかります。

メッセージを処理（メッセージがなにかはわからないけど）
キー入力を処理
フレームレートを更新（「フレームレートを更新」というのがどういうことかわからいけど）
オフスクリーンの描画領域をクリアー
ゲーム開始（ループ毎に「ゲーム開始」というのがどういうことかわからないけど）
オフスクリーンの描画領域を使用
フレームレートを調整

処理の中身がイメージできないものもありますが、全体像はわかりました。

それでは、順に見ていきましょう。

`InputInterface`

以下の2つの処理が関連していそうなのでまとめて見ていきます。

	while(!InputInterface::isOn(KEY_INPUT_Q)) { //Qを押したら終了

		InputInterface::updateKey();

なぜ関連していそうかと思ったかというと同じクラスに属しているからです。関連しているものをまとめるためにクラスを使うのは読みやすくていいですね。

ただ、InputInterfaceという名前は大げさかなぁと思いました。というのは、現状、このクラスはキー入力しか扱っていません。キーだけでなく、マウスやタッチスクリーンなどいろんな入力も扱っているならInputInterfaceでもいいかもしれませんが、そうではないので、KeyboardInterfaceくらいの方がよさそうに思います。私ならInterfaceは冗長だと思うのでKeyboardにするかなぁ。

InputInterface.hは次のようになっています。

#ifndef INCLUDED_INPUTINTERFACE_H
#define INCLUDED_INPUTINTERFACE_H

class InputInterface {
public:
	static int key[];
	static bool isOn(int id);
	static void updateKey();
};

#endif

気になるのは次の2点です。

#ifndef INCLUDED_INPUTINTERFACE_H
すべてのメンバー関数が静的メンバー関数

最初の#ifndef ...からいきます。

これはヘッダーファイルを重複して読み込まないようにするための伝統的なガード方法なのですが、今どきはこれを実現するには次のようにします。

#pragma once

class InputInterface {
public:
	static int key[];
	static bool isOn(int id);
	static void updateKey();
};

#ifndef ...に指定する識別子を考えなくてもいいし、最後の#endifも書かなくていいのでかなりスッキリします。

「最近のコンパイラーでしか使えないんじゃないの。。。？互換性を考えると#ifndef ...の方がいいんじゃ。。。」と思うかもしれませんが、今どきのC/C++のコンパイラーなら全部使えるので気にしなくてよいです。Wikipediaのpragma onceのページによると使えないのはCray C and C++くらいのようです。（ドキュメントには使えると書いていない。）

次にすべてのメンバー関数が静的メンバー関数なことについてです。

すべて静的メンバー関数であればインスタンスは必要ありません。他のところではシングルトンパターンを使っているので、ここでも同じようにシングルトンパターンを使った方がよいでしょう。（私はインスタンスを作るようにする方が好みですが。）

同じことを実現するのに違うやり方を使っていると「どうしてここは違うやり方なんだろう？実は違うことを実現したいのかも？」と読んだ人が詰まってしまいます。同じプロジェクト内では、同じことを実現するときは同じやり方を使いましょう。

それでは順番に関数を見ていきましょう。

まずはInputInterface::isOn()です。boolを返す関数の名前にisをつけるのは読みやすくなっていいですね。boolを返す関数にisをつけるのはよく見る書き方なので、そういう書き方を知っている人は読みやすいです。isXXX以外にも真偽値を返す関数によく使われる名前があります。たとえばis_XXXやXXX?（SchemeやRuby）やXXXp（Lisp）などです。それぞれの言語でよく使われている書き方を踏襲すると読みやすくなります。

それでは実装を見ていきましょう。

int InputInterface::key[256];
bool InputInterface::isOn(int id) {
	bool flag = false;
	//updateKey();
	if(key[id]) {
		flag = true;
	}
	return flag;
}

おそらく、InputInterface::keyにはキーが押されたら0でない値が入っているのでしょう。0以外の値が入っていたらflagがtrueになるようになっています。

ここで気になることはflagという名前です。オン・オフを表しているのでフラグなのですが、一般的過ぎるので使わなくていいなら使わない方がよい名前だと私は思っています。私ならどう書くかというとこう書きます。

int InputInterface::key[256];
bool InputInterface::isOn(int id) {
	return key[id] != 0;
}

真偽値を返すならそもそもkey[id] != 0の結果を直接使えるので、ローカル変数は必要ないのです。

言語を問わず次のようなコードをちょいちょい見ます。

if(condition) {
	return true;
} else {
	return false;
}

こういうコードは次のようにしましょう。↑のようにifしてからtrue・falseをreturnしていると、読むときに「なにか特別なことをしているのかも？」とちゃんと確認しなければいけません。↓のように不必要なifを使わないことで「あぁ、この条件の結果を返すんだな。特別なことはしていないな。」と読むことができます。

return condition;

なお、コメントアウトして使っていない次のコードも削除しておいた方がよいです。コードをバージョン管理して必要ないコードは削除してしまいましょう。残っていると読むときに「どうしてコメントアウトしているんだろう？」と考えなければなりません。バージョン管理しておけば後から取り出すことができるので、思い切って消しましょう。

	//updateKey();

次はInputInterface::updateKey()を見てみましょう。

void InputInterface::updateKey() {
	char stateKey[256];
	GetHitKeyStateAll(stateKey);
	for(int i = 0; i < 256; i++) {
		if(stateKey[i] != 0) {
			key[i]++;
		} else {
			key[i] = 0;
		}
	}
}

DXライブラリのGetHitKeyStateAll()関数で押されているキーの情報を取得してInputInterface::keyの値を更新しています。押されていればインクリメントして押されていなければ0にしています。

stateKeyはkeyStatesの方がいいんじゃないかと思いました。複数形にすることで複数のキーの状態を保持していることがわかるからです。

同様にInputInterface::keyもkeysと複数形にした方がいいと思います。ただ、keysだとなにが入っているのか不明瞭なので、ちょっと長いですがkeyPressedCountsとかそういう方がいいんじゃないかと思います。

余談ですが、私は「何回」というのを表すときはn_XXXという名前を使っています。たとえばn_pressedです。英語の「the number of XXX」を略してn_XXXです。これはGLibで使われている名前のつけ方ですが、GLib以外でもよく見ます。ただ「何回」が複数個のときには使えないんですよねぇ。XXXの部分が名詞だと複数形になるからです。たとえば「要素数」ならn_elements（「the number of elements」）です。複数形をさらに複数形にできないので、n_elements_setとかn_elements_listとかにするんですが微妙だなぁと思っています。余計な情報が入ってしまうからです。setだと重複を許さないような気がするし、listだとリストで実現していそうな気がします。なので、keyPressedCountsかなぁと思いました。

ところで、keyの値はインクリメントする必要がないかもしれません。次のように単にboolを入れておけば十分な気がします。

key[i] = (stateKey[i] == 1);

この値を使っているところを見てみると、Game/Character.cppに次のようなコードがあるのですが、ここは押されたかどうかの情報だけでも十分なように見えます。

	if(InputInterface::key[keyIndex] == 1 && !isJump) {

あとはバッファーサイズは定数にしておきたいところです。

void InputInterface::updateKey() {
	const size_t nStates = 256;
	char stateKey[nStates];
	// ...
	for(size_t i = 0; i < nStates; i++) {
		// ...
	}
}

Visual C++では配列のサイズに変数を使えなかったような気がするので、こう書かないといけないかもしれません。

void InputInterface::updateKey() {
	char stateKey[256];
	const size_t nStates = sizeof(stateKey) / sizeof(*stateKey);
	// ...
	for(size_t i = 0; i < nStates; i++) {
		// ...
	}
}

ただ、これは少しわかりにくいのが難点です。こう書くときはだいたい次のようにマクロを使ってわかりやすくします。

#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*x))
void InputInterface::updateKey() {
	char stateKey[256];
	const size_t nStates = ARRAY_SIZE(stateKey);
	// ...
	for(size_t i = 0; i < nStates; i++) {
		// ...
	}
}

C++なので同じことをテンプレートでも実現できるのですが、テンプレートでの実装がわかりやすいかというと、うーん、という感じなので今回はやめておきます。（テンプレートを使うとARRAY_SIZEにintの配列ではなくintのポインターを渡したときにエラーにできるという利点があります。）

まとめ

リーダブルコードの解説を読んで「自分が書いたコードにコメントして欲しい」という連絡があったのでコメントしています。今回はメインループ内で使っているInputInterfaceを読んでコメントしました。次回はメインループの違う処理を読んでいきます。

「リーダブルなコードはどんなコードか」を一緒に考えていきたい人はぜひ一緒にコメントして考えていきましょう。なお、コメントするときは「悪いところ探しではない」、「自分お考えを押し付けることは大事ではない」点に注意しましょう。詳細はリーダブルなコードを目指して：コードへのコメント（1）を参照してください。

ククログ

リーダブルなコードを目指して：コードへのコメント（3）

メインループ

InputInterface

まとめ

関連情報

`InputInterface`