株式会社クリアコード > ククログ

ククログ


Supershipさまで「既存のコードを変更するノウハウ」を学ぶ研修を行いました

クリアコードの結城です。

この度Supershipさまからご依頼を頂き、先方のグループ各社内の新入社員の方々を対象とした研修の一環として、「既存のコードを変更するノウハウ」を学ぶワークショップを2018年9月に開催しました。
この記事では研修の実施に至った背景事情と、実際に研修の中でお伝えした内容をご紹介します。

(研修中の様子の写真)

「社会人レベルのコードを書けるようになる」とはどういう事か?

本件は元々、Supershipさまのグループ各社横断で行われる新人研修の一環で、「社会人レベルのコードを書けるようになる事」をテーマに1日かけて行う特別プログラムとしてご相談を頂きました。

そこでまずは、「社会人レベルのコードを書ける」とはどういう事か、「学生レベル」や「趣味レベル」との違いはどこにあるのかという事から考え直してみることにしました。
その結果、以下のようなトピックが挙がりました。

  • 「チームでプログラムを開発する人」としての能力
    • 既存のコードを自然な形(周囲のコードに馴染む形)で変更する能力。
      ー 発生した問題の原因を調べる能力。
    • 他の人が見て分かりやすい(他の人がメンテナンスしやすい、他の人が調査しやすい)コードを書く能力。
    • テストしやすいコードを書く能力。
  • 「ビジネスマン」としての能力。
    • 納期に間に合わせる能力。
    • 予算(想定工数)の範囲内に収める能力。
    • 「納期が厳しくてメンテナンス性を諦めないといけない」のような二律背反の状況で、物事の優先順位を付けて判断する能力。

ただ、研修の実施日程を考慮すると、これら全てを扱うことはできません。
そこでご依頼主さまと相談し、この中で特に「既存のコードを自然な形(周囲のコードに馴染む形)で変更する」という話題に焦点を当てた内容とする事にしました。

課題の選定

研修にあたっては「課題のための課題」は避けたいという思いがありました。やるなら、そこまでに実施された研修の内容を踏まえて、なるべく日常的に実プロダクトで行う事に近い事をしてもらうのが良いはずです。

ただ、ご依頼主さまの会社内の機密情報に基づく内容とした場合、資料の二次利用は困難になります。
当社の期待としては「成果をフリーソフトウェアにしたい」「実施した事の内容を公開して、同様の事でお悩みの会社さまに見付けていただく」という事があるため、できれば公開可能な情報のみに基づく資料としたい所でした。

事前に伺った情報では、ご依頼主さまの事業においてRuby on Rails の大規模なコードベースを長期わたって運用されているとの事でした。
実際の業務では、そのようなプロダクトに対し変更を行っていく機会がそれなりに見込まれます。
そこで、著名な公開プロジェクトの中でも条件が近い物として、Redmineを題材にすることにしました。

また、Redmineに対してどのような変更をするかに関しても、題材に使いやすそうな規模の既知の要望が見つかりました。
目標を「この要望を実現するパッチを作る事」に置けば、既存の大きな実装に対して変更を行う際の一通りの作業を体験できるはずですし、進度次第では成果をそのまま公開でき、ご依頼主さまの会社自体の社外向けのアピールにも繋がります。

この要望については、既に同様の事を実現するプラグインが存在しているというのも好都合でした。
実装が難しい部分はプラグイン自体のコードをそのまま流用する事で手間を省けるため、「既存のコードに馴染むように機能を組み込む」という点に集中して取り組めるはずです。

以上の事を踏まえ、研修当日にソースコードのダウンロードやHTTPサーバーのセットアップ等の本質的でない部分で手間取る事がないよう、必要なソフトウェアやソースコードなど一式を用意した状態のVagrant Boxを準備し、研修当日に臨みました。

座学:既存のコードを変更する時の心得

当日の研修開始直後からいきなり「さあ、やってみましょう」で作業を始めてしまうと、参加者の皆さんが途方に暮れてしまうのは目に見えています。
そこで、研修当日の内容は2部構成とし、まず最初に「既存のコードに変更を行う」という作業の一般的な流れを説明する座学の時間を設け、実作業を伴う演習はその後に行う事にしました。

ここで用いたプレゼンテーション資料はgitlab.com上のリポジトリにて公開しています
また、スライドはSpeaker Deckでも見る事ができます。

新機能の追加にせよ不具合の修正にせよ、継続的に運用されるサービスに関わる開発業務のかなりの部分は「既存のコードの変更」となります。

既存のコードに合わせた変更を行う・既存のコードに馴染むコードを書くためには、既存のコードを読んで内容を把握する必要があります。
しかし、膨大な量のコードを頭から1行ずつ読んでいくというのは非現実的です。
業務でコードを書く人は常に時間に追い立てられているなので、見るべきポイントを押さえて少しでも効率よくコードを読みたいものでしょう。

そのため、座学の前半では「どんな情報を読み取るためにコードを読むのか」という点に焦点を当てて、「コーディングスタイル」「設計方針」「背景事情」のそれぞれを読み取るために注目するべきポイントやコードの読み進め方を説明しました。

座学の後半では、コードを読んで得た情報を元にして変更を行う時の具体的な流れとして、「変更計画の立て方」「(TDDでの)自動テストの書き方」「自動テストをしやすい形での実装の仕方」などについても説明しました。
ただ、前半の話題に比べるとこれらの点は一般化が難しく、一般的に語れるレベルの話となると抽象度が高すぎて研修の趣旨にそぐわなくなると予想されたため、後半の説明は軽めにして、なるべく長い時間を実際にコードと向き合う演習に充てられるようにする事にしました。

演習:Railsアプリケーションへの機能追加

休憩を兼ねた昼食を挟んだ後は、研修の第2部であり本題となる演習の時間としました。

教材のセットアップ

最初にgitlab.com上のリポジトリで公開されている資料を皆さんの手元にcloneしていただき、記載された手順に則って教材用の環境をセットアップしていただきましたが、ここで1つ多くの方が躓かれていた点がありました。

この手順でセットアップした環境はVagrant Box(実体はVirtualBoxの仮想マシン)となり、その中に置かれたファイルを編集するためにはSSHでリモートログインするなどの手順を踏む必要があります。
今回の研修に参加された方々のほとんどは、それまでの研修ではVisual Studio CodeやAtomなどのテキストエディタを使われていたことから、ここで「VS CodeやAtomでVagrant上のファイルを閲覧・編集する」という事を実現するための設定が必要となっていました。

この時は、手間取っていた人には既にうまく行っていた人が情報を共有するという形で相互に助け合ってもらい、本題に取り組む段階に無事進む事ができました。

コードの読解と設計の把握、実際の変更

準備が整った事でようやく本題に着手です。

前述の通り、今回の課題はRedmineでコメントの変更があった時にメールで通知できるようにするという物です。
これについて、とりあえずソースコードを検索して読んでみる人、送信されるメールのテンプレートを起点に探してみる人、怪しそうな所をとりあえず変更してみる人、同様の事を実現する既存のプラグインを実際に組み込んでその動作の様子を追う人など、各人各様のアプローチで課題に取り組まれている様子が窺えました。

ただ、中には、いきなり大規模のソースコードに触れた事で混乱が生じたためか、どこから切り崩していけばよいか判断に迷って途方に暮れている人も見られました。
そのような場合の取り組み方について座学でいくつか方法を紹介したつもりではあったのですが、知識として知ったという事と、それを実際の場面に適用できるという事の間には隔たりがあるのかもしれません。
そのため、室内を巡回しながら、戸惑っている様子が見られた方には個別に声をかけて、「座学で述べたこの話をここで活かすと、こういう風に調べていける」という補足説明をしていく事にしました。

そうこうするうちに実装案を思いつく所に到達した人も現れてきて、講師が意見を求められたため確認した所、Controllerのレイヤで目的を実現するという方針を取っていました。
このアプローチは、目的とする機能を実現できているという点は良いのですが、Redmineの他の部分に自然に馴染むかという点では、違和感が大きいというのが講師視点での印象でした。
というのも、Redmineには既に「チケット自体の変更をメールで通知する」という仕組みが実装されており、それらとはかなり異なるアプローチでの実装と言えます。
将来的にRedmineプロジェクトに変更を取り込んでもらうという場面までを考慮に入れると、このように「既存の実装に馴染まない」「収まりの悪い」変更内容は、マージに至らない事が多々あります。
そのため、その人に対してはアプローチの再検討を試みてはどうかと講師から促しました。

実装の進度には受講者同士の間で差が生じていたため、進度が高い人には適宜、他の詰まっている人にノウハウや情報を共有するという事もしてもらいました。
何かを自分で人に説明してみるというのは、説明対象についての理解を深める上で非常に効果的です。
これは、筆者がワークショップに関わる際に意識している点の1つです。

