要約を全編に渡って書こうと思ったけど序盤で力尽きた…続きはあなたの目で！
=================================================
■ ルール1
できるだけ単純であるべきだが、単純化してはいけないあまりに汎用的な処理を書こうとしてプログラムが複雑化していないか？
解決すべき問題を単純化したり制限をかけることでプログラムを単純に保つことができないか？本書ではとある計算処理において桁数の限界を取っ払うために補助クラスを作成する例を挙げた。
しかし、そもそも桁数の限界を取っ払う必要ある？
桁数に限界を設けてASSERTしちゃえばコードをシンプルに保てるでしょ？
という提言。単純なコードは読み易い。本のように最初から最後までまっすぐ読み通せる。
=================================================
■ ルール2
バグは伝染する開発者Aが作成したclass Hoge;を開発者Bが使用してclass Foo;を実装したとする。
しかし実は class Hoge にはバグが 含まれていた。
ところが、開発者Bはその挙動が開発者Aの意図したものだと思い込んでバグったまま class Foo を実装してしまった。
更に開発者Cがclass Fooを使って別の機能を実装し……という感じで、 class Hoge; のバグはどんどん伝染して絡まる。
class Hoge; のバグを直したとたんに開発者B,Cが作成したコードは正しく動かなくなる可能性がある。さて、class Hoge; のバグを早期に発見してバグの伝搬を防ぐには？？？
・バグったコードを放置すると、どんどんコードが絡まるからバグを早期発見できるようにすべき
・バグを早期発見するためにテスト駆動しようよ！
・じゃぁ、テストしやすいコードって？
・ステートレスな関数は自動テストしやすい。
・テストしやすくしようと考えると、必然的に単純なコードを書く羽目になる。
・ステートを保持する必要があるクラスには監査関数(Hoge::audit())を用意する。
friend で内部変数をテストに公開してテストコード側でチェックを行うより保守が楽。
// いい例ではないかもしれないが…
void Hoge::audit() {
// ステートがIdleの時は _data にはデータが入っていないはずである etc...
Assert(_state == State::Idle && _data == nullptr);
}
・状態の不正、使われ方の不正をテストやASSERTで検知しやすいような設計を心がけよう。
=================================================
■ ルール3
優れた名前こそ最高のドキュメントである・打鍵数をケチって短すぎる変数名を付けるのを避けよう。ちゃんと意味の分かる変数名にしよう
・規則を混ぜない
例えば、STL式のコンテナと独自のコンテナを混在して使う時、メンバ関数名と意味を揃えないと混乱が生じる
・厳格で機械的な命名規則を徹底させよう
誰が書いても必然的に同じ命名になるはずだ。
他の人が書いたコードを読み易くなる
ハンガリアン記法も徹底すればそんなに悪いものじゃないぜ？(他の命名規則と混じると途端にアレだけど)=================================================
■ ルール4
一般化には3つの例が必要・YAGNI
https://ja.wikipedia.org/wiki/YAGNI
ある1つ目の「赤い標識を見つける」という要件に対して Sign* findSign(const vecotr<Sign*>& signs, Color color);という関数を書くより
Sign* findRedSign(const vecotr<Sign*>& signs);と書くべきだというような話。
将来的に「任意の色を指定して標識を見つける」という処理が必要になるかを現時点から想像しながら書くのは無駄になる確率が高い。
2つ目の「position の近くの標識を見つける」という要件が出てきた場合も一般化するのはまだ早い
Sign* findSignNearPosition(const vecotr<Sign*>& signs, const Location& location, float maxDistance); を用意するだけにする。
3つ目の「赤くて、かつposition近くの標識を見つける」という要件が来て初めて一般化を考える
Sign* findSignByColorAndPosition(const vecotr<Sign*>& signs,
const Color& color = Color::Invalid,
const Location& locaton = Location::Invalid, float maxDistance = FLT_MAX);
を用意する。
こうした場合でも元の関数を消す必要はない。
相変わらず位置だけを条件にしたい場合は findSignNearPosition を使えばよい。・YAGNIよりひどい。。。
汎用化をさせすぎると機能を拡張するたびにどんどん複雑になり、呼び出し側のコードも複雑になる。
後者の方がシンプルで拡張もしやすくね？？って話。
// 青か、メインストリートに近い標識を取得する
vector<Sign*> signs;
FindQuery* colorQuery = new FindQuery();
query->colors = { Color::Blue, };FindQuery* areaQuery = new FindQuery();
query->areas = { mainStreetr, };FindQuery* query = new FindQuery();
query->queries = {colorQuery, areaQuery, };
query->boolean = Boolean::Or;
vector<Sign*> results = query->DoQuery(signs);
or
// これでええやん？
vector<Sign*> signs, results;
for(Sign* sign : signs) {
if(sign->color == Color::Blue || matchArea(sign, mainStreet)) {
results->push_back(sign);
}
}
=================================================
■ ルール5
最適化に関する教訓その1は、「最適化するな」
・とにかく最適化すしない！を肝に銘じる
素直で読み易いコードを単純明快なコードを書こう
ロジックが単純でメモリアクセスが素直な方が速い場合も多い
単純なコードをは最適化しやすいから単純なコードを書け。
・最適化手順
1) 計測する
2) バグがないか確認する
そもそも想定と違う関数の呼び出し方をしていないか？など
3) データを計測、観測する
関数の引数、扱ってるデータがどんなものか観測する
場合によっては何回も行っている計算やメモリ確保を減らすことができる可能性がある
4) プロトタイプを作る
最適化対象の関数が複雑な計算をしているとして、その処理を全部取っ払って単純な結果を返す関数に置き換える。
最適化対象の関数のコストを0に近づけた時に、高速化の結果が期待通りになるかどうかを先に確認する。
メモリアクセスのキャッシュ状況などが変わって、結局別の場所の処理負荷が高まるかもしれない。
5) 最適化をする
「同じ処理を高速にする」より「やることを減らす」方向で。
関数を呼び出すたびに同じ結果になる処理を繰り返してるかもしれない...etc
=================================================
■ ルール6
コードレビューが役に立つ3つの理由・コードレビューをやる理由
1) 誰かが見ることが分かっていれば、誰しもより良いコードを書くようになる
・最大の利点である。
・ハックややっつけで実装することが減る
・整形や命名に気を使うようになるだろう。
・ちゃんとコメントを書くようになるだろう
2) チェックイン前にバグを検出できる
3) チーム全体に知識を広げる優れた手段
・バグではないが、適切でない実装を指摘する
(一般化が早すぎる、単純な問題に対して複雑な実装、コード整形)
・シニア(ベテラン)からジュニア(新人)に対しての教育の機会にもなる
・ジュニアがシニアへの質問を通して、シニアの知識の整理にもなる
・シニア同士のレビューではバグの検出の良い機会にもなる
・ジュニア(新人)同士のレビューは禁止！
・禁断のコードレビュー
知識の伝達もなく、バグの発見にも至らない
妙なパラダイムを取り込んでしまい、それが公式方針と誤解を招く結果となる・コードレビューの価値は費やす時間と労力、やり方で決まる
・基本的にはレビューは対面で行う
・レビュイーは変更点の説明を行う
・レビュアーは納得いくまで質問や提案を行う
・５分で済む場合もあれば何時間もかかることがある
・レビュイーは質問、提案をすべて記録し、ほとんどの場合修正する義務がある
・通常はレビューは１回ですむ。
・レビュー、変更追加の取り込み、コミット、という流れになる。
・レビューによってバグが発見される過程
・レビュイーはレビュー前に変更差分を改めて確認する
・その時点で恥ずかしい間違いや、誤りを修正することになる
・レビュイーが変更点やアプローチの内容についてレビュアーに説明する
・その結果、認識の誤りをレビュイー自信やレビュアーが気づく
・レビュアーが変更点を眺めるだけでバグを見つけることは稀。
・とはいえ、コードレビューだで全てのバグを検出できるかではない
バグの検出がレビューの最大の目的ではない。=================================================
■ ルール7
失敗が起こる場合をなくす・関数の引数が多いのは要注意
・以降も引数が増えることが容易に想像できる。
・例)
// 引数が多すぎるし、bool値などはどんな意味かヘッダを見に行かねばわからない・・
const Param param(3 /*LineWidth*/, Color::Red, true, false, false);// 初期化はデフォルト値をセット。デフォルト値から変更したいものだけセット
Param param;
param.SetLineWidth(3);// param を constに。メソッドチェインイディオム
const Param param = Param().SetLineWidth(3).SetColor(Color::Red).SetShadow(true);・間違った使い方が不可能なインターフェース設計をする
・例) メソッドの呼び出し順序が想定されている
// Commit()した後に、SetLineWidth()が、Commit()前にdrawSphere()が呼べてしまうインターフェース
Draw draw;
draw.SetLineWidth(3);
draw.SetColor(Color::Red);
draw.Commit();
draw.drawSphere(....);
// 呼び出し順序が強制されるインターフェース
const Param param = Param().SetLineWidth(3).SetColor(Color::Red);
Draw draw(param);
draw.drawSphere(.....);
=================================================
■ ルール8
実行されていないコードは動作しない教訓：
・使わなくなった関数はガンガン削除していけ
・使われなくなった関数は最初は無害だが、他の箇所の機能追加によるコードの変化に追従しなくなり、バグの温床となる。
・「使われなくなった関数を正しい状態に保つ」というのは現実的ではないしコスト的にも無駄。
・実行されなくなった時点で削除するべき。
・バージョン管理ソフトで、いつでも削除したコードを元に戻せるのだから恐れない。実例：
// 次のように味方、敵の一覧を取得する関数が定義されているクラスがあったとする。
class Person {
bool IsEnemy(const Person* otherPerson) const;
void FindAllies(vector<Person*>* allies)const;
void FindEnemies(vector<Person*>* enemies) const;
static vector<Person*> s_persons;
};
// その後「視界に入っている味方の一覧取得」という要件が追加され、FindAllies()が未使用になったとする
struct IsVisibleAlly {
IsVisibleAlly(const Person* person) : m_person(person) {}
bool operator() (const Person* otherPerson) const {
return m_person != otherPerson
&& !m_person->IsEnemy(otherPerson) // 味方
&& IsClear(m_person, otherPerson); // 視界内
}
};
class Person {
// これは使われなくなった。
void FindAllies(vector<Person*>* allies)const;
// テンプレートを使って一般化
template<class COND>
void FindPersons(COND condition, vector<Persons*>* persons) const {
for(auto person : s_persons) {
if(condition(person)) {
persons->push_back(person);
}
}
}
};
vector<Persons*> allies;
player->FindPersons(IsVisibleAlly(player), &allies);こうなった時点で Person::FindAlies() は無用となる。
今後のPersonクラスの機能拡張によりFindAlies()を実装した時点での前提条件が崩れてバグの温床となるが、呼ばれないためバグが顕在化しずらい…
時間が経ってバグが混入していることに気づかずに、他の誰かがPerson::FindAlies()を呼んだときに、原因不明の良くわからないバグが発生してしまう。
=================================================
■ ルール9
集約可能なコードを書け分かりやすいコードを書け！
短期記憶に収まる認知負荷が低いコードを書こう
・変数名を適切に付ける(int xx; って中身はなんだったかな？を記憶しておく必要がない)
・コードブロックが何をしているのかを簡潔に記述する。
言い換えると、簡潔にコメントできないコードブロックは複雑すぎるということ。
・何でもかんでも関数化・抽象化するのは正義か？必ずしもそうではない。
関数化、抽象化する前に立ち止まって考える
その変更の結果、単純で理解しやすくなるか？
int sum = 0;
for(auto value : values) { sum += value; }
を
int sum = reduce(values, 0, add);
としたところで、わかりやすくなるとは限らない。
とはいえ、「// valuesの合計を求める」とコメントがあれば認知負荷を下げることができるかもしれない。・複雑なコードも抽象化された複数のコードブロックの集合であり、それをパズルのように組み合わせた結果である、と理解しやすいコードが読み易い。
=================================================
■ ルール10
複雑性を局所化せよアプリケーションが巨大になればコードが複雑になるのは避けられない。
だが、複雑性を封じ込めることはできる。
インターフェースを単純にすれば、その中身がいくら複雑でも外部(インターフェース使用者)からは単純に使用することができる。
例えば、sin(), cos() のように。コードに機能を１個追加しようとしたときに、書き換える場所が多すぎる(3か所以上)の場合は警戒する。
コードが局所化されていない可能性がある。
・例えば、ステータスと敵の数を条件にUIの表示を切り替える処理があったとする。
・そこに、天候の考慮を追加するとする。
・その際に、条件判定をする個所を３カ所以上直す必要があるようなコードはおかしい。
・関数を用意し、そこに、ステータス・敵の数・天候からUI状態を決定する判定を記述する、などの対応を検討する。
