Logo開発品質ガイドライン

コーディングガイド

開発のフローの中で最も重要な工程であるコーディングについて、気をつけるべきことについて解説します。

コードを書く際に意識すべきこと#

責務を意識する#

関心の分離#

関心の分離とは、ソフトウェア工学においては、プログラムを関心（責任・何をしたいのか）毎に分離された構成要素で構築することである。

引用：wikipedia

優秀なエンジニアは、そのクラスが何をすべきで何をすべきではないのかを意識してコードを書いています。責務を意識しないで書いたコードは、分かりづらくメンテナンスしにくコードとなるでしょう。プルリクエスト時に指摘を受け、大きな出戻りが発生します。

単一責任の原則#

単一責任の原則 は、プログラミングに関する原則であり、モジュール、クラスまたは関数は、単一の機能について責任を持ち、その機能をカプセル化するべきであるという原則である。

引用：wikipedia

クラスと同様に関数についても責務も意識する必要があります。関数が処理すべき内容を１つに絞ったり、主目的でない処理をカプセル化し隠蔽することで、見通しのよいコードになります。

では、具体的に「関心の分離」と「単一責任の原則」を守った良い例と守ってない悪い例を紹介します。

悪い例

class User {
  String name;
  String email;
  DateTime dateOfBirth;

  User(this.name, this.email);

  void changeName(String newName) {
    this.name = newName;
  }

  String getUserDetails() {
    final year = dateOfBirth.year.toString();
    final month = dateOfBirth.month.toString();
    final day = dateOfBirth.day.toString();
    final birthDay = '$year年$month月$day日';
    return 'Name: $name, Email: $email, BirthDay: $birthDay';
  }

  void sendEmail(String message) {
    print('Sending email to $email...');
    // 省略: メールを送信する実際のロジック
    print('Email sent!');
  }
}

良い例

class User {
  String name;
  String email;
  DateTime dateOfBirth;

  User(this.name, this.email);

  void changeName(String newName) {
    this.name = newName;
  }

  String getUserDetails() {
    final birthDay = dateOfBirth.toDateString();
    return 'Name: $name, Email: $email, BirthDay: $birthDay';
  }
}

class EmailService {
  void sendEmail(String email, String message) {
    print('Sending email to $email...');
    // 省略: メールを送信する実際のロジック
    print('Email sent!');
  }
}

ポイント１：カプセル化

良いコードでは、toDateString()を使い、主目的ではないコードをカプセル化しています。これによりソースコードの可読性が良くなっています。

ポイント２：責任の分離

良いコードでは、User クラスはユーザーの情報を管理する責任だけを持ち、EmailService クラスはメールの送信という責任を持つようになっています。

このように、単一責任の原則を守ることで、各クラスはその責任に集中でき、コードの可読性や保守性が向上します。また、問題の特定や修正が容易になるというメリットもあります。

クラスや関数の責務を考えてコードを書こう！

通信部分を実装する時#

フロントエンドエンジニアが、サーバーからデータ取得するような通信部分の実装する際に、注意すべきことは、正常系と異常系の２つを考慮して実装することです。

正常系とは、期待するデータを受け取り、期待する結果が返ってくることを想定した場合のことです。一方、通常ユーザーが入力しないであろうデータを送ったり、サーバー側でエラーが発生し、期待する結果が返ってこない場合を想定することを異常系と呼びます。

良いコード#

Future<void> printUsers() async {
  try {
    List<User> users = await apiService.fetchUsers();

    if (users.isEmpty) {
      print('ユーザー情報はありません。');
    } else {
      print('ユーザー情報を取得しました:');
      for (var user in users) {
        print('ID: ${user.id}, 名前: ${user.name}, 年齢: ${user.age}');
      }
    }
  } catch (e) {
    print('エラーが発生しました: ${e.toString()}');
  }
}

悪いコード#