より小さい規模での実践の機会も

研修の実施中、思いがけない出来事もありました。
参考になる実装例として紹介した既存のプラグインの動作を確認していた人が、その動作に奇妙な点があると気付いたので深掘りしてみた所、なんとプラグインの方に不具合があるという事が明らかになったのです。

対象とするソフトウェアの規模的には想定から外れますが、この問題に取り組む事は、「既存のソフトウェアに変更を加える」という本旨の部分では研修の趣旨に沿った物である言えます。
そのため、講師の側で判断して、その人には当該プラグインに対するフィードバックを行ってみてもらう事にしました。

こちらは、最終的にはプルリクエストの形で変更を提案し、いくつかのやりとりを経て微修正後に無事マージされるに至っています。

成果の報告とまとめ

研修は1日の予定でしたが、当日に台風が接近しており帰宅困難となる恐れがあったため、後半の演習を途中で打ち切り、日を改めて続きを実施しました。
初日に引き続いての作業の後、研修の最後の30分はそこまでの成果の報告会としました。

報告会では複数の参加者から、既存の実装を読んでメソッドの呼び出し元を辿り処理の流れを追う事の意義を実感したという声が聞かれました。

Railsというフレームワーク自体の理解が浅かった参加者からは、メソッドの呼び出し関係を辿るのに苦労したという声も聞かれました。
フレームワークを使ったアプリケーションの処理を追う上では、そのフレームワークの一般的な作法の知識が必要になります。基本的にはフレームワーク自体のチュートリアルを一通り実施して、そのフレームワークの作法に触れた上で、実際に作られたプロダクトに触っていくのが望ましいでしょう。

メールの送信に関わっていそうな部分を当てずっぽうで変更してみたものの期待した結果を得られず、後になってみると見当違いの所ばかりを見てしまっていた、という反省の声もありました。コードの全体を把握するのは時間がかかりすぎるという状況においては、ある程度は推測に基づいて調査範囲を絞り込む必要がありますが、完全な当てずっぽうでは、当たるか当たらないかは運次第になってしまいます。仮説を立てる際は、「ここで表示されているメッセージがここで定義されているので」や「必要な処理がここで呼ばれているので」のように、その仮説の根拠を説明できるようにしておくと良いでしょう。

進捗が良かった参加者の中には、処理を追ってControllerの該当部分を特定できたという人(プラグインを参考に処理を組み込む方法を検討している所で時間切れになった)や、実際に動作してコメントの変更内容の差分がメール送信される所にまで到達した人もいました。この方々には思考の整理も兼ねて、どのように調査してその実装に至ったのかについて他の参加者に情報を共有する事をお薦めしました。

また、成果発表の時間の最後には模範解答例として、講師がこの作業を行った際の手順もご紹介しました。この時は担当した講師にRailsとRedmineの知見があったため、クラスの定義箇所に見当を付けて探すなど、効率よく調査を行えたケースとなっていました。

今回の研修で使用した課題そのものは一般に公開可能な内容のため、研修の終了後に自主的な取り組みとして、以下のような事にも挑戦してみて頂きたい旨ご案内しました。

  • 差分を出力する処理について、Redmine独自の処理と、プラグインが使っているGemのどちらを使うべきか(あるいは全く別の選択肢を取るべきか)の検討。
  • 通知設定に基づいて、自分自身の変更を送らないようにする方法の検討。
  • 実際にRedmineプロジェクトの当該チケットにパッチを提出してみる。

まとめ

以上、Supershipさまにて実施した「既存のコードを変更するノウハウ」を学ぶ研修の様子についてご報告しました。

当社はフリーソフトウェアとビジネスの両立という理念に則り、フリーソフトウェア・OSSの開発に長く関わる中で培ってきた技術的な知見をビジネスに繋げる形で、開発やテクニカルサポートなどの業務を行っています。
その中には「知見そのものを伝える事」も含まれており、過去にはSEゼミのリーダブルコード勉強会アジャイルアカデミー「実践リーダブルコード」などのようなワークショップ形式のイベントの開催に度々携わってきました。また、テクニカルサポートやコンサルティングのような形で顧客企業内のOSS推進の取り組みを支援させて頂いた事例もあります。
今回の研修も、その流れの中にある事例の1つと位置付ける事ができます。

本件の研修と同内容の研修や、その他のフリーソフトウェア・OSSの開発に実際に関わる形での研修を社内で実施したいとお考えの会社さまは、ぜひお問い合わせフォームよりお問い合わせ下さい。

2018-11-12

リーダブルなコードを目指して:コードへのコメント(5)

db tech showcase Tokyo 2018Apache Arrow 0.11.0のリリースとかしていたら2ヶ月ちょい経ってしまっていた須藤です。

リーダブルなコードを目指して:コードへのコメント(4)の続きです。前回はフレームレート関連の処理のところを読んでコメントしました。

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

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

ゲーム本体

今回はついにゲーム本体の処理を見ていきます。ゲーム本体の処理はメインループからはじまるのでまずはメインループの中をおさらいします。

	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();
	}

「ゲーム開始」というコメントがあるのでSequence::Parentを見ていきましょう。

定義はSequence/Parent.hにありました。名前空間とパスが一致していて探しやすいですね。

#ifndef INCLUDED_SEQUENCE_PARENT_H
#define INCLUDED_SEQUENCE_PARENT_H

namespace Sequence {

class Child;

class Parent {
public:
	static void create();
	static void destroy();
	static Parent* instance();

	void update();

	enum NextSequence {
		NEXT_TITLE,
		NEXT_GAMEOVER,
		NEXT_ENDING,
		NEXT_GAME,

		NEXT_NONE
	};
	void moveToNextSequence(NextSequence);
	
	void deleteChild();

private:
	Parent();
	~Parent();
	static Parent* mInstance;

	NextSequence mNextSequence;
	Child* mChild;
};

}

#endif

以下の部分はシングルトンパターンを実現するためのコードなので今回は無視しましょう。

	static void create();
	static void destroy();
	static Parent* instance();
	Parent();
	~Parent();
	static Parent* mInstance;

ということで注目するのは以下の部分です。

namespace Sequence {

class Child;

class Parent {
public:
	void update();

	enum NextSequence {
		NEXT_TITLE,
		NEXT_GAMEOVER,
		NEXT_ENDING,
		NEXT_GAME,

		NEXT_NONE
	};
	void moveToNextSequence(NextSequence);
	
	void deleteChild();

private:
	NextSequence mNextSequence;
	Child* mChild;
};

}

メインループはupdate()を呼んでいるだけだったので、update()の中でmoveToNextSequence()deleteChild()を呼んでいるのでしょう。

名前から想像するとそれぞれのメンバー関数が実現する機能は次の通りです。

  • update()
    • ループ毎に適切な次のなにかを実行する。キャラクターを動かすとか。
  • moveToNextSequence()
    • NextSequenceNEXT_TITLEとかNEXT_GAMEOVERなので、ゲームの状態(開始とか終了とか)の遷移をするんだろう。
    • Sequenceという名前空間はこのNextSequenceSequenceと同じ意味なの?
    • 同じ意味ならSequence::Parent::NextSequenceというように複数回出現するのは冗長かなぁ。
    • 違う意味なら紛らわしいので違う単語を使うとわかりやすくなりそう。
  • deleteChild()
    • 子どもを削除?
    • このクラスはParentclass Child;という前方宣言もあるのでなにかしら親子関係がありそうだけど、なにとなにが親子関係になっているんだろう。

私はゲームを作らないのでゲーム固有の用語をよく知らないのですが、シーケンスという名前や(なにかわからないけど)親子関係にするのは普通なのかもしれません。実装も読んでみましょう。

update()は次のようになっています。

void Parent::update() {
	mChild->update(this);

	switch(mNextSequence) {
		case NEXT_TITLE:
			deleteChild();
			mChild = new Title();
			break;
		case NEXT_GAMEOVER:
			deleteChild();
			mChild = new GameOver();
			break;
		case NEXT_ENDING:
			deleteChild();
			mChild = new Ending();
			break;
		case NEXT_GAME:
			deleteChild();
			mChild = new Game::Parent();
			break;
	}

	//処理をしておかないと、次へ進めない
	mNextSequence = NEXT_NONE;
}

メインの処理はmChildが扱うようです。update()ではなにもしていませんでした。

caseの中を見るとそれぞれのシーケンス(状態?)毎にmChildのオブジェクトが変わっています。NEXT_TITLEならTitleオブジェクトになっていますし、NEXT_GAMEOVERならGameOverオブジェクトになっています。

update()内でmNextSequenceが変わっていないのが気になりますが、たぶん、deleteChild()内で変えているのでしょう。

caseNextSequenceのすべてをカバーしていないことが気になりました。NEXT_NONEだけがないのですが、おそらく、NEXT_NONEのときはなにもしないから省略したのでしょう。

