コーディング時に大切にしているポイント

はじめに

「きれいなコードを書きたい」「このコードの書き方になにか違和感があるけど、どう直せばよいかわからない」
コーディング時にこのように思われる方は多いのではないでしょうか。
（私はコードの書き方によく悩んでおりました (゜゜)）
そのような時に、参考になるものがあれば！と思いこちらの記事を残しました。
私が業務や日常でのコーディング時・コードレビュー時に大切にしているチェックポイントや、コーディング始めたての頃にこんなことを知りたかった！と思った内容を記載しております。

※あくまで「コードの可読性や保守性を上げるにはどうすると良いのか」に主眼を置いており、本記事では、「パフォーマンスチューニングなどのアルゴリズムの最適化」のようなコーディング技術については記述しておりません。

サンプルコードはSalesforce開発で使用されるApex（Javaライクなオブジェクト指向言語）という言語を使用しております。
どの言語でも通じる部分はあると思うので、別の言語を使用している方は適宜読み替えていただけますと幸いです
（一部、オブジェクト指向ならではの書き方をしている部分があります）

対象読者

• （コーディングを伴う）ソフトウェア開発を始めたての方
• コードを改善したいけど、どう改善してよいか困っている方
• 開発の中でコードレビューを行う方（コードレビュー時の観点を知りたい方）

コメント

コード内のコメント（why notを記述）

• なぜその処理以外の処理を行わなかったのかを記述
• 処理内容をそのまま書くようなコメントは不要（処理内容はコードから理解できるように、変数名・メソッド名・クラス名などを見直すと良い）
• 参考：コードに書けないことのみをコメントにする

（修正前：処理内容のみをコメントしている）

// Aaa項目を上書きする
SObject firstRecord = Trigger.new[0];
firstRecord.Aaa__c = "更新後";
return;

（修正後：処理しなかった理由をコメントしている）

// beforeトリガ後にSalesforce内部で値の更新が行われるため、明示的にはinsertやupdateの記述を行っていない
// insertやupdateの記述を行うとトリガが再帰的に呼び出されてループしてしまう
SObject firstRecord = Trigger.new[0];
firstRecord.Aaa__c = "更新後";
return;

コミットログ（whyを記述）

• なぜその変更を行ったのかを記述

（修正前：変更内容しか記述していない）

メソッド分けを実施

（修正後：変更内容+変更した理由を記述している）

メソッド分けを実施
ループ内でネストが深くなるのを防ぐため

変数

変数名

• 基本的にBoolean以外は形容詞+名詞の形で命名
• 変数名は省略せずに記述

（修正前）

String custSName = "test inc.";

（修正後）

String customerShortName = "test inc.";

• Booleanは「isXXX (isNotXXX)」「shouldXXX (shouldNotXXX)」「canXXX (cannotXXX)」など「Yes/No」が明確になるように命名する

（修正前）

Boolean deleteFlg = false;

（修正後）

Boolean isDeleted = false;

説明変数・要約変数

• （コード行数は多くなってしまいますが）可読性向上のために、以下のようにあえて処理結果を一度変数に入れると良い場面がある
• 説明変数…代入した値や式の意味合いを変数で表現する
• 要約変数…その値の業務的意味合いは何かを変数で表現する

（修正前：説明変数を使用しない）

List<SalesDetails> children = doHoge();
if (!(children == null || children.isEmpty())) {
  // childrenに関する処理
}

（修正前：説明変数を使用する）

List<SalesDetails> children = doHoge();
Boolean hasChildren = !(children == null || children.isEmpty());
if (hasChildren) {
  // childrenに関する処理
}

（修正前：要約変数を使用しない）

if (salesOrderDivision == "unformalOrder") {
    // 内示受注の時は出荷不可のエラーとする処理
}

（修正後：要約変数を使用する）

