C++でコードレビューを受けて気づいたこととか【C++ Advent Calendar 2015】

この記事は、C++ Advent Calendar 2015の12月16日ぶんの記事です。

はじめに

今年10月より、Sapporo.cppの企画として「オセロAIの作成に取り組む」ということを行っていて、私がそのベースとなるシステム（対戦を行わせる部分。打つ手を決める部分はユーザが作成する）を作成していた。
私が一度作成したうえで、他のSapporo.cppのメンバーからコードについてコメントを多数いただいたのだが、

• 自分にとって当たり前だったことが、他者の目には当たり前に見えないことも多々あった。
• というかそもそも、自分はこれまで（仕事含め）基本的に個人でC++を書いていたため、コードレビューを受けることがそもそもなかった。

ということで今回、自分がコメントを頂いて自分が気づいたことや考えたことを紹介する。
なお、AIを作ってみての話は http://qiita.com/h_hiro_/items/f6f1576ba8b19b804468 （Kawaz Advent Calendar 2015、12月15日ぶん）に書いている。

なおこのオセロAI企画は、Sapporo.cppのオンラインもくもく会（おおむね毎週1回実施。上記リンク先を参照）にて継続実施しています。オンラインで参加できますので、もし興味を持った方がいらっしゃったらご参加ください。

ソースコード

• 当初のコード: https://github.com/sapporocpp/othello-battler （こちらのissuesにいろいろ書かれています）
• 全面改訂版のコード: https://github.com/sapporocpp/othello-shell

頂いたコメント

autoは積極的に使うべき？

C++11で規格化された、型を具体的に書く代わりに使う「auto」。

auto foo = 1; // fooはint型と扱われる

autoが必須になる事例としては

• どんな型の値が返ってきても受け取れる変数を定義したい場合（ユーザ指定のクラスをtemplateの引数で受け取る場合など）
• ラムダ式を受け取る変数を定義したい場合（ラムダ式を表す型は明示的に書けないため。ラムダ式に限定する必要がないならstd::functionを使うという手もある）

などがある。また、それ以外でautoが多用される事例としては

• 型が長く複雑になる場合（典型的なものとしてはstd::vector<MyClass>::iterator）
• 関数ポインタを返り値にする場合（autoを使うことですっきり書ける。例：auto specifier (since C++11) - cppreference.com）

というものがある。

さて、私は後者の1番目「型が長く複雑になる場合」については、autoは使わないことにしている。その理由として、

• デバッグの際にその型が何であるか知りたいとき、変数宣言の右辺値（auto foo = bar;におけるbar）の定義まで戻る必要があり、1ステップ余計になる
• 変数宣言の右辺値の型を変更した場合、コンパイルエラー発生箇所が変数宣言の場所になるのでわかりやすい

というものがある。

これに対して、積極的にautoを使ったほうがよいのでは、というコメントもあった。理由としては

• 型名が長い場合、すっきりかける
• 型名が長い場合、それだけではなく、自分でどんな型なのかいちいち調べないとならない場合もある。autoを使えばそれを回避できる。
• 未初期化変数がなくなる

というものがあった。

これはどちらがよいかという結論は出ず、私の実装であるautoを基本的に利用していないものが残っている。

オセロの石をenum classで表現する件

当初、オセロの石は単なるchar型とし、

const char BLACK = 1; // 黒が置かれている
const char WHITE = 2; // 白が置かれている
const char EMPTY = 0; // 石は置かれていない
const char INVALID = 4; // 無効な座標を指定した場合など

のようにしていた。これに対して、C++11で規格化されたenum classを使えばよいのでは、というコメントを頂いたのでそのように実装。

enum class Color : char {
    BLACK = 1, // 黒が置かれている
    WHITE = 2, // 白が置かれている
    EMPTY = 0, // 石は置かれていない
    INVALID = 4 // 無効な座標を指定した場合など
};

従来からのenumと違って、intではなく個別の型として扱われるので、意図せず単なる整数型から代入してしまうことを防げる（static_castが必要）ほか、元になる型が指定できるので（今回はchar）、メモリ節減にも役立つ（オセロの盤面を表現するのに用いる値であるため、これを何度も複製して使うことを考えると、メモリは節減したかった）。

さてこれで解決…と思いきや、Visual Studioでは2012以降でしか動かないということで、代替となる実装を考えることに。これについて、私は消費メモリをcharのままにするということを重視したかったため、

class Color {
private:
    char val_;
public:
    Color();
    Color(char val);
    operator char() const;
    bool operator ==(Color other) const;
    bool operator <(Color other) const;