caseですべての値をカバーしていないとコンパイラーが警告を出すはずなので私は書いておいた方がよいと思います。次のような感じです。

	switch(mNextSequence) {
		case NEXT_TITLE:
			// ...
			break;
		case NEXT_GAMEOVER:
			// ...
			break;
		case NEXT_ENDING:
			// ...
			break;
		case NEXT_GAME:
			// ...
			break;
		case NEXT_NONE:
			// Do nothing
			break;
	}

case NEXT_NONE:を使わずにdefault:を使う方法もあります。

	switch(mNextSequence) {
		case NEXT_TITLE:
			// ...
			break;
		case NEXT_GAMEOVER:
			// ...
			break;
		case NEXT_ENDING:
			// ...
			break;
		case NEXT_GAME:
			// ...
			break;
		default:
			// Do nothing
			break;
	}

私は使わなくて済むときはdefault:を使わないようにしています。そうすると、新しいenum値を追加したときにコンパイラーが(警告という形で)教えてくれるからです。これにより新しいenum値の対応漏れを減らせます。

たとえば、NextSequenceNEXT_CONTINUEを追加して次のようになったとします。

	enum NextSequence {
		NEXT_TITLE,
		NEXT_GAMEOVER,
		NEXT_ENDING,
		NEXT_GAME,
		NEXT_CONTINUE,

		NEXT_NONE
	};

default:を使っていると既存のswitchで特に警告はでませんが、使っていない場合は「NEXT_CONTINUEの処理がないよ」とコンパイラーが教えてくれるはずです。

それではdeleteChild()を見てみましょう。

//画面遷移する際に、今の画面を破棄する
void Parent::deleteChild() {
	delete mChild;
	mChild = 0;
}

mChildを解放しているだけでした。mNextSequenceParent内では変えないようです。

mChild = 0はC言語ではNULLを代入しているのと同じことですが、C++では0よりもnullptrを使った方がよいです。整数と区別できますし、ヌルポインターを使いたいという意図を表現できます。

mChild = nullptr;

ただ、C++11からしか使えないので古いコンパイラーもサポートしないといけないときは使えません。

一応、moveToNextSequence()も見てみましょう。

//画面遷移を更新
void Parent::moveToNextSequence(NextSequence next) {
	mNextSequence = next;
}

mNextSequenceを更新しているだけですね。外のオブジェクトからこのメソッドを呼んで状態を更新するのでしょう。でも誰がどこから?Sequence::Parentはシングルトンパターンになっているのでどこからでも更新できます。mChildのオブジェクトの実装を見ていくと見つかるでしょう。

Sequence::Parentは以上です。今回はわからないことが多かったです。どうしてSequenceという名前空間なんだろうとか、どうしてParentというクラス名なんだろうとかです。ParentmChildではなく、もう少し具体的な名前をつけられないでしょうか。たとえば、ScenariomStageとかです。今後、mChildのオブジェクトになるクラスを見ていくとなにかよい名前が浮かぶかもしれません。

まとめ

リーダブルコードの解説を読んで「自分が書いたコードにコメントして欲しい」という連絡があったのでコメントしています。今回はついにゲームの処理に入りました。しかし、まだわからないことが多く、名前がピンときていません。次回はより具体的な処理を1つ選んで読んでいきます。

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

2018-10-26

リーダブルなコードを目指して:コードへのコメント(4)

まだギリギリ3週間に1回のペースの須藤です。このシリーズのファンができました

リーダブルなコードを目指して:コードへのコメント(3)の続きです。前回はユーザーからの入力処理のところを読んでコメントしました。

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

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

フレームレート

今回はメインループ中のフレームレート関連の処理を見ていきましょう。

まずはメインループの中をおさらいします。

	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();
	}

この中の以下の部分がフレームレート関連の処理のはずです。

		Framerate::instance()->update();
		Framerate::instance()->wait();

なぜかというとFramerateクラスに属しているからです。

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

#ifndef INCLUDED_FRAMERATE_H
#define INCLUDED_FRAMERATE_H

class Framerate {
public:
	static void create();
	static void destroy();
	static Framerate* instance();

	void update();
	void wait();

	int cnt() const;

private:
	Framerate();
	~Framerate();
	static Framerate* mInstance;
	
	int mStartTime;
	int mCnt;
	float mFramerate;
};

#endif

以下の部分はシングルトンパターンを実現するためのコードなので今回は無視しましょう。

	static void create();
	static void destroy();
	static Framerate* instance();
	Framerate();
	~Framerate();
	static Framerate* mInstance;

ということで注目するのは以下の部分です。

class Framerate {
public:
	void update();
	void wait();

	int cnt() const;

private:
	int mStartTime;
	int mCnt;
	float mFramerate;
};

名前から想像するとそれぞれのメンバー関数が実現する機能は次の通りです。

  • update()
    • 環境に合わせたフレームレートを変える。
    • マシンが重かったらフレーム減らすとか。
  • wait()
    • 現在のフレームレートに合わせたフレーム数にするために速すぎたら少し休む。
  • cnt()
    • 単位時間内でいまのところ何フレーム表示したかを返す。

うーん、update()はメインループ中で毎回呼ばれているんですが、そこでフレームレートを毎回調整するとは思えないんですよねぇ。実装を見てみましょう。

//フレームを更新
void Framerate::update() {
	if(mCnt == 0) {
		mStartTime = GetNowCount();
	}
	if(mCnt == INTERVAL) {
		int t = GetNowCount();
		mFramerate = 1000.0f / ((t - mStartTime) / (float)INTERVAL);
		mCnt = 0;
		mStartTime = t;
	}
	mCnt++;
}

この中で初めて見るのはGetNowCount()INTERVALです。

GetNowCount()はDXライブラリが提供している「Windowsが起動してからの経過時間をミリ秒単位で返す」関数でした。

INTERVALCommon.hで次のように定義されていました。

//フレームレート
#define INTERVAL 60
#define FPS 60

これをふまえると次のことがわかります。

  • フレームレートの単位時間は「Framerate::update()の呼び出し回数がINTERVAL(60)回」
    • 1秒とかではない。
  • フレームレートの単位時間がくる度にmStartTimeの時刻をリセットしている。
  • Framerate::update()を呼び出す毎にmFramerateを更新している。
    • が、ここで計算しているフレームレートの単位時間がわからない。変な計算式な感じ。
    • mFramerateを使っているところがなさそうなので、ここのコードは今は必要なさそう。

mFramerateは必要なさそうなので消した方がいいでしょう。必要ないコードがあると、読むときに「どうしてここにこんなコードがあるんだ。。。」と考えないといけなくなり、理解を妨げてしまいます。コードをバージョン管理していれば消したコードを戻すことができるので、1人で開発しているときでもバージョン管理しましょう。バージョン管理していれば安心してリーダブルなコードにするための変更を重ねていけます。実は、バージョン管理したほうがいいというのはリーダブルコードの解説にも書いています。

実装を読んでみた結果「フレームレートは更新していない(mFramerateがいらなそう)だし、フレームレートの更新というかフレームを1回進めるのが目的っぽいのでupdate()じゃない名前がよさそう」という気持ちになりました。なんていう名前がいいのかなぁ。tick()とかかなぁ。フレームを1つ進めます、というイメージ。Node.JSにはprocess.nextTick()というのがあるし。

ということで、こんな感じにするのはどうだろう。

//フレームを進める
void Framerate::tick() {
	if((mCnt % INTERVAL) == 0) {
		mStartTime = GetNowCount();
		mCnt = 0;
	}
	mCnt++;
}

うーん、FramerateというかFrameの方がいいのかなぁ。

もやもやしたまま次に進みましょう。Framerate::update()です。

//フレームが早かったら、早いぶんだけ待つ
void Framerate::wait() {
	int t = GetNowCount() - mStartTime;
	int w = mCnt * 1000 / FPS - t;
	if(w > 0) {
		Sleep(w);
	}
}

こちらは名前から予想していた通りの実装です。ただ、FPSの使い方がもやっとします。update()では単位時間は秒ではなかったのにここでは単位時間は1秒(FPSはFrame Per Secondだから)になっています。INTERVALFPSを統合できないかしら。こんな感じ?

//フレームを進める
void Framerate::tick() {
	if((mCnt % FPS) == 0) {
		mStartTime = GetNowCount();
		mCnt = 0;
	} else {
		int t = GetNowCount() - mStartTime;
		int w = mCnt * 1000.0 / FPS - t;
		if(w > 0) {
			Sleep(w);
		}
	}
	mCnt++;
}

で、メインループでは最後にtick()を呼ぶだけ。これで動かないかしら。

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

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

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

あと、変数名はこんな感じにしたいですね。