Boolean cannotShip = salesOrderDivision == "unformalOrder";
if (cannotShip) {
    // 内示受注の時は出荷不可のエラーとする処理
}

スコープ

• 変数のスコープ（変数を使用できる範囲）は出来るだけ小さくして、影響範囲を狭めるのが好ましい
  （影響範囲を小さくすることで、機能修正や機能追加を容易にするため）
• 特にクラス内でメンバ変数とする必要のない変数はローカル変数として定義する

（修正前）

class Hoge {
    private String message;

    public void doHoge() {
        message = "Mr.A";
        String helloMessage = "hello" + message;
        this.doFoo(helloMessage);
    }
}

（修正後）

class Hoge {
    public void doHoge() {
        String message = "Mr.A";
        String helloMessage = "hello" + message;
        this.doFoo(helloMessage);
    }
}

変数の使い回し

• 変数は意味合いが同じである場合を除き、既存の変数を使いまわさず別の変数を用意する
  （変数の中に入っている値を追うのが困難になるため）

（修正前）

Integer size = hogeList.size();
// 何らかの処理
size = fooList.size();
// 何らかの処理
// sizeには「hogeList.size()の結果」か「fooList.size()の結果」かどちらの値が入っているのかが分かりにくくなる

（修正後）

Integer hogeSize = hogeList.size();
// 何らかの処理
Integer fooSize = fooList.size();
// 何らかの処理
// hogeSizeには「hogeList.size()の結果」、fooSizeには「fooList.size()の結果」の値が入っていることが分かりやすくなる

中間変数

• 中間結果を保持するためだけの変数（中間変数）を用いると、処理が不用意に長くなることが多い
  結果的に、処理を追うのに時間がかかり可読性の低下につながる
• このような中間変数は削除できる場面が多いため、中間変数が出てきた時には削除して可読性が向上するかを確かめると良い

（修正前）

// 選択した商品はすべて在庫があることをチェックする処理
public void validateExistingStock(List<Item> selectedItems) {
  Boolean hasNotStock = false;
  for (Item selectedItem : selectedItems) {
    if (selectedItem.stocks.empty()) {
      hasNotStock = true;
    }
  }

  // 1商品でも在庫がない場合はエラーとする
  if (hasNotStock) {
    throw new CustomException("選択した商品の在庫がありません。");
  }
}

（修正後）

public void validateExistingStock(List<Item> selectedItems) {
  for (Item selectedItem : selectedItems) {
    if (selectedItem.stocks.empty()) {
      throw new CustomException("選択した商品の在庫がありません。");
    }
  }
}

メソッド

メソッド名

• 基本的にBooleanを返却するメソッド以外は動詞から始まる形で命名する
• Booleanを返却するメソッドは、動詞だけでなく、変数名の命名と同様に「Yes/No」が明確になるように命名するのも良い

（修正前）

public void userCreation() {
  // ユーザ作成処理
}

（修正後）

public void createUser() {
  // ユーザ作成処理
}

アクセス修飾子

• メソッドのアクセス修飾子（メソッドのアクセス可能な範囲の定義）は出来るだけ範囲を小さく指定して、影響範囲を狭めるのが好ましい
  （影響範囲を小さくすることで、機能修正や機能追加を容易にするため）
• 特にあるクラス内だけでしか使用されないメソッドは private メソッドとして定義する

（修正前）

class Hoge {
    public void main() {
        String message = this.createHelloMessage();
        System.debug(message);
    }

    // Hogeクラス内でしか使用されていないのに、public指定で他クラスからも参照可能な状態となっている
    // createHelloMessageの影響範囲が広いように見えてしまう
    public String createHelloMessage() {
        return "hello";
    }
}

（修正後）

class Hoge {
    public void main() {
        String message = this.createHelloMessage();
        System.debug(message);
    }

    // Hogeクラス内でしか使用され得ない = 影響範囲をHogeクラスに限定できる
    private String createHelloMessage() {
        return "hello";
    }
}