    static const char BLACK;
    static const char WHITE;
    static const char EMPTY;
    static const char INVALID;
};

const char Color::BLACK = 1;
const char Color::WHITE = 2;
const char Color::EMPTY = 0;
const char Color::INVALID = 4;

という実装をした。ただこれに対し、「enum classに対応してないコンパイラには単なるenumのものを提供すればよいのでは。メモリの使い方は変わるかもしれないが、同じことをするのにコードが複雑になりすぎ」という意見も頂いた。これは結局話が平行線となり、上記のままの実装が残っている。

intがいい？size_tがいい？

私の実装においては、オセロの盤面の座標値（縦が0から7、横も0から7）を表す型はintで統一していた。これに対し、「座標値は負の数を取り得ないものなのだから、size_tのほうがよいのでは？」というコメントをいただいた。

これには明確な理由があって、オセロの盤面の処理においては「はみ出した場所の座標も利用できる」と計算が楽になるためである。
例えばオセロでは石を取る処理等において「同じ色の石がどこまで続いているか」を判定するのだが、下の図で「黒石は座標(3, 2)から左下に向かってどこまで続いているか」を判定することを考えると、座標の変数に-1を使ってよい場合とそうでない場合では、処理の書き方の簡潔さが段違いなのである。

intがいい？int32_tがいい？

さて、この実装においては、座標値を表す型はintで統一していた。これに対し、「intよりもint32_tのような大きさが明示された型のほうがよいのでは」というコメントをいただいた。

これについては私がコメントするのは難しかったのだが、他の方々による議論は結局平行線で、そのままintの実装が残っている。議論としては以下のようなものがあった。

• int32_tなどを使うべきという主張
  • サイズのせいで意図しない挙動を起こさないようにすべき
  • プログラマーがサイズを明示できる場合はそうすべき
• intを使うべき/intでよいという主張
  • 大きさが問題にならないのなら、コンパイラが勝手に選んでくれる大きさにするのがよい（環境依存性を自分で定義しないで済むのならそれに越したことはない）

オセロAIのユーザコードを読み込む実装

オセロの対戦を管理するプログラムと、オセロAIのユーザコードをどう合わせるかという問題。

最初、私は「AIに相当するコードを#includeで読み込むのがよいのでは」と言われたのだが、「2つのAIで関数名が重なった」などの場合に対応できるいい方法が思いつかず、以下の方法で実現していた。

• AIは、対戦を実行するプログラムとは別のバイナリとして出力される。
• 対戦を実行するプログラムが、AIに相当するバイナリを実行し、標準入出力を使った通信によってAIの思考結果を得る。

しかしそれだと、エラーが発生した（AIが不正な値を返したなど）場合にデバッグがしにくいという問題が発生、改めて「AIに相当するコードを#includeで読み込む」実装について検討した。

結局できたのは以下のコード（それぞれ冒頭を読んでいただければと思います）。

• オセロの対戦実行：othello_match.cpp
• オセロのAIの例：othello_sample_LV2A.hpp

AI側は事前に定められたクラス名OTHELLO_AIでクラスを作るものとし、それを読み込む際は

• 先攻のAIについては #define OTHELLO_AI OthelloAI1 を先に記述した上で#includeする
• 先攻のAIについては #define OTHELLO_AI OthelloAI2 を先に記述した上で#includeする

という方法を採用した。

一時的に使う処理であっても気を抜かず

以下のようなコメントを頂いたのだが、共通して言えることは、私は「一時的に使う処理だと、意味論等よりも簡単に書けること」を重視しちゃうんだよなーということ。気を抜いてはいけないですね、はい。

• 変数の意味が一見してわからないものには、一文字の名前等わかりにくいものを付けない
• 仮に一時的に使う変数であっても、その値が代入された以降更新されないなら、constを付ける
• 仮に内部的だけに使う関数であっても、多数の変数を返り値にする関数は、値を複数の参照渡しの引数で返すのではなく、構造体経由で返す

こんな関数あったんだ！

C++らしい方法を知らず、C言語らしく書いていた箇所に対してあったコメント。

• std::getline：「Enterキーを押したら終了」をfgetsで実装していた自分に対してのコメント。だいぶ前に使ってはいたのだが完全に忘れていた。（参考：getline - cpprefjp C++日本語リファレンス）
• std::stoi：C++11で規格化された、strtolの「C++らしい」版（参考：stoi - cpprefjp C++日本語リファレンス）。単純に知らなかった。ただ、stoiはどうやらmingwでは使えないようで、結局strtolに戻している。

おわりに

コメントをくださった皆様ありがとうございました。精進します。