//フレームを進める
void Framerate::tick() {
	if((mIndex % FPS) == 0) {
		mStartTime = GetNowCount();
		mIndex = 0;
	} else {
		int expectedElapsedTime = (1000.0 / FPS) * mIndex;
		int elapsedTime = GetNowCount() - mStartTime;
		int restTime = expectedElapsedTime - elapsedTime;
		if(restTime > 0) {
			Sleep(restTime);
		}
	}
	mIndex++;
}

Framerate::cntは単にmCntを返しているだけでした。

int Framerate::cnt() const {
	return mCnt;
}

そうだろうなぁという実装です。が、これを使っているコードはなさそうなので消したほうがよさそうに思いました。

まとめ

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

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

つづき: 2018-10-26
2018-08-13

リーダブルなコードを目指して:コードへのコメント(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_XXXXXX?(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以外の値が入っていたらflagtrueになるようになっています。

ここで気になることは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してからtruefalsereturnしていると、読むときに「なにか特別なことをしているのかも?」とちゃんと確認しなければいけません。↓のように不必要な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にしています。

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

同様にInputInterface::keykeysと複数形にした方がいいと思います。ただ、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_SIZEintの配列ではなくintのポインターを渡したときにエラーにできるという利点があります。)

まとめ

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

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

つづき: 2018-08-13
2018-07-20

リーダブルなコードを目指して:コードへのコメント(2)

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以外なら失敗というのはよく知られているので-1returnしても特段わかりにくいというわけではありません。

なお、プロセスの終了値でなにかを伝えたいプログラムの場合は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()で作ったインスタンスを解放しているのでしょう。

シングルトンパターンは少し行儀のよいグローバル変数のようなものなので、できるだけ使わない方がよいです。グローバル変数のようにどこからも参照できる(スコープが広い)ので、影響範囲を把握しにくくなります。これはリーダブルさを下げてしまいます。

今回のケースでは、メイン関数内のローカル変数としてFramerateSoundManagerSequence::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)を参照してください。

つづき: 2018-07-20
2018-06-28

リーダブルなコードを目指して:コードへのコメント(1)

リーダブルコードの解説を書いた須藤です。

リーダブルコードの解説を読んだ大木さんから次のような連絡をもらいました。

私はプログラマーなのですが、まだまだ未熟で、人が読めるコードを書くことができません。 本の内容を意識しコーディングしてみましたが、自信がありません。 そこで、添削していただきたいと考え、メールを送りさせていただきました。

リーダブルコードの解説に「私にコードを見せてみるといい」と書いていたので連絡してくれたとのことです。解説を書いてから6年経ちますがこのような連絡をもらったのは初めてです!うれしいですね。

大木さんから提供してもらったコードは https://github.com/yu-chan/Mario です。そこそこ量があるので何回かに分けて少しずつコメントします。

なお、私と大木さんだけでやりとりしてコメントをするのではなく、誰でも参照できるところでコメントするのは大木さんと合意しています。できるだけ多くの人と情報を共有したいという私の希望もある(リーダブルコードの解説がCreative Commonsなのも同じ理由)のですが、大木さんにとっても私以外の人からもコメントをもらえる機会がある方がよいと考えたからです。ということで、この記事を読んだ人で興味がある人はコメントしてみてください。この記事のように文章でまとめて https://github.com/yu-chan/Mario/issues で大木さんに連絡するのもよいですし、リポジトリー上のコードに直接コメントするのでもよいでしょう。

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

注意点は次の通りです。

  • 悪いところ探しではない
    • 目的はどんなコードがリーダブルだろう?どうしてリーダブルなんだろう?を共有することです。既存のリーダブルなところを見つけてそれを明文化することはそのためにすごく大事なことです。
    • 悪いところ探し「だけ」をしたい人はコメントしない方がよいでしょう。
  • 自分の考えを押し付けることは大事ではない
    • リーダブルかどうかは読む人によって変わります。「自分」がリーダブルなら他の人もリーダブルであるべきだ、ということではありません。
    • コメントする中でいろんな視点に気づき、いろんなリーダブルがあることを知ることが大事です。それは自分がリーダブルなコードを書くときにも使えますし、チームのリーダブルを見つけるときにも使えるはずです。
    • 自分の考えを押し付けたい人はコメントしない方がよいでしょう。

README

私はこのコードの前提知識がないので、まずはREADMEから確認します。

トップレベルのREADME.mdは次のようになっています。

"# Mario" 

どこから手を付ければよいかのヒントを期待したので期待と違いました。

トップレベルには次のファイル・ディレクトリーがありました。

  • Mario/
  • Mario.sln
  • README.md

拡張子が.slnのファイルがあるのでVisual Studioでビルドするんだな、ということがわかります。README.md.slnのファイルを除くとあとはMario/ディレクトリーしかないので、このディレクトリーを見てみましょう。

このディレクトリーの下のファイルの拡張子の多くは.cpp.hです。C++で実装していることがわかります。このディレクトリーにもREADMEがあるので見てみましょう。

1. 開発環境

OS       : Windows7/10
(ゲーム作成したときの環境に7、10を使用していた。
もしかしたら8でもいけるかもしれない。)
OSビット : 64ビット
IDE      : Visual Studio Express 2012 for Windows Desktop

まず開発者が使っている環境が書いてあります。これはすごくよいですね!この環境なら動くということがわかるので、読む人にとって非常に助かる情報です。

また、「もしかしたら8でもいけるかもしれない」と「やっていないこと」についても書いているのがよいです。コードのコメントに書くときもそうですが、「やっていないこと」についての情報は有用なことがあります。「やっていない」条件で使わないといけなくなったときにどう対応すればよいかの指標になります。「いけるかもしれない」であれば、「動くなら動いた方がよい」ということがわかります。この場合なら、Windows 8で動くことを確認できたらここの記述を更新すればよいし、動かなくても簡単に動くようにできそうなら頑張るということがわかります。

「やっていないこと」と同じように「やらないこと」もあれば書いてあると読む人の役に立つことがあります。たとえば、「Windows 8はサポートしない」と書いていれば、Windows 8用の作業はしないと判断できます。

READMEのこれ以降の説明を読むとDXライブラリを使っていることがわかりました。DXライブラリのURLも書いているのでDXライブラリのサイトにすぐにアクセスできました。サイトには次のような説明があるのでゲーム開発用の便利ライブラリーだということがわかりました。

DirectXを使ったWindowsデスクトップアプリの開発に必ず付いて回るDirectXやWindows関連のプログラムを使い易くまとめた形で利用できるようにしたC++言語用のゲームライブラリ

コードの構成の説明は特にないのでメイン関数(プログラムの処理を始める関数)から見ていくことにします。手元で動かせるならまずは動かすのですが、私の環境はWindows 7/10ではないので今回は動かさずにコードを見るだけにします。

メイン関数

どこにメイン関数があるか探し方はいろいろあります(たとえばmainで検索する)が、ファイルを眺めていたらmain.cppという名前のファイルがあったのですぐにわかりました。よい名前ですね。

メイン関数をmain.cppという名前をつけるのはよくあるやり方です。よくあるやり方に合わせるのは理解しやすくなる人が増えるのでリーダブルという観点でよいやり方です。

他によくあるやり方は実行ファイルと同じ名前を使うやり方です。今回はMario.exeができると思うのでMario.cppという名前にするという感じです。

main.cppの中は次のようになっていました。上から順に見ていきましょう。

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) {
	ChangeWindowMode(TRUE);
	SetWindowSize(WINDOW_WIDTH, WINDOW_HEIGHT);

	if(DxLib_Init() == -1) {
		return -1;
	}
	SetDrawScreen(DX_SCREEN_BACK);

	srand((unsigned)time(NULL));

	//フレームレート制御
	if(!Framerate::instance()) {
		Framerate::create();
	}
	if(!SoundManager::instance()) {
		SoundManager::create();
	}
	if(!Sequence::Parent::instance()) {
		Sequence::Parent::create();
	}

	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();
	}

	Sequence::Parent::destroy();
	SoundManager::destroy();
	Framerate::destroy();
	DxLib_End();

	return 0;
}

最初の行で「ん?」と思いました。

	ChangeWindowMode(TRUE);

なぜ「ん?」と思ったかというと、「TRUEだとどんなモードということなのか」ということがわからなかったからです。ChangeWindowMode()について調べるとDXライブラリが提供している関数でした。DXライブラリには「ウインドウモード」と「フルスクリーンモード」というモードがあるらしく、ChangeWindowMode(TRUE)は「ウインドウモードにする」ということでした。

説明を読んだ結果、「この行はDXライブラリのことを知っていればわかるんだろうなぁ」と思いました。おそらく、このコードを読む人はDXライブラリのことを知っているべきだと思う(前提知識としてもよさそう)のでこのままでもよいと思いました。(前提知識によってリーダブルなコードがどんなコードかは変わってくるので前提知識をどこに置くかは大事です。)