メソッド分け

• 1つのメソッドが長い場合はメソッド分けを行うと、可読性が上がることが多い
  メソッド分けのポイントとして以下を見ると良い
  • 引数が使用されている範囲の把握
  • ループなどでネストが深くなっている箇所の抽出

（修正前）

public void callHoge() {
    this.doHoge(param1, param2);
}

private void doHoge(String param1, Integer param2) {
    // param1はここまでしか使用されていない
    Foo fooInstance = new Foo(param1);
    List<Integer> fooList = fooInstance.main();

    // ループによってネストが深くなっている
    for (Integer foo : fooList) {
        // fooとparam2を使用した長い計算処理
    }
}

（修正後）
可読性の向上だけでなく、分けたメソッド（doHoge や doHogeSecond）がより汎用的になることもある

public void callHoge() {
    // param1のみを使用する部分、param2を使用する部分でメソッド分けする
    List<Integer> fooList = this.doHoge(param1);
    for (Integer foo : fooList) {
        this.doHogeSecond(foo, param2);
    }
}

private List<Integer> doHoge(String param1) {
    Foo fooInstance = new Foo(param1);
    return fooInstance.main();
}

private void doHogeSecond(Integer foo, Integer param2) {
    // fooとparam2を使用した長い計算処理
}

条件分岐

• 条件分岐の if がコード中に多いと、コードを読む時に条件を追うのに時間がかかる・条件内容を何度も確認しなおすことになってしまう
• 以下の様な考え方によって if を用いずに条件分岐の処理をおこなうこと/出来るだけ可読性・汎用性を保って条件分岐を行うことができる

連想配列（マップ・辞書）の使用

• 条件によって値を取得する時に有効
• 以下の例では、if else や switch を使用せずとも条件に応じた値の取得が可能

Map<String, String> englishJapaneseWeekDayMap = new Map<String, String>{
    "Monday" => "月曜日",
    "Tuesday" => "火曜日",
    "Wednesday" => "水曜日",
    "Thursday" => "木曜日",
    "Friday" => "金曜日",
    "Saturday" => "土曜日",
    "Sunday" => "日曜日"
};
String englishWeekDay = "Tuesday";
// 入力値が「Tuesday」であれば「火曜日」を取得したい
System.debug(englishJapaneseWeekDayMap.get(englishWeekDay)); // 火曜日

メソッド呼び出し側で分岐を解決

• メソッド内に存在する条件分岐をメソッド呼び出し元に移動させると、そのメソッドが汎用的になることがある

（修正前）

public void main() {
    this.doHoge();
}

private void doHoge() {
    String message = "";
    if (/*条件1*/) {
        message += "aaa";
    }

    if (/*条件2*/) {
        message += "bbb";
    }

    // messageを使用した処理
    // -> messageに「aaa」や「bbb」の文字が入るような特殊なメソッドになってしまう
}

（修正後）

public void main() {
    String message = "";
    if (/*条件1*/) {
        message += "aaa";
    }

    if (/*条件2*/) {
        message += "bbb";
    }

    this.doHoge(message);
}

private void doHoge(String message) {
    // messageを使用した処理
    // -> messageに「aaa」や「bbb」以外の文字が入る場合にも対応できる
}

メソッド呼び出し側で行う処理を決定

• 機能追加・機能修正時の対応範囲を狭くすることができる
• 機能修正時のテストの範囲を限定することができる
• Strategyパターンとも言う

（修正前）

public void main() {
    this.doByCondition();
}

public void doByCondition() {
    if (/*条件1*/) {
        // 処理A
    } else if (/*条件2*/) {
        // 処理B
    } else if (/*条件3*/) {
        // 処理C
    }
}

（修正後）

public void main() {
    // 呼び出しもとで何の処理を行うかを決定
    String className = "Aaa";
    this.doByCondition(className);
}