Future<void> printUsers() async {
  List<User> users = await apiService.fetchUsers();

  if (users.isEmpty) {
    print('ユーザー情報はありません。');
  } else {
    print('ユーザー情報を取得しました:');
    for (var user in users) {
      print('ID: ${user.id}, 名前: ${user.name}, 年齢: ${user.age}');
    }
  }
}

この例では、以下のような問題があります：

エラーハンドリングが不十分

fetchUsers()メソッド内でエラーが発生した場合でも、例外処理が行われず、エラーメッセージが表示されません。

異常系の処理が欠落

ユーザー情報が取得できなかった場合やエラーが発生した場合でも、適切なエラーメッセージが表示されず、処理が続行されます。このようなコードでは、異常な状態に対する適切な処理やエラーメッセージの表示が行われず、バグの原因となる可能性があります。

正常系と異常系の両方のシナリオを適切に考慮し、適切なエラーハンドリングを行うことが重要です。

ジュニアエンジニアの場合、異常系を考慮せず実装してしまうため、バグが混入することがあります。ユーザーやサーバーが正常でない場合を考えて、実装をする必要があります。

UI を実装する時#

フロントエンドエンジニアが UI を実装する場合、デザインの再現度を意識しましょう。ただし、限られた予算の中で制作をしているため、完璧を追い求めないことが重要だと考えています。

デザイナーは次のことを意識してデザインを制作しています。

  • 整列するように余白を意識している。
  • 情報の塊がわかるようにあえて余白をあけている
  • テキストやボタンの色
  • 文字サイズや要素の大きさ

UI の実装が完了したらプルリクエストを作成し、デザイナーにレビューをしてもらいましょう。 デザイナーがこだわっている箇所についてフィードバックを貰えるはずです。

当社では、プルリクエストのレビュー時にデザイナーを交えることで、デザインチェックを走らせるとともに、デザイナーとエンジニアの認識の違いを早期発見できる体制をとっています。

ビジネスロジックを実装する時#

ビジネスロジックとは、アプリ固有のロジックのことです。 先程、「関心の分離」という話をしましたが、例えば、UI 層にビジネスロジックを書くのはよくありません。なぜなら、UI はあくまでも UI を制作、制御するのが責務だからです。

ビジネスロジックは、サービス層を作成し、UI から分離させます。また、単体テストが書ける状態で実装を行います。

コメントに関するルール#

コメントを書くべきかどうかという議論がありますが、コメントを書くことのメリット、デメリットを把握した上で、コメントを書くべきところには、コメントを書きましょう。

例えば、複雑な処理をするメソッドやビジネスロジックを実装する部分では、コメントを書いた方が、他の人が読みやすいでしょう。

一方で、ソースコードを変更したけど、コメントを変更しなかったより、逆に分かりづらくなることがあります。コメントはしっかりと運用してく必要があるので、プルリクエスト時やレビュー時に、実装とコメントの内容が合っているか確認する必要があります。

以下に、良いコメントと悪いコメントの例を記載します。

良いコメントの書き方#

明確性: コメントはコードの目的を明確に伝えるべきです。 複雑な処理をしている際に、ステップ毎にコメントを記入するのが効果的です。

// お問い合わせがあったユーザーの情報を取得
Future<User> fetchUser(int contactUserId) async {
  // ...
}

なぜコメント: コードが何をするかではなく、なぜそのようにするのかを説明する

// カスタムソーターを使用しています。なぜなら、Dartの組み込みソート関数はこのケースでは適切な結果を返さないからです。
list.sort(customSorter);

悪いコメントの書き方#

冗長: コードが自己説明的である場合、それを再度説明するコメントは冗長で不要です。

// このコードはユーザーIDに1を加算します。
int newUserId = oldUserId + 1;

あいまいな表現や専門用語の過度な使用は避けるべきです。 また、具体的な内容を示さずに「このコードを修正する」といったコメントも有用ではありません。

// FIXME: この部分はもっと良くなるべき。
void someFunction() {
  // ...
}

コメントの無視: コードの変更が行われたときにコメントが更新されない場合、それは混乱を招く可能性があります。