ただ、もしDXライブラリのAPIが次のようになっていればDXライブラリのことを知っていなくてもわかりそうだなぁと思いました。

	UseWindowMode();

もし、DXライブラリをすでに使っている人でも同じような気持ちになるなら、DXライブラリにフィードバックしてよりよいAPIを模索するのがよさそうだなぁと思います。

DXライブラリを知らない人でも「ん?」と思いにくくするならアプリケーション内でUseWindowMode()という名前の関数を定義してそれを使うという方法があります。こうすればUseWindowsModeという名前から「あぁ、ウインドウモードというものを使うんだな」ということがわかります。「ウインドウモード」というのがわかりにくそうなので、DisableFullScreenとして「あぁ、フルスクリーンじゃない(普通のウインドウを使う)んだな」というコードにするのもよいでしょう。

static int UseWindowMode() {
	return ChangeWindowMode(TRUE);
}

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) {
	UseWindowMode();
	// ..
}

それでは次の行を見てみましょう。

	SetWindowSize(WINDOW_WIDTH, WINDOW_HEIGHT);

これもDXライブラリが提供する関数で、ウインドウモードの設定をするものでした。DXライブラリを知らない人でもわかりやすくするならこれも合わせて次のようにするとよいと思います。

static void InitializeWindowMode() {
	ChangeWindowMode(TRUE);
	SetWindowSize(WINDOW_WIDTH, WINDOW_HEIGHT);
}

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) {
	InitializeWindowMode();
	// ..
}

次のようにコメントで補足する方法もありますが、今回のケースでは関数に切り出す方がよいでしょう。関数に切り出すと読む人は詳細を知らなくてもよくなるからです。「あぁ、まずウインドウモードというやつを初期化するんだなー」くらいの理解でよいなら関数に切り出した方がよいです。「まずウィンドウモードというやつを初期化するんだな。そして、それはこういう内容なんだな。」というところまで知っておいた方がよいなら切り出さずにWinMain()の中に書いておいた方がよいです。

int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE hPrevInstance, LPSTR lpCmdLine, int nCmdShow) {
	// ウインドウモードの初期化
	ChangeWindowMode(TRUE);
	SetWindowSize(WINDOW_WIDTH, WINDOW_HEIGHT);
	// ..
}

SetWindowSize()の引数を640, 480と書かずに、WINDOW_WIDTH, WINDOW_HEIGHTと書いているのはよいですね!関数名がSetWindowSizeなので引数が幅と高さだろうということはすぐにわかるので640, 480でもそんなに悪くないのですが、「他にもウインドウサイズに関係している処理があったらどうしよう。ここの値だけを変更するのでは変更漏れがあるかも。」という気持ちになるので、値に名前をつけておくとリーダブルになります。

リーダブルかどうかの判断基準の大きな部分は「そのコードがなにをやっているかがすぐにわかるかどうか」ですが、「そのコードを読んでもやっとしないかどうか」も大事な要素です。「もやっとする」と他のところが気になってしまって集中できません。そうすると次のコードに進みにくくなってリーダブル度が下がってしまいます。

まとめ

リーダブルコードの解説を読んで「自分が書いたコードにコメントして欲しい」という連絡があったのでコメントをはじめました。

まだ、READMEとメイン関数の2行目までしかコメントしていませんが、引き続きコメントしていきます。「リーダブルなコードはどんなコードか」を一緒に考えていきたい人はぜひ一緒にコメントして考えていきましょう。

2018-06-14

クリアなコードの作り方: 変わらない値とわかるようにする

問題を調査する場合やプログラムを改良する場合、修正する場合などプログラムの挙動を把握しなければいけないことは多々あります。プログラムの挙動を把握する方法はいろいろあります。処理の流れを追う方法や特定の値に注目する方法や状態の変化を追う方法などです。

処理Aと処理Bの間でおかしくなっていることがわかっている場合は処理の流れを追う方法がよいでしょう。AからBの間の処理の流れを追っていけば問題の箇所を見つけられるからです。

特定の値がおかしくなっていることがわかっている場合はその値に注目して把握する方法がよいでしょう。その値が変わる可能性がある箇所に注目すれば問題を特定できます。

メインループ(GUIのイベントループや言語処理系の評価器のループなど)内の処理を把握する場合は状態の変化を追う方法がよいでしょう。状態を変えながら何度もループする処理では状態によって実行する内容が変わります。状態を把握することで実行する内容も把握しやすくなります。

ここでは特定の値に注目してプログラムの挙動を把握する場合にこんなコードだったら把握しやすいというコードを紹介します。これはコードを読む人視点の言い方です。コードを書く人視点で言うと、挙動を把握しやすいコードの書き方を紹介します、になります。

特定の値に注目する

「特定の値に注目する」とはどういうことでしょうか。それはその値が変わるタイミングに注目するということです。

たとえば、次のようなクラスがあり、@ageの値に注目するとします。

class User
  def initialize(age)
    @age = age
  end

  def age
    @age
  end

  def age=(new_age)
    @age = new_age
  end
end

@ageが変わる可能性がある箇所は以下の2箇所です。

  def initialize(age)
    @age = age
  end

  def age=(new_age)
    @age = new_age
  end

そのため、この2つのメソッドが呼ばれる箇所に注目します。なぜなら、ここでしか@ageは変わらないからです。ここに注目しておかしな値を設定している処理を見つけることで問題を特定できます。

users = {}
users[:user1] = User.new(-1) # 注目
users[:user2] = User.new(14) # 注目
puts(users[:user1].age)
users.each do |key, user|
  if user.age == -1
    user.age = 29 # 注目
  end
end

この例では@ageを変更できる箇所は2箇所でしたが、もっと変更できる箇所がある場合は注目する箇所も増えます。注目する箇所が増えるということは挙動を把握することが難しくなるということです。逆に言うと、変更できる箇所が少ないと挙動を把握しやすくなるということです。

ここまではコードを読む人視点の話です。

特定の値に注目しやすいコードの書き方

それでは、コードを書く人視点の話に入ります。

値を変更できる箇所が最小限になっているとコードを読むときに挙動を把握しやすくなるのでした。つまり、値を変更できる箇所が最小限なコードを書くと挙動を把握しやすいコードになります。

それでは、値を変更できる箇所を少なくする方法をいくつか紹介します。

セッターメソッド(ライターメソッド)を提供しない

前述の例ではコンストラクターとセッターメソッドが@ageを変更できる箇所でした。

  def initialize(age)
    @age = age
  end

  def age=(new_age)
    @age = new_age
  end

変更できる箇所を少なくするならage=メソッドを提供しなければよいです。

class User
  def initialize(age)
    @age = age
  end

  def age
    @age
  end
end

これで変更できる箇所が1つ減りました。age=メソッドがなくなったので使い方も変わります。

users = {}
users[:user1] = User.new(-1) # 注目
users[:user2] = User.new(14) # 注目
puts(users[:user1].age)
users.each do |key, user|
  if user.age == -1
    # ↓userを変更するのではなく新しくインスタンスを作ることになった
    users[key] = User.new(29) # 注目
  end
end

セッターメソッドを無くしても注目する箇所は3箇所から減っていないのですが、コードを読む側としてはうれしいことが増えました。それは、一度作ったインスタンスでは@ageは変わらないということです。

インスタンスは1度作られるといろいろな箇所で使われるでしょうが、インスタンスが作られる箇所はそれよりは少ないことが多いです。たとえば、以下の箇所でインスタンスを使っています。

users = {}
users[:user1] = User.new(-1)
users[:user2] = User.new(14)
puts(users[:user1].age) # 使っている
users.each do |key, user|
  if user.age == -1     # 使っている
    users[key] = User.new(29)
  end
end

あれ、使っている箇所(2箇所)より作っている箇所(3箇所)の方が多いですね。。。実際のプログラムでは使っている箇所の方が多いはずです。。。

使っている箇所が多い前提で話を進めると、多いと@ageが変わる可能性があるかを検討することが大変になります。そのため、検討する箇所が少なくなるのはコードを読む側にとってうれしいです。

コンストラクターでだけ値を設定する

セッターメソッドを無くした結果、コンストラクターでだけ値を設定するようになりました。

class User
  def initialize(age)
    @age = age
  end

  def age
    @age
  end
end

@ageを変更する箇所を減らすということであれば、次のようにすることもできました。

class User
  def initialize
  end

  def age
    @age
  end

  def age=(new_age)
    @age = new_age
  end
end

使うときはこうなります。

user = User.new
user.age = 29

これよりもコンストラクターで値を設定するコードの方がよい理由は次の通りです。

  • @ageが変更されるタイミングはインスタンス作成時だけということが明確になる。(=インスタンスを作った後に変更されない。)
  • @ageは必ず設定されていることが明確になる。(セッターメッソドを使う場合だとインスタンスを作った後に設定を忘れると設定されない。)