public void doByCondition(String className) {
    // 呼び出されるメソッド側ではifの条件分岐が消える
    Hoge hogeInstance = (Hoge) Type.forName(className).createIncetance();
    hogeInstance.doHoge();
}

public interface Hoge {
    void doHoge();
}

// 機能追加・削除・修正時は対象のクラスのみを変更するのみで良い
public class Aaa implements Hoge {
    public void doHoge() {
        // 処理A
    }
}

public class Bbb implements Hoge {
    public void doHoge() {
        // 処理B
    }
}

public class Ccc implements Hoge {
    public void doHoge() {
        // 処理C
    }
}

共通処理

役割ごとに処理を分離（何でもかんでも共通処理に詰め込まない）

• 共通化を行うか否かの判断
  • 「同じ処理であるか否か」ではなく、「同じ意味を持つ処理であるか否か」
    たとえ同じ処理を行っていたとしても、偶然処理が一致しているだけで、意味合いの違い処理は別メソッド・別クラスとして記述する
• 個別処理（一定の状況でしか使用されない分岐処理など）を入れない
  • 何でもできる神共通処理はメンテナンスしづらいため
  • 個別処理は個別側で行う
    参考：Twitter - クソコード動画「共通化の罠」

（修正前）
（機能Aや機能B、機能Cの処理に変更が入ると他の共通処理への影響考慮・再テストが必要→共通処理の変更は影響範囲が大きい）

public void main () {
  this.executeCommonProcess();
}

// いろんな役割の処理を１つにまとめてしまっている共通処理
public void executeCommonProcess() {
  // 機能A（共通的に使われる機能）

  if (/*条件*/) {
    // 機能B（共通的に使われる機能）
  }

  // 機能C（共通的に使われる機能）
}

（修正後）

public void main () {
  this.executeCommonProcessA();

  // 条件分岐は、個別処理側で実行することで、共通処理Bの汎用性が高まる
  // →この条件分岐は、ある意味で個別処理とみなせる
  if (/*条件*/) {
    this.executeCommonProcessB();
  }

  this.executeCommonProcessC();
}

// 役割ごとに処理を分けている共通処理

public void executeCommonProcessA() {
  // 機能A（共通的に使われる機能）
}

public void executeCommonProcessB() {
  // 機能B（共通的に使われる機能）
}

public void executeCommonProcessC() {
  // 機能C（共通的に使われる機能）
}

状態管理を極力行わない

• DML発行（データ作成・更新・削除）など状態変化の伴う処理は個別処理に任せる
  共通処理側で状態管理を行ってしまうと、「まとめてデータ更新したい」という場合に共通処理の呼び出し方が限定されてしまう/パフォーマンス低下などの影響があるため

（修正前）

public void main() {
  for (Integer i = 0; i < 10; i++) {
    // ループ処理の中でDB更新を行ってしまうため、パフォーマンス低下につながる
    this.createUser();
  }
}

public void createUser() {
  User newUser = new User();
  newUser.Name = "Mr.test";
  newUser.Email = "[e-mail]";
  insert newuser;
}

（修正後）

public void main() {
  List<User> newUsers = new List<User>();

  for (Integer i = 0; i < 10; i++) {
    User newUser = this.createUser();
    newUsers.add(newUser);
  }

  // 複数データをまとめてデータ更新
  insert newUsers;
}

public User createUser() {
  User newUser = new User();
  newUser.Name = "Mr.test";
  newUser.Email = "[e-mail]";
  return newuser;
}

参考

• リーダブルコード――より良いコードを書くためのシンプルで実践的なテクニック
• Twitter - コードには How テストコードには What コミットログには Why コードコメントには Why not を書こうという話をした
• プログラマが知るべき97のこと - コードに書けないことのみをコメントにする
• Processingで学ぶ 実践的プログラミング専門課程 第25回　デザインパターン（3） Strategy
• Twitter - クソコード動画「共通化の罠」