はじめに
プログラミングビギナーのコードレビューをする機会があり、いつものように指摘を入れていたのですが、ふと自分がどのような観点や考えをもってコーディングしているのかまとめといた方がいいなと思い、執筆してみました。
プログラミングビギナーに読んでもらう資料として活用できたらいいなと思ってます。
こうしなさいというルールではなく、あくまで考えてたり、注意していることです。あってほしくないが、状況によって例外はあると思います。
コーディング規約を守る
ルールは守ろう！
可読性、保守性、品質が高くなるように設計されているはず。
一貫性を持たせる
特にコーディング規則がなかったとしても、自分の中でルールを定めて統一する。
プログラムのふるまいなども同様で、結果は同じでも似たようなロジックを書くときにＡモジュールとＢモジュールで違うふるまいをしていると問題も見つけづらいし、横展開で改修しづらくなる。(共通化したらいいのでは？という指摘は一旦置いといて…)
よくある？例
- インデントが半角スペースだったりタブだったり
- インデントの半角スペースの数が2個だったり4個だったり
- 改行コードが LF だったり CRLF だったり
- セミコロンがあったりなかったり
- 非同期処理に async/await を使用したり Observable を使用したり
同じロジックは共通化する
何度も同じロジックは書かない！
もう何度も言われてるし、言ってる気がする。
共通化することで問題発生時にも横展開しやすい。
同じロジックを書かないことで記述量が膨らむのも防止できる。
変数のスコープを正しく理解し最適なものを選択する
変数のスコープはできるだけ狭くするとはよく言われているが、脳死で狭くすればいいというわけでもない。
問題発生時の影響範囲を狭くしたい気持ちは分かるが、スコープが狭いせいで変数が乱立する、インターフェースが複雑化するのはもっとBADな気がする。
privateやstaticなどのキーワードを使用して最適なスコープを選択しよう。
スコープを選ぶ際に考えること (パターンが確立したらチャートみたいなの作りたい)
- 当該ブロック以外で参照するか
- 当該インスタンス以外で参照するか
- 当該クラス以外で参照するか
命名は正しい英語を使う
ローマ字やスペルの間違い、英語にはない単語はNG。
よくある？例
- スペルミス
NG: DataEditer
OK: DataEditor - 英語にはない単語
NG: registData
OK: registerData
参考
シンプルかつ直観的な記述を心掛ける
複雑な記述は品質に影響するし、レビュワーに優しくない。
改善例
// NG: 直観的でないのでできればやめたい (昔ながら感あるよね)
for (let i = 0; i < collection.length; i++) {
console.log(collection[i]);
}
// OK: コレクションの要素をループして参照していることが直観的に分かる (foreach)
for (const item of collection) {
console.log(item);
}
// NG: ネストが深い
function getFirstString(collection: string[]): string {
if (collection) {
if (collection.length !== 0) {
if (collection[0]) {
return collection[0];
} else {
return "";
}
} else {
return "";
}
} else {
return "";
}
}
// OK: 参照できないケースを先にリターンすることでネストを浅くできる
function getFirstString(collection: string[]): string {
if (!collection) {
return "";
}
if (collection.length === 0) {
return "";
}
if (!collection[0]) {
return "";
}
return collection[0];
}
// OK: それぞれにエラーハンドリングがないならブロックも少なくしたい
function getFirstString(collection: string[]): string {
if (collection && collection[0]) {
return collection[0];
}
return "";
}
// OK: 三項演算子でも書けるけど嫌いな人いそう
function getFirstString(collection: string[]): string {
return collection && collection[0]
? collection[0]
: "";
}
// NG: ANDとORが入り交じっていて複雑
if (someCondition1 && (someCondition2 || someCondition3)) {
console.log("some process")
}
// OK: ANDはあえてネストするなどの工夫をする
if (someCondition1) {
if (someCondition2 || someCondition3) {
console.log("some process")
}
}
コストのかかる処理はできるだけ少なくする
ここでいうコストはお金だけでなく実行時の処理負荷、メモリ使用量も含みます。オーバーヘッドがあるロジックはなるべく少なくするなどの工夫があるとよい。どちらかといえば設計の話にはなるが、コーディング時に気づくこともある。
コストのかかる処理の例
- 標準入出力、ファイル入出力
- DBアクセス
- プロセス間連携やWeb APIなどの他システム連携
既存のコードは触らない
リファクタリングでもない限り、実績あるコードは正義とする。
(言語仕様として確実に影響がないことを説明できるものは触っちゃうかも)
リファクタリング衝動にかられたときに読む記事
コピペの時でも一行一行に責任をもつ
適当にコピペだけして完成としない。
変数名は適切？ホコリ被ったダメコードじゃない？
(コピペでいけるということは共通化できるかも検討してみるとよいかも)
nullチェックする
nullになり得るオブジェクトを参照する前には必ずnullチェックする。
もしくはnullにならないように設計するなどでガードしよう。
一クラス、一メソッドに仕事を持たせすぎない
これもどちらかといえば設計の話かもしれないが、一クラス、一メソッドに仕事を持たせすぎると、記述量が膨大になり、可読性が悪くなってしまう。また、仕事をたくさん持ったモジュールは名前が汎用的になりがちで、名前から直観的に機能が分かりづらくなる。
おわりに
思いつきや先輩方からのアドバイス次第で加筆修正していこうかなと思ってます。
次はコードレビュー中や設計中に考えていることを執筆したい。