つまり、コードを書いた人の意図が明確になるので読むときに助かるということです。

「変更しない値はコンストラクターでだけ設定する」はコードを書いた人の意図を明確にする書き方なので、「このクラスのインスタンスは値を変更しないぞ!」という意図でコードを書いているときは積極的に活用したい書き方です。

変更できないようにする

これまでの例では、あえて値が数値になるようにしていました。これは、Rubyでは数値は変更不可能なオブジェクトだからです。文字列は変更可能なオブジェクトなので事情が変わってきます。

たとえば、次のコードはmessageメソッド経由でも@messageの値を変更できます。

class Commit
  def initialize(message)
    @message = message
  end

  def message
    @message
  end
end

次のようにすると変更できます。

commit = Commit.new("Hello")
puts(commit.message) # => Hello
commit.message << " World!"
puts(commit.message) # => Hello World!

これも防ぐようなコードにするかどうかはケースバイケース(オーバースペックの場合の方が多い)ですが、たとえば、次のようすればmessageメソッド経由では@messageの値を変更できなくなります。

class Commit
  def initialize(message)
    @message = message.dup
    @message.freeze
  end

  def message
    @message
  end
end

commit = Commit.new("Hello")
commit.message << " World!" # 例外:can't modify frozen String (RuntimeError)

Rubyは値(オブジェクト)を変更不能にする方法はfreezeですが、方法は言語ごとに違います。たとえば、CやC++ではconstを使います。

まとめ

コードを書くときに「変わらない値」とわかるような書き方にすることで、読むときに挙動を把握しやすいコードになるということを紹介しました。

いくつか書き方を紹介しましたが、中でも、「変更しない値はコンストラクターでだけ設定する」書き方は書いた人の意図を明確にしやすい書き方なのでそのような場面になったら活用してください。

なお、この話題についてまとめるきっかけになったのは次のようなコードを見たことがきっかけでした。外部で作られているオブジェクトに新しくroute_keyというデータを付与したいというコードです。

route = create_output
route.instance_eval do
  route.singleton_class.class_eval do
    attr_accessor :route_key
  end
end
route.route_key = key

このコードは最終的に次のようなコードになりました。セッターメソッドで値を設定するのではなくコンストラクターで値を設定するようになっています。

class Route
  attr_reader :output
  attr_reader :key
  def initialize(output, key)
    @output = output
    @key = key
  end
end

output = create_output
route = Route.new(output, key)
2016-08-17

WEB+DB PRESS Vol.86特集「1年目から身につけたい! チーム開発 6つの心得」の全文公開が始まりました

約1年前に執筆した記事の全文が、技術評論社Webサイトの開発者向け記事のコーナーで公開され始めています

全7章を短期集中連載の形式で順次公開していく予定で、本稿執筆時点では第6章までが公開済みとなっています。(5月2日追記:現在は全章を公開済みです。)

各章の概要

特集は全7章ですが、各章の紹介である第1章を除いた第2章から第7章までが、タイトルの「6つの心得」に対応しています。

前半の第2章から第4章は、 「チームで開発する際の、プログラムそのものの良い設計」 をテーマにしています。

一般に、開発者向けで新人さんなどを対象にした記事というと、GitHubなどのサービスやGitなどの個別のプロダクトの使い方を紹介する物が多いのではないでしょうか。 これらの章では敢えてその方向には振らずに、サービスやツールに依存しない一般的な知見の紹介に努めています。

開発者向けのサービスやツールは流行り廃りが激しいため、ともすれば、トレンドを追う事だけに振り回されてしまうというような事に陥りがちです。 しかし、ここで述べている基本的な設計の仕方や考え方についての理解を深めれば、いたずらにトレンドを追わされるのではなく、目指す設計や開発の仕方をサポートするための物として、トレンドの中から適切な道具を選び取っていけるようになるでしょう。

後半の第5章から第7章は 「開発やメンテナンスにおける、チーム内での良いコミュニケーション」 をテーマにしています。

チーム開発は文字通り「チームでの作業」ですから、チームメンバー同士でどのように連携するかが大切になってきます。 どんなに有能なメンバー達を揃えていてもコミュニケーションが取れていなければ期待したような成果は上げられません。

後半の章では、チームメンバー間でのコミュニケーションを円滑にし、より良い形で作業を進められるようにするための、考え方や施策を紹介しています。 新しいメンバーをチームの一員として迎え入れ、お互いの長所を活かし合って最大限のパフォーマンスを引き出すための最初の一歩として、これらの事が役に立つのではないでしょうか。

記事の再利用について

この特集記事を公開するにあたって、本文と画像はすべてクリエイティブ・コモンズのライセンスを設定しました。 ライセンス種別はCC-BY-NC-SAです。

クリエイティブ・コモンズは作品の使い方について許可する利用形態をあらかじめ明示するライセンスで、CC-BY-NC-SAは以下を再利用の条件として定めています。

  • BY:作者名を明記すること。例えば、作者名を勝手に書き換えて自分1人の著作であると言ったり、著者不明の公共物と言ったりして再配布することはできません。
  • NC:実費も含め、対価を得ないこと。例えば、紙に印刷した物を配布するときにコピー代を受け取ったり、有料の電子書籍として販売したりすることはできません。必ず無料で配布する必要があります。
  • SA:同一の条件で配布すること。例えば、内容を改良した物を配布する時に「この改良版に対する変更は禁止します」のように新しい条件を付け加える事はできません。内容を変更した版も、元の版と同じ条件で再配布する必要があります。

言い換えると、これらの条件に則った使い方であれば、完全に無許諾での再利用が可能です。 例えば以下のような利用方法が考えられます。

  • 社内での研修資料として無料で配布する。
  • 有料のセミナーで資料として使う。(対価を得てはならないのは資料自体の配布についてなので、セミナー自体の参加費用を求める事は問題ありません。一方で、セミナー内で資料を印刷した物が必要な人には追加で100円を支払ってもらう、というような形だと、資料の配布自体に対価が発生している事になるため不許可となります。)
  • ツールの移り変わりなど、最新の状況に合わせて内容を更新した物を公開する。
  • 「これ以外にももっと大事な事がある!」と思う内容を新しい章として追加して公開する。
  • 「ここ、凄く良い事言ってる!」と思う部分を抽出して、発展させた新しい記事を書き、公開する。(※その場合、「SA」の条件があるため、発展版の記事も同一の条件で再配布する必要があります。)

また、著作権法上の「私的利用」や「引用」の要件を満たす使い方であれば、いわゆるフェアユースにあたるため、上記の諸条件は適用されません。例えば以下のようなケースが該当します。

  • 手元に置いておくために、印刷して保存しておく。あるいは、ページをダウンロードして自宅のファイル共有サーバに保存しておく。
  • 自分で書いた記事の中での意見の補強材料として、あるいは反対意見の紹介として、この記事の一部を引用する。また、その記事を有料の電子書籍として販売する。

なお、これらはあくまでこちらへご連絡をいただかずに再利用される場合の条件です。 例えば有料の電子書籍としての販売などをお考えの場合は個別の対応が可能ですので、お問い合わせフォーム等にてご連絡下さい。

まとめ

WEB+DB PRESSの特集記事「1年目から身につけたい! チーム開発 6つの心得」が公開されました。 再利用しやすいライセンスを設定していますので、広くお使いいただいて、良い開発・良いコミュニケーションの促進に役立てていただければ幸いです。

2016-04-28

「チームメンバーのリーダブルコードを見つけて共有」を体験 - ピクシブさんでリーダブルコードワークショップを開催

2015年8月11日にピクシブさん会場提供でピクシブの開発者(7名)と永和システムマネジメントの開発者(1名)に参加していただき、「リーダブルコードワークショップ」を開催しました。

このワークショップは「チームメンバーのリーダブルコードを見つけて共有」することを体験する内容になっています。体験した内容が有益だと思ったらチームに取り入れていって欲しい、という趣旨です。

この内容は「メンバーのリーダブルコードを見つけて共有することが習慣になっているチームはリーダブルコードが当たり前になっていく」という仮説をベースとしています。図にすると次の通りです。

フィードバックループ

メンバーのリーダブルコードを見つけるためにはメンバーのコードを読まないといけません。書く側は「メンバーに読まれている」とわかっていればリーダブルコードを書こうという気持ちが強くなり、リーダブルコードが増えていきます。リーダブルコードが増えると見つかるリーダブルコードも増えていきます。

自分が見つけたリーダブルコードは、次に自分が書くコードに取り入れます。チームの誰かがリーダブルコードを書くことにより、リーダブルコードが徐々に他のメンバーにも共有されていきます。新しいリーダブルコードの書き方を見つけた・編み出したらそれを使ってコードを書く。リーダブルコードを書くことによりチームでリーダブルコードを共有できる。そんなフィードバックループです。

