「Good Code, Bad Code」を読んだ。 コードを書く上で気をつけるべきことが簡潔にまとまっているので読みやすかった。 折に触れて読み返したい内容になっていたと思う。
せっかくなので現在仕事で使用しているFlutter（Dart）で記載されているプラクティスを実践してみる
本書の概要
まず本書についてざっくり説明する。 本書では「どのように悪いコードを良いコードへ直すか？」というテーマを扱っている。
構成は大きく3つに分かれている。
- 理論編
- 実践編
- ユニットテスト編
理論編
良いコードとは何か、どのような考え方でコードを書くべきか、といった内容に触れられている。
以下のフレーズについて気になる場合は読むと良い。
- ユーザーのメンタルモデルへの注意
- コードは正しく使われるとは限らない
- 抽象化レイヤーのメリット
- コードの再利用、汎用化
- 「このコードはどのようにテストするのか」という自問
実践編
理論編で説明された内容をどのようにコードに落とし込んでいくかが説明される。 以下のテーマで説明される。
- コードを読みやすくする
ユニットテスト編
コード品質を一定以上に保つテストの実践方法が説明される。
実践編の適用例
コードを読みやすくする
名前は機能の要約
変数名やクラス名は機能を端的に要約したものになるべき。 例えば以下のクラスは何を表しているか非常にわかりにくい。
class U { String n; int a; double w; double h; U(this.n, this.a, this.w, this.h); double getB() { return w / pow(h, 2); } }
なので次のように名前を変えれば、意味を理解するのは非常に簡単だろう。 名前を変更しただけである。
class User { String name; int age; double weight; double height; User(this.name, this.age, this.weight, this.height); double getBMI() { return weight / pow(height, 2); } }
コメントはわかりにくい名前の代替案になるべきではない
上で示した U クラスをわかりやすくするためにコメントで補足するのは望ましくない。
なぜならコメントをメンテする必要が出るし、何よりクラスを使用する際に、一目で意味がわからないことは変わりないからだ。
以下はコメントをつけた例だが、避けた方がいいだろう。
/// ユーザー class U { /// 名前 String n; /// 年齢 int a; /// 体重 double w; ///身長 double h; U(this.n, this.a, this.w, this.h); /// ユーザーのBMIを取得する double getB() { return w / pow(h, 2); } }
コメントはなぜそのコードが存在するかを説明すると良い
クラスが作成された背景などはコードを見てもわかりにくい。 その場合、なぜそのクラスが存在しているのかをコメントで補うといい。
逆に非常に具体的な処理をしている箇所では、変数名などが用途を説明できるのでコメントは最小限にとどめる方が読みやすくなる。
コメントすべき例（背景を説明）：
// 実年齢をそのまま返すとモテないため2歳低くする // 上記の仕様を決定したチケットurl→https://... int get publicAge => _age - 2;
コメントを避けるべき例（具体的な処理の説明）：
// 実年齢から2を引いてint型で値を返す int get publicAge => _age - 2;
冗長なコメントはつけない
コードを見ればわかるコメントは不要。 以下は不要な例。
String greeting(){ // "Hello I am 名前 , 年齢 years old"という文字列を返す return "Hello I am $name , $age years old"; }
コードは1行にまとめれば良いというわけではない
コードを簡潔に書くことは良いことだが、あくまで理解しやすい範囲で簡潔に書くべき。 例えば以下のコードは記述自体は簡潔だが何をしているかがわかりにくい。
void main() { var l = [1,2,3,4,5]; print(l.where((e)=>e%2==0).map((e)=>e*2).toList()); ... }
これはリスト l の要素のなかで偶数のもののみを2倍にして新しいリストを作成するコードである。
コンパクトだが見にくい。そこで次のように書き換えると見通しが良くなる。
void main() { var numbers = [1, 2, 3, 4, 5]; var doubledEvens = getDoubledEvens(numbers); ... } List<int> getDoubledEvens(List<int> list) { return list .where((number) => number % 2 == 0) .map((evenNumber) => evenNumber * 2) .toList(); }
コーディング規則を守る
例えばDartの場合、クラス名は大文字で始めることが一般的である。 もし小文字で書かれていたら何かの変数かと見間違える可能性がある。 なのでコーディング規則を守ることで保守性を高められる。
もっとも、IDEなどが規則に反した書き方をした際にメッセージを出すので それに従いコーディングをすれば良い。
ネストは深くしない
ifなどでネストが深いコードは読みにくい定番の書き方である。
void checkUser(User user) { if (user != null) { if (user.isActive) { if (user.age >= 18) { print("User is active and 18 or older."); } else { print("User is active but younger than 18."); } } else { print("User is not active."); } } else { print("User is null."); } } class User { bool isActive; int age; User({required this.isActive, required this.age}); }
checkUser 内でネストが3階層になってしまっている。
このような場合、ガード節などで早期に値をリターンさせてあげると良い。
void checkUser(User? user) { if (user == null) { print("User is null."); return; } if (!user.isActive) { print("User is not active."); return; } if (user.age < 18) { print("User is active but younger than 18."); return; } print("User is active and 18 or older."); } class User { bool isActive; int age; User({required this.isActive, required this.age}); }
これでネストが1階層に抑えられ、ぐっと見やすくなった。
ガード節でリファクタリングできない場合は処理を見直す
メソッドが一度に複数のことを行なっていると、ガード節を使いづらいケースがある。
void registerUserAndSendEmail(User user){ if (user.address in CustomerList.items){ final result = _sendEmail(user.address); if(result){ log("success!"); } else { throw Exception("send email failed."); CustomerList.remove(user); } } else { final result = _registerUser(user); if(result){ log("registration succeed!"); } else { throw Exception("registraiton failed"); } } }
上記のコードはユーザーが顧客リストに含まれているかを確認し、含まれていればメールを送信、含まれていなければユーザーを登録するコードである。 上記のようにネストがやや深くなり始めており、以下のように分割するといいだろう。
void sendEmail(User user){ final shouldSendEmail = checkCustomerList(user); if(!shouldSendEmail) { _register(user); return; } _sendEmail(user.address); }
上記は一部しかコードを書いていないので、実際は後者の方がコード量が多くなるが、読みやすさや保守性、機能の分離は明らかに後者の方が良くなっている。
関数には名前つき引数を使う
関数に引数が複数ある場合、それぞれの引数が何を表しているかは重要。
しかし以下のような関数は意味が分かりづらい。
void createUser(String name, int age, String email, String address, String phoneNumber, String password) { // 何らかの処理 print('User created: $name, $age years old, $email, $address, Phone: $phoneNumber'); } // 使用する際は引数が何を表しているか分かりにくい createUser("tom", 20, "mail@com", "tokyo","000000", "tomtom");
そこで名前つき引数を用いる。
void createUser({ required String name, required int age, required String email, String? address, String? phoneNumber, required String password }) { // 何らかの処理 print('User created: $name, $age years old, $email, ${address ?? 'N/A'}, Phone: ${phoneNumber ?? 'N/A'}'); } // 引数が何を表しているか分かる createUser( name: "tom", age: 20, email: "mail@com", address: "tokyo", phoneNumber: "000000", password: "tomtom");
enumを使用して意味が伝わりやすくする
例えばUserクラスのメンバ変数として利き腕を表すdominantHandがあったとする。
この場合以下のようにすると入力ミスなどが起きやすい。
class User { /// 受け取る値は "right" or "left" String dominantHand; ... }
そこでenumを使って以下のようにすると分かりやすい。
enum DominantHand{ right, left } class User { /// 受け取る値は "DominantHand.right" or "DominantHand.left" DominantHand dominantHand; ... }
enumを宣言する手間がかかるが、入力ミスが発生する可能性がぐっと減る。 また聞き手によって処理が分岐する場合でも、switch文が使いやすくなる。
マジックナンバーは定数として宣言する
一般的にマジックナンバーはコードを読みにくくする元である。 以下にマジックナンバーが含まれている分かりにくいコード例を書く。
void main() { var salary = 50000; var bonus = calculateBonus(salary); print("Total amount: ${salary + bonus}"); } double calculateBonus(double base) { return base * 0.15 + 2500; }
calculateBonusに含まれる0.15や2500の意味がわからず、コードを修正するとき怖い。
そこでこれらを定数化して意味をわかりやすくする。
void main() { const double SALARY = 50000; var bonus = calculateBonus(SALARY); print("Total amount: ${SALARY + bonus}"); } double calculateBonus(double base) { const double BONUS_RATE = 0.15; const double BONUS_BASE_AMOUNT = 2500; return base * BONUS_RATE + BONUS_BASE_AMOUNT; }
0.15はボーナス率、2500はボーナスのベース額であることがすぐにわかる。 また定数を宣言する場合は、dartでは大文字で記載する。
長すぎる無名関数は小さい単位に分割する
関数名は処理の内容を推測する際に役立つが、無名関数は名前がないため処理を理解するにはコードを読むしかない。 しかし長すぎる場合は理解するのに時間を要する。
以下は長すぎる無名関数の例である。
void main() { var items = ['apple', 'banana', 'cherry']; items.forEach((item) { // 無名関数ここから print("Checking item: $item"); if (item.contains('a')) { print("The item contains the letter 'a'."); if (item.length > 5) { print("The item has more than 5 characters."); } else { print("The item has 5 or fewer characters."); } } else { print("The item doesn't contain the letter 'a'."); } print("----"); }); // 無名関数ここまで }
戻り値にマジックナンバーを使わない
戻り値は開発者が想定できる範囲の値を返すべきである。
以下の関数は戻り値に、一見すると意味の不明な-1という値を返している。
int getNumber(){ int result; // 何かの処理 if (/* 何らかのエラー判定 */){ return -1; } return result; }
この関数を使う場合、内部の処理を確認しないと-1がエラー発生を意味するとわからない。
なのでこういう場合はnullを返すべき。
nullであれば戻り値を利用する際に値の判定が必要だし、何らかのエラーが起きたことを推測しやすい。
int? getNumber(){ int result; // 何かの処理 if (/* 何らかのエラー判定 */){ return null; } return result; }
副作用は明確にする
関数名は処理の要約なので、関数名に含まれない処理は極力避けるべきである。 特に他のデータを操作する際は注意が必要。
以下は関数名に含まれていない処理を行なっている例である。
class UserData { static List<String> data = []; String get data { // DBに接続 final db = connectDB(); // DBからデータを取得 data = db.getData(); return data; } }
getメソッドのdataは、実はDBに接続してデータを取得している。
名前からするとdataをそのまま返して来そうだが、実は違う処理を行なっている。
このgetメソッドを一度呼ぶだけなら特に問題には感じないが、多数回連続で呼ぶ場合（例えば1万回など）、 1万回dbとのコネクションを作成してしまうのでかなりのオーバーヘッドが生じる。
なのでメソッド名を変えるか、処理内容を見直すほうがいいだろう。
class UserData { static List<String> _data = []; // メソッド名を変えた場合 String getDataFromDB() { // DBに接続 final db = connectDB(); // DBからデータを取得 data = db.getData(); return data; } // 処理を変えた場合 String get data => _data; }