// この関数はユーザーの電話番号を取得します。（実際にはメールアドレスを取得している）
String fetchUserContact(int userId) {
  // ...
}

TODO コメントと FIXME コメント#

実装をしている最中にバグや残タスクに気がついたら、TODO や FIXME コメントを記述します。 この際、具体的にコメントを書くことに注意しましょう。

プロジェクトのリーダーは、プルリクエスト時に TODO や FIXME を見つけたら、必ずチケットを作成します。

良い例

// TODO: 画面遷移をする時にuserIdが渡せてないので渡すように対応必要
void hoge() {
  // ...
}

// TODO: 仕様確認中のため別チケットで対応。=> チケット番号32参照
void foo() {
  // ...
}

悪い例

// FIXME:
void hoge() {
  // ...
}

// FIXME: この部分はもっと良くなるべき。
void foo() {
  // ...
}

自動テストに関するルール#

ありとあらゆるコードに対して、テストを書くのは、限られた予算の中では、現実的ではないことが多々あります。

そういった際は、ビジネスロジックに限定して自動テストを書くことを推奨しています。

ビジネスロジックは、アプリ固有のロジックで、複雑になりやすく、かつアプリの核心となるので、バグが発生すると致命的な問題につながりやすいからです。

テストが書いてあることで、運用保守フェーズでも自信を持って変更することができ、検証にかかるコストを抑えることができます。

ビジネスロジックは複雑になるので、テストを書こう！

given, when, thenのフレームワークとは#

given, when, thenのフレームワークはテストをより構造化し、可読性を高め、テストケースの意図を明確にするために非常に役立つ記法です。 各セクションに分けてテストを記述することで、テストが何をしているのか、どのような条件で実行されるのか、期待される結果は何か、が明確になります。 これにより、テストケースの理解やメンテナンスが容易になります。

given(前提条件)#

  • テストを実行するための前提条件を定義します。
  • データを作成します。

when(操作)#

  • テストケースの条件に応じて、givenで定義したデータを変化させます。
  • 変化させたデータを用いて、テスト対象のメソッドを呼び出し、テストを実行します。

then(操作した結果)#

  • メソッドの呼び出しの結果が、期待される結果と一致するかを検証します。
  • テストが成功したかどうかを確認します。

テストを書く際のコツ#

given, when, thenのフレームワーク通りにテストコードを書くことです。

今回はRailsを使用してUserモデルをテストします。

Userモデルをテストする#

今回のテストケースは以下のようにテストを行います。

  1. name,emailともにバリデーションが通るかどうか。

  2. nameが空の場合はバリデーションが通るかどうか。

  3. emailが空の場合はバリデーションが通るかどうか。

  4. emailが正規表現にマッチしない場合はバリデーションが通るかどうか。

require "test_helper"

class UserTest < ActiveSupport::TestCase
  setup do
    # given
    @user01 = users(:user01) # fixturesからUserデータを参照
  end

  # name,emailともにバリデーションが通るかどうか
  test 'should be valid' do
    # when
    # 何も変化なし

    # then
    assert @user01.valid? # 結果を比較 バリデーションが通ればtrueが返ってくる
  end

  # nameが空の場合はバリデーションが通るかどうか
  test 'should be invalid without name' do
    # when
    @user01.name = '' # user01のnameを空に変更

    # then
    assert_not @user01.valid? # 結果を比較 バリデーションが通らななければfalseが返ってくる
  end

  # emailが空の場合はバリデーションが通るかどうか
  test 'should be invalid without email' do
    # when
    @user01.email = '' # user01のemailを空に変更
    # then
    assert_not @user01.valid? # 結果を比較 バリデーションが通らななければfalseが返ってくる

  # emailが正規表現にマッチしない場合はバリデーションが通るかどうか
  test 'should be invalid with invalid email' do
    # when
    @user01.email = 'test' # user01のemailをtestに変更
    # then
    assert_not @user01.valid? # 結果を比較 バリデーションが通らななければfalseが返ってくる
  end
end