さらに進んだフィードバックループ

このフィードバックループが回り始めると「リーダブルコードが当たり前のチーム」になっていくのではないかということです。

5時間程度でこのフィードバックループを1周体験するのがこのワークショップです。今後、このワークショップをいろんなチームに提供していくべきかどうか、有用性を確認するためにピクシブさんと永和システムマネジメントさんに協力してもらいました。後述する通り、貴重なフィードバックをもらいました。ありがとうございます。

参加者視点でどうだったかについては、ピクシブさんがまとめてくれています。興味のある方はチームにとってのリーダブルコード - pixiv insideもあわせてご覧ください。

背景

どうしてこの内容のワークショップを提供しようと検討しているかについて、簡単に背景を説明します。

リーダブルコードの解説を書いてからは「リーダブルコード(=読む人が読みやすいコード)」というテーマでコードについて他の人に伝える機会が増えました。

クリアコードは社名の通り「クリアなコード(=書いた人の意図が明確なコード)」を大事にしています。「クリアなコード」と「リーダブルコード」はどちらも大事にしていることは同じです。読んで理解できるコードです。

「クリアなコード」を大事にするというのは、元々は「自分たちが書くコードはクリアなコードにしよう!」という気持ちでした。しかし、「リーダブルコード」というテーマでコードについて他の人に伝える機会が増えるうちに「他の人たちがクリアなコード・リーダブルコードを書くことも手伝いたい!」という気持ちになってきました。

開発は実際に手を動かす作業がたくさんあります。説明を聞いて頭で理解するだけよりも、実際に体験して体でも感じられた方が、より多くのことを伝えることができそうです。そのため、ワークショップの提供を検討しています。

対象者

ワークショップ開催前にピクシブさんからヒアリングをしました。こちらが想定している内容が参加予定の開発者にとって有用そうかを検討するためです。

ヒアリングの結果、想定していた内容を変えて、もっと突っ込んだ内容にした方がよいことがわかりました。想定よりも対象者のレベルが高かったのです。(ヒアリングしてよかったです。)

最終的にワークショップの対象者は次のような開発者にしました。

  • 世の中で言われているベストプラクティスが自分にあったものかどうかを取捨選択できる人
  • 書籍「リーダブルコード」を読んで日々リーダブルコード(=読む人が読みやすいコード)を書いている人、または、書籍を読んでいないが日々リーダブルコードを書いている人。(本人が「リーダブルコードを書いている」と考えていれば書いているとする。)
  • 自分だけでなくチーム全体でもリーダブルコードを書きたい人

ざっくりとまとめると、自分はリーダブルコードを書ける自立した開発者で、チーム全体でも同様のことを実現したい人です。

内容

ワークショップは次のような内容にしました。

  • 概要説明
  • 個人で開発を体験
  • ペアで開発を体験
  • チームで開発を体験

体験が3つにわかれているのは、1人→2人→3人以上というように段階的に体験するためです。

「個人で開発を体験」するフェーズでは「後で他のメンバーがリーダブルコードを探すために読む」という前提で開発します。それを意識して「リーダブルコード」を書きます。これは、フィードバックループでいう「読まれるしリーダブルコードを書こう!」という部分の体験になります。

読まれることが前提でリーダブルコードを書こう!

「ペアで開発を体験」するフェーズでは「個人で開発を体験」するフェーズで開発したコードの中からリーダブルコードを見つけて共有します。これは、フィードバックループでいう「リーダブルコードはないかな?」と「お、リーダブルコードだなぁ。自分が書く時に取り入れよう!」の部分の体験になります。

リーダブルコードはないかな?お、リーダブルコードだなぁ。自分が書く時に取り入れよう!

「チームで開発を体験」するフェーズでは参加者全員(今回は8人)のコードの中からリーダブルなコードを見つけて共有します。これは、これまで個人(1人)・ペア(2人)でやったことをチーム(3人以上)にスケールさせる体験です。

チームでフィードバックループ

これらの内容を通じてフィードバックループを1周体験し、「メンバーのリーダブルコードを見つけて共有」が自分たちのチームにとって割にあうか(コストよりメリットが大きいか)を判断してもらおうという狙いです。

なお、内容の詳細はGitHub上で公開しており、CC BY-SA 4.0のライセンスで自由に利用できます。

フィードバック

チームにとってのリーダブルコード - pixiv insideの「感想」のところにもある通り、参加者のみなさんから貴重なフィードバックをもらいました。ありがとうございます。

開催側からの視点として整理します。

普段はレビューを通して、良くない部分や直すべき部分を指摘する・される事が多いのですが、今回の*「良い部分を共有する」という体験は新しく*、それを意識づけるきっかけができて良かったです。

この感想をもらえたことは、このワークショップの内容が開催側の狙いを実現するための内容として適切だったということと捉えました。

最近は、コードを読むというと「コードレビュー」としてコードを読むという場面を思い浮かべることが多そうです。Webの記事や雑誌の記事でコードレビューについて書かれていることが多いからです。コードレビューは「コードに問題がないことを確認する」ことが1番の目的です。コードを共有する側面もありますが、それは副次的な目的です。

一方、このワークショップでの読み方の1番の目的は「リーダブルコードを見つけて共有する」ことです。問題が見つかることもありますが、それは副次的な効果です。

目的が違えば読み方も変わります。

「問題がないことを探す」ときは漏れがないか、特殊なケースでも大丈夫か、などといった観点で読むことでしょう。言い方はアレですが、少しイジワルな視点で読むということです。

「リーダブルコードを探す」ときは「自分がすぐに理解できたか」を意識して読まないといけません。意識しないと見つけられないからです。(意識しなくてもすぐに理解できるようなコードがリーダブルコードです。)「いいこと探し」視点で読むということです。

この違いを体験できたという感想をもらえたので、ワークショップの内容として方向性は間違っていなそうです。

レビューする側もされる側も人間なので、指摘を通してある種のすまない気持ちや否定された気持ちを持ってしまう事があるのですが、良い部分を共有するという行為を習慣化する事で、コードのためだけではなくチームメンバーの感情のためにも良い影響がありそう

今回のワークショップは技術的な理由だけから「チームメンバーのリーダブルコードを見つけて共有」を体験する内容にしたので、感情的な側面については考えていませんでした。たしかに感情的な側面でもよい効果がありそうなので、新しい視点を得られた貴重なフィードバックでした。

なにかしら仕組みを作ることで感情的な側面の効果をより活かすことができそうです。GitHubベースで考えるなら、GitHubではコミットやコードに対してコメントをつけることができるので、よい部分を見つけたらコメントしておき、別途用意した「よい部分を見つけた!」コメントを収集して一覧表示するツールを使ってチームで共有するという仕組みを思いつきました。

他にも、すでにコードレビューでうまく回っているチームならこのワークショップは必要ないかもという知見も得られました。

まとめ

クリアコードは「チームメンバーのリーダブルコードを見つけて共有」を体験する「リーダブルコードワークショップ」の提供を検討しています。

ピクシブさんと永和システムマネジメントさん協力のもとこのワークショップの有用性を検証してみました。それぞれの開発者が自立してリーダブルコードを書いているが、チーム全体の取り組みにはまだ届いていないというチームには有用そうです。

「チームメンバーのリーダブルコードを見つけて共有」するという取り組みは技術面だけでなく、チームの感情的な側面で有効かもしれないという仮説が得られました。

ピクシブさん・永和システムマネジメントさんご協力ありがとうございました。

自分たちのチームでも「チームメンバーのリーダブルコードを見つけて共有」を体験して、チームでリーダブルコードのフィードバックループを回すか検討する材料にしたい、という方はお問い合わせください。1回10万円(最大8名)で「リーダブルコードワークショップ」を提供します。

チームでフィードバックループ

2015-08-13

SEゼミ2015 - OSS Hack 4 Beginnersを開催 #sezemi

2015-06-27にOSS開発参加未経験学生向けOSS開発イベントであるOSS Hack 4 Beginnersを開催しました。このイベントについて、内容を作った立場からどうしてこのような内容にしたのかについて紹介します。また、今回の内容の課題と今後の解決案についてもまとめます。

目標

このイベントの目標は「OSS開発参加への不安を取りのぞくこと」にしました。参加者から不安が取りのぞかれていれば目標達成ということです。

この目標にした理由は、学生から「OSSの開発に参加したい気持ちはあるけど、漠然とした不安があって行動に移せていない」という声を聞いたからです。実際、イベントに参加した学生も同様の理由でOSSの開発の参加に至っていないということでした。

OSSの開発に参加することが当たり前の人視点では「とりあえず手を動かせばいいのに」と考えてしまいます。そのため、OSSの開発の参加に至っていない理由が前述の理由だということには思いもよりませんでした。勉強になりました。

OSSの開発に限らず、「漠然とした不安」で行動に移せていないケースは他にもあるのかもしれません。

内容の方針

「漠然とした不安」を取りのぞくため、次の方針の内容にしました。

  • 選択肢を減らす
  • 安全な環境を用意する
  • 実際に経験する

選択肢を減らす、特に、選択肢をオススメのもの1つにすることにより、「間違った選択肢を選んでしまう」という不安を排除し、正しい方向へ進んでいる感を与えます。選択肢がなくなった部分に関して自分で考えることが減ってしまうというデメリットがありますが、それは不安が解消し、余裕をもって行動できるようになってからでよいと割りきります。

なお、選択肢を減らすという方針はpixivこしばさんからのリーダブルコード勉強会へのフィードバックが元になっています。ありがとうございます。

安全な環境を用意するのは「うまくいかなくても大丈夫感」を与えて不安を排除するためです。選択肢を減らすのは「うまくいっている感」を与えて不安を排除するためでしたが、逆方向からも不安を排除するということです。

上述のように最初の1歩の敷居をさげ、実際に経験します。実際に経験することで、よくわからない「OSSの開発に参加」というものが具体的なものになります。具体的なものになれば、「漠然とした不安」は「心配することではなかった」と「具体的な困っていること」になります。「具体的な困っていること」は個別に具体的な対策を立てて解消できます。

内容

この方針を元に次の内容にしました。

  1. オススメの手順を「1つだけ」伝える
  2. 手順をやっているところを見せる
  3. 実際に手順をやってみる
    • 手順をやるときはそばでメンターがサポートをする
    • 手順をやるときは失敗しても大丈夫なOSSに対してやる

「1.」と「2.」は「選択肢を減らす」方針の実装です。1.で手順を伝え、2.で見本を見せ、「これを真似すればよい」ようにします。

「3.」の箇条書き2つは「安全な環境を用意する」方針の実装です。メンターがそばでサポートするので、自分ではうまく進められていなくても軌道修正できます。失敗しても大丈夫なOSSの開発に参加するので、メンターのサポートをすり抜けて失敗してしまっても大丈夫です。なお、失敗しても大丈夫なOSSとは「メンターが開発に関わっているOSS」です。開発者がその場にいるので失敗しても大丈夫だよ、というわけです。

「3.」は「実際に経験する」方針の実装です。↑の不安を排除する施策を実施した上で実行します。

OSSの開発に参加するオススメの手順

実際に紹介したOSSの開発に参加するオススメの手順を説明します。

それは「まず動かす」ことです。開発者としてではなくユーザーとして動かします。

開発するぞ!という意気込みで着手しようとするといきなり開発者として関わりたくなるものですが、そうではなく、まずはユーザーとして関わることがオススメです。

理由は、まずユーザーとして関わることで次のことがわかるからです。(逆に言うと、次のことをわかるためにユーザーとして動かします。)

  • 何をするソフトウェアなのか
  • どうやって使うか

これがわかったかどうかは「他の人に教える」ことで確認できます。他の人に何をするソフトウェアか、どうやって使うかを教えて、その人がそのソフトウェアのことをわかって使えるようになったら自分がわかっているということです。

さて、これらのことがわかるためには次のことをする必要があります。

  1. 「このOSSについて」のようなドキュメントを読む
  2. そのOSSをインストールする
  3. インストールしたOSSを動かしながらチュートリアルを実行する

多くのOSSでは、これらをする中でなにかしらつまづきます。

今までOSSの開発に参加していなかった人は、つまづいたら次のような行動をとります。

  • グチる
  • 使うことをやめる
  • Webを検索して解決策を見つけて解決し、先に進む

OSSの開発に参加するオススメの手順では違う行動をとります。つまづいたところをメモに残します。(今回のイベントではGitHubのissueを使いました。つまづいたところ1つに対して1つのissueです。)

メモに残すことは「直す」あるいは「報告する」ためです。「直す」も「報告する」もOSSの開発に参加していることになります。つまり、自分がつまづいたところを「OSSの開発に参加するチャンス!」だと捉えて行動するということです。これまで「OSSの開発に参加するきっかけなんてないよ…」と思っていた人も、見方を変えるだけで実はきっかけはたくさんあったことに気づきます。

グチったり、使うことをやめたり、回避策を見つけて先に進む代わりに、「次の人が同じようにつまづかないように」します。それが「直す」あるいは「報告する」です。

この時点では「直す・報告する」対象はコードではなくドキュメントのことが多いでしょう。それでいいのです。ドキュメントの改良も大事なOSSの開発です。ドキュメントの書き方でつまづかなくなる人がいるのですから。自分のちょっとした行動で、次の人がつまづかなくなるってステキなことじゃないですか。

一通り動かして、このOSSのことがわかったら、溜まっていたメモを元にupstream(開発元)にフィードバックします。

フィードバック方法については、次のような例を示し、個別のケースについてはメンターのみなさんにお任せしました。(もっと具体的な手順を示した方がよかったかもしれない。)

  • フィードバック時には次の情報を含める(「バグレポートに必要な情報」が指標)
    • 何をしたか
    • 期待した結果はなにか
    • 実際の結果はどうだったか
  • ↑の情報をまとめるときは次の手順でまとめる
    1. まずは自分がわかるようにまとめる
    2. 自分がわかるようにまとめたものを、開発者に伝わるようにまとめる(開発者にとってリーダブルなようにまとめる)

なお、オススメの手順を説明した資料は次の資料です。

結果

ほとんどの参加者が漠然とした不安を払拭できたようです。(イベントの最後に全員に挙手してもらって確認しました。)

この内容は今回のイベントのために考え、初めて実行したものです。今回の結果だけを考慮しただけですが、「漠然とした不安」を解消するやり方として、「オススメの手順を1つだけ示し」、「安全だと感じれる環境」で、「実際に経験する」というのは有効なやり方と言えそうです。

機会があれば、同様のアプローチで「OSSの開発に参加する人を増やす」ことを試してみたいです。(OSSの開発に参加する人を増やしたい方はご相談ください。)

よかったこと

今回のイベントでよかったことおよび次も続けたいことをまとめます。

  • メンターが楽しそうだったこと
    • 楽しいポイントがどこだったのかはわかりませんが、楽しかったのは非常によいことです。(参加者にも「楽しんでOSSの開発に参加して欲しい」と伝えました。)
  • メンターが参加者の隣に座れるイスがあったこと
    • イスに座ってサポートするとより深くサポートできるのではないかという気がしました。
    • pixivさんが提供してくれた会場には脇にたくさんイスがあり、すぐにイスを追加できて便利でした。

課題と解決案

今回の内容は、優秀なメンターがいたから成り立っていました。そのため、同様の内容を再現することが難しいのが課題です。

また、失敗しても大丈夫なOSS(メンターが開発者なOSS)は必ずしも題材として適切ではありません。たとえば、難易度が難しいとか必要な前提知識が多すぎると適切ではありません。適切な題材となりえるOSSを用意できるかどうかということも課題です。

解決案は…今のところないです。どうするといいでしょうねぇ。

まとめ

今回初めて開催したOSS Hack 4 Beginnersについて内容を考えた立場から紹介しました。OSS Hack 4 Beginnersは続くOSS Hack Weekendのためのイベントでした。OSS Hack Weekendでのびのびと楽しくOSSの開発に参加するために漠然とした不安を解消したのです。

なお、OSS Hack WeedendはOSS Hack 4 Beginnersに参加していなくても参加できます。引き続き優秀なメンターが多数参加するので、OSSの開発に参加したそうな学生さんに教えてあげてください。OSS Hack Weekendでは学生が希望するOSSの開発に参加します。OSS Hack 4 Beginnersはイベント開催側が対象OSSを選びましたが、OSS Hack Weekendでは野生のOSSの開発に参加します。「このOSSの開発に参加してみなよ」という案を持っている方はそれも一緒に学生に教えてあげてください。

  • OSS Hack Weekend:2015-07-11と07-12の2日間開催(07-06申し込み締め切り)

今回の勉強会に参加したメンターの感想に興味のある方は次のWebページを見てみてください。

2015-07-02

2008|05|06|07|08|09|10|11|12|
2009|01|02|03|04|05|06|07|08|09|10|11|12|
2010|01|02|03|04|05|06|07|08|09|10|11|12|
2011|01|02|03|04|05|06|07|08|09|10|11|12|
2012|01|02|03|04|05|06|07|08|09|10|11|12|
2013|01|02|03|04|05|06|07|08|09|10|11|12|
2014|01|02|03|04|05|06|07|08|09|10|11|12|
2015|01|02|03|04|05|06|07|08|09|10|11|12|
2016|01|02|03|04|05|06|07|08|09|10|11|12|
2017|01|02|03|04|05|06|07|08|09|10|11|12|
2018|01|02|03|04|05|06|07|08|09|10|11|
タグ: