2021年4月4日日曜日

良いコードの書き方(特に大規模開発では極力グローバル変数は使用しない方が良いです。)

https://qiita.com/alt_yamamoto/items/25eda376e6b947208996
シェアしました。


@alt_yamamoto

良いコードの書き方

概要

チームによる継続的開発を前提としたコーディングのガイドライン。
特定の言語を対象としたものではないが、主に静的型付けのオブジェクト指向言語を想定している。
サンプルコードは別段の定めがなければSwiftで記載。

ガイドラインの目的

  • 生産性を高め、メンテナンスコストを下げる
  • バグが生まれづらくする
  • 開発メンバー(特に新規参加者)がコードを理解しやすくする
  • 初心者プログラマーの教育

内容の説明

タイトルの頭についた【:star:数字】は重要度。
高いほどシステムに与える影響が大きいが、低いものの方が影響が小さく改修しやすいものが多い。


:star:5】変数のスコープを小さくする

変わり得る値は複雑さを生み誤解やバグに繋がるため、プログラムは変数が少ないほど問題が生まれづらい。
プログラミングの大原則として、変数は必要最低限を心がけ、むやみに増やさないようにする。

また、変数はスコープや寿命が大きいほど悪影響が大きいため、変数を使う場合はスコープと寿命を最小にするよう心がける。
基本的には以下の上のもの程スコープが広くなるので使用を避けるよう心がける。

  1. グローバル変数
  2. インスタンス変数(クラスのメンバー変数)
  3. ローカル変数

グローバル変数

前述のとおり変数はスコープが大きいほど害が大きいため、一番スコープが大きいグローバル変数は極力使用を避ける。

グローバル変数とは何か

どこからでも読み書きできる変数をグローバル変数と言う。

C言語などのいわゆるグローバル変数に加えて、Javaなどのstatic変数(クラス変数)もグローバル変数と呼ばれることが多い。
さらにここではもう少し意味を広げて、どこからでもアクセス可能なシングルトンや共有オブジェクトなどもグローバル変数として話を進める。

また、データベースやファイルなどのストレージも、どこからでも読み書きできるという性質は同じなため、若干イレギュラーだがここではグローバル変数の一種として扱う。

グローバル変数はなぜダメなのか

グローバル変数はどこからでも値を読み書きできる特性上、以下のような問題を生み出す可能性が高い。

  • 色々なところから値が変更されてプログラムが複雑になり、処理の流れを追いづらくなる
  • 問題があったときに原因が何なのか特定しづらくなる
  • 想定外のところで値が変更されてしまう

加えて、グローバル変数が存在すると、本来無関係なはずのクラスやモジュールが、グローバル変数を通して互いに影響を与え合ってしまう可能性がある(いわゆる密結合な状態になる)
その結果、クラスやモジュールを再利用しづらくなったり、問題の切り分けや、ユニットテストがしづらくなるのも大きな弊害である。

全く使ってはいけないのか

では、グローバル変数を全く使ってはいけないのかと言うと、そんなことはない。
ここでは前述のとおり、シングルトンや共有オブジェクト、DBもグローバル変数の一種と定義しているが、それらを全く使わずアプリケーションを作るのは難しい。

アプリケーションには設定やセッション情報など、全体で共通的に必要になるコンテキストのような情報がある場合が多い。
そういった情報は関数の引数などでバケツリレーのように別のオブジェクトに渡していくより、グローバル変数のようにどこからでもアクセスできる方法を用意した方が実装が簡単になる。

ただし、極力グローバル変数の使用を避けるという方針は変わらない。
全体の設計を慎重に検討した上で、どうしても必要な最低限のものだけグローバルなアクセス方法を用意するようにする。

バックエンドのグローバル変数

HTMLをサーバー側で生成するタイプのWEBアプリ(PHP、Rails、SpringMVC、ASPなど)では、DBやセッションなどがグローバル変数の役割を担うため、厳密な意味でのいわゆるグローバル変数(PHPのグローバル変数やJavaのstatic変数など)が必要になることはほとんどない。
WEBアプリでいわゆるグローバル変数を使っている場合、設計に問題がある可能性が高い。

また、バッチ処理のような短期のプロセスも、DBを除いたいわゆるグローバル変数が必要になることはあまりないように思われる。

フロントエンドのグローバル変数

対して、スマホやデスクトップアプリ、SPAなどのフロントエンドアプリケーションでは、グローバル変数を自前で用意して使う場合が多い。
そのような場合は、以下のような使い方を避ける。

  • 一時的なデータの受け渡しのためにグローバル変数を使う
  • アプリケーションの一部の機能でしか使わないデータをグローバル変数にする
  • グローバル変数にアクセスするクラスを制限せず、無差別にどこからでもアクセスする

裏を返すとグローバル変数は、一時的ではなく永続的に存在し、アプリケーションの様々な機能で共通して利用される情報のために用意され、アクセスするクラスが一部に制限されるなら使って良いということになる。

グローバル変数を使う際のポイント

グローバル変数やstatic変数を作る場合は、IntやStringのような単純なデータ型をそのまま変数に保持するのではなく、関連のあるデータをオブジェクトにして、シングルトンや共有オブジェクトの形にして保持するのが良い。

Bad
var userName: String = ""
var loginPassword: String = ""
Good
class AccountInfo {
   static var shared = AccountInfo()

   var userName: String = ""
   var loginPassword: String = ""
}

また、このような共有オブジェクトを誰もが無制限に追加できると収拾がつかなくなるため、共有オブジェクトの設計はチーム内の一部の有識者で行うのが好ましい。

さらに、グローバル変数を作っても、むやみに色々なクラスから参照してはならない。
グローバル変数にアクセスしても良いレイヤーを決めて、それ以外のレイヤーからはアクセスしない。

シングルトンより共有オブジェクト

グローバル変数はシングルトンや共有オブジェクトの形にするのが良いと書いたが、実際にはシングルトンより差し替え可能な共有オブジェクトの方が良い。

Bad
class Sample {
   // 再代入できないシングルトン
   static let shared = Sample()
}
Good
class Sample {
   // 再代入可能な共有オブジェクト
   static var shared = Sample()
}

シングルトンはインスタンスが一つしかないため以下のデメリットがある。これらのデメリットは特にUnitTestで障害になる上、アプリケーションの実装上問題になるケースもある。

  • インスタンスを複数作りたい場合に作れない
  • 保持する情報をクリアしたい場合にオブジェクトを破棄して作り直すことができない
  • インスタンスの動作をカスタマイズしたい場合に継承ができない

共有オブジェクトならシングルトン的な機能を持たせつつ、上記のデメリットを解消できる。
共有オブジェクトはシングルトンと異なり、インスタンスが一つであることが機械的に保証されるわけではないが、インスタンスを一つにしたいのであれば開発チーム内でそのような設計方針を共有すれば良い。

関連記事
シングルトンパターンの誘惑に負けない

グローバル変数はデータベースとしてとらえる

  • 関連のあるデータをまとめてオブジェクトとして保存する
  • 一時的ではなく永続的に存在するデータを保存する
  • アプリケーションの様々な機能で共通して利用されるデータを保存する
  • グローバル変数にアクセスするクラスは一部に制限する
  • 誰もが自由に追加するのではなく、チーム内の一部の有識者が設計する

共有オブジェクトを用いたグローバル変数について、これまでの項で書いた上記のことを振り返ると、これはDBの位置づけとよく似ている。
グローバル変数はデータベースやMVCパターンで語られるRepositoryのようなものととらえて設計するとイメージしやすい。

DIコンテナ

DIコンテナがあるプラットフォーム(SpringやAngularなど)では、DIコンテナで共有オブジェクトを管理するのが良い。

DIコンテナのある環境ではアプリケーション全体がDIコンテナにべったり依存した設計になりがちだ。
それが良いか悪いかはさて置き、そのような環境では自前で共有オブジェクトを管理するのではなく、DIコンテナに管理を任せた方が統一感があって分かりやすい。

グローバル変数を全く使わない設計は可能か?

必要な情報を全てバケツリレーのように引数で渡していけば、グローバル変数を全く使わずにアプリケーションを作ることもできる。
必要な情報を使用者が取りに行くのではなく外から渡される形(以下これをDIと呼ぶ)である。

ただし、DIの形をとっても、状態変更可能なオブジェクトを渡してしまうと、そのオブジェクトが複数の使用者に変更されてグローバル変数のように扱われてしまうため、DIで渡す情報は状態変更できない(書き込み不可の)値オブジェクトだけにする必要がある。

このように変更不可なオブジェクトをバケツリレーのように渡していくことを徹底すれば、グローバル変数のようなものを完全に排除でき、一見疎結合で綺麗な設計になると思われるが、この方針には実は落とし穴がある。

この方針の問題は、バケツリレーの末端のオブジェクトで必要な情報を、仲介する全てのオブジェクト(バケツリレーの渡し役)が一時的に持たなければいけなくなる点だ。
この問題により特定のクラスが一時的に無関係な情報を持つ必要がでてくるので、一概に疎結合になるとも言えない。

結局のところ、グローバル変数の使用を避けろとは言うものの、全く使わないのも現実的ではなく、適切な方針のもとでグローバル変数を使用することが最善の設計なのかもしれない。

インスタンス変数(クラスのメンバー変数)

グローバル変数同様、インスタンス変数も可能な限り使用を避ける。
新たに変数を追加する場合は、気軽に追加するのではなくどうしても必要かをしっかり考える。
同じ機能を実装したクラスが2つあった場合、インスタンス変数が少ない方が良い設計だと言ってしまっていいくらい重要なポイントだ。

インスタンス変数を少なくする方法

インスタンス変数の使用を極力避けると書いたが、具体的にどうやって変数を少なくするかというと、まずは以下の三つが基本になる。

  • 関数の引数で足りるデータをインスタンス変数にしない
  • 加工した値をインスタンス変数に保持しない
  • プログラムで取得できる情報をインスタンス変数に保持しない

関数の引数で足りるデータをインスタンス変数にしない

初心者によく見られるが、複数の関数でデータを使い回すためだけにインスタンス変数を使ってはいけない。

長期保存する必要のないデータであればインスタンス変数を使わず関数の引数で渡すようにする。

bad
class Foo {
    var user: User?

    func setUser(user: User) {
        self.user = user
        printName()
        printEmail()
    }

    func printName() {
        print(user?.name)
    }

    func printEmail() {
        print(user?.email)
    }
}
good
class Foo {
    func setUser(user: User) {
        printName(user: user)
        printEmail(user: user)
    }

    func printName(user: User) {
        print(user.name)
    }

    func printEmail(user: User) {
        print(user.email)
    }
}

関連記事
(あなたの周りでも見かけるかもしれない)インスタンス変数の間違った使い方

加工した値をインスタンス変数に保持しない

パフォーマンス最適化のためにやむを得ない場合もあるが、基本的に何らかの値を加工した値をインスタンス変数に保持しない。

以下の例では itemsA と itemsB が items を加工した値になる。

bad
class Foo {
    var items = ["A-1", "A-2", "B-1", "B-2"]
    let itemsA: [String]
    let itemsB: [String]

    init() {
        itemsA = items.filter { $0.hasPrefix("A-") }
        itemsB = items.filter { $0.hasPrefix("B-") }
    }
}

この例では itemsA と itemsB の値をインスタンス変数にする必要はなく、items だけを変数にして、そこから関数でitemsA と itemsBを生成すればよい。

good
class Foo {
    var items = ["A-1", "A-2", "B-1", "B-2"]

    func itemsA() -> [String] {
        return items.filter { $0.hasPrefix("A-") }
    }

    func itemsB() -> [String] {
        return items.filter { $0.hasPrefix("B-") }
    }
}

プログラムで取得できる情報をインスタンス変数に保持しない

「加工した値をインスタンス変数に保持しない」と少し被るが、他の値から判定できる情報や、何らかのAPIやプログラムで取得できる情報はインスタンス変数に保存せず、必要になったタイミングで都度プログラムを実行して取得する。

クロージャを使ってインスタンス変数を消す

初心者には少し難しいが、クロージャを使って無用なインスタンス変数を減らすことができる。

基本的にインスタンス変数はデータを長期間保持するために使われるが、クロージャを使うとインスタンス変数を使わずにデータを長期間保持することができる。

以下の例ではafterの形にすることによって、インスタンス変数を使わずにAPI通信が完了するまで dataType を保持することができる。

before
class Before {
    var dataType: DataType?

    func fetchData(dataType: DataType) {
        self.dataType = dataType // インスタンス変数に保存

        APIConnection(dataType).getData(onComplete: { response in
            self.setResponse(response)
        })
    }

    func setResponse(_ response: Response) {
        print("\(dataType) がセットされた")
    }
}
after
class After {
    func fetchData(dataType: DataType) {
        APIConnection(dataType).getData(onComplete: { response in
            // クロージャ内でdatTypeを使うことで、API通信完了までdataTypeを保持することができる
            self.setResponse(response, dataType: dataType)
        })
    }

    func setResponse(_ response: Response, dataType: DataType) {
        print("\(dataType) がセットされた")
    }
}

関数型プログラミングを活用すると、変数を減らしたり、変数のスコープを狭めたりすることができる。
Haskellなどの一部の関数型言語では、そもそも変数に再代入ができないので、ある意味変数がないとも言える。

関数型プログラミングを使っても結局はどこかにデータが保存されるが、スコープや影響範囲が小さくなり、より害のないコードを書けるようになる。

ローカル変数

使ってもよいが、必要になったときに初めて定義して、スコープを最小にするよう心がける。

Bad
var num = 0
for i in list {
    num = i
    print(num)
}
Good
for i in list {
    let num = i
    print(num)
}

ただし、一部の言語(C言語やJavaScriptのvarの巻き上げへの対処など)ではスコープの先頭で変数を宣言する必要がある場合もある。

説明変数は積極的に使って良い

可読性のため、実装上必ずしも必要ではない変数を作り、式の結果をいったん代入する場合がある。
そのような変数は説明変数といい、他の変数と異なり必要であれば積極的に使って良い。

説明変数には値の再代入をすることがなく、定数のように扱われるため増やしてもあまり害がない。

before
let totalPrice = ((orangePrice * orangeQuantity) + (applePrice * appleQuanitity)) *  (1 + taxPercentage / 100)
after
// 以下の3つは説明変数
let orangePriceSum = orangePrice * orangeQuantity
let applePriceSum = applePrice * appleQuanitity
let includesTaxRate = 1 + taxPercentage / 100

let totalPrice = (orangePriceSum + applePriceSum) * includesTaxRate

説明変数を使うとコードの行数は多くなるが可読性は上がり、式の途中結果がわかるためブレイクポイントを使ったデバッグがやり易くなるメリットもある。

変数の寿命を短くする

何らかの状態を変数に保存する場合、値の存在期間はできる限り短くする。
必要になったタイミングで値を保存し、必要なくなったタイミングでなるべく早くクリアする。
変数に保存した値はその瞬間のスナップショットであり、時間が経つほど最新の状態とズレてしまう危険性があるため、変数に保存した値の寿命は極力短くする。

:star:5】単一ソースの原則

同じ情報を重複して複数保持しない。
例えば以下の例では年齢を別の形で2つのフィールドに保持しており、情報が重複している。

Bad
class Person {
   var age = 17
   var ageText = "17歳"
}

このような場合、以下のように保持する情報は一つにして、その値を加工して使うのが良い。

Good
class Person {
   var age = 17
   var ageText: String { 
      return "\(age)歳" 
   }
}

情報の重複はシステムに以下のような悪影響をもたらす。

  • 複数ある情報のどれを使っていいか分からなくなる
  • 複数ある情報の一部しか更新されないと、差異や矛盾ができる
  • 誤った情報を参照、または更新してしまう可能性がある
  • 情報を変更する場合、複数のフィールドを変更する必要がある

この例は単純で分かりやすい情報重複だが、他にも様々な形の情報重複がある。

キャッシュによる重複

以下の例ではDBのデータを読み込んでインスタンス変数に保持(キャッシュ)しているが、これによりインスタンス変数に保持した情報とDB内の情報が重複してしまう。

Bad
class Foo {
   var records: [DBRecord]?
   func readDBRecord(dbTable: DBTable) {
      records = dbTable.selectAllRecords()
   }
}

DBから読み込んだデータを上記のようにインスタンス変数に保持するのはなるべく避けた方が良い。
Fooオブジェクトが長期間存在する場合、その間にDBが更新されるとFooが持つインスタンス変数の情報と、DBの情報に差異が出てしまう。

分かりきった情報をコード化しない

以下の例ではDictionary(Map)のキーに0、1、2を使用しているが、Arrayにすればインデックスがあるので、順番をとりたいのであれば不要な情報だ。

Bad
func getName(index: Int) -> String? {
    let map = [0: "佐藤", 1: "品川", 2: "鈴木"]
    return map[index]
}

Arrayにすればインデックスでのアクセスが可能なので0〜2の情報は不要になる。

Good
func getName2(index: Int) -> String? {
    let names = ["佐藤", "品川", "鈴木"]
    if 0 <= index && index < names.count {
        return names[index]
    }
    return nil
}

プログラミング以外への適用

情報を重複させないという方針は、プログラミングに限らずドキュメント管理などでも役に立つ。

Bad
ローカルマシンに仕様書をコピーして見ていたら、仕様書が更新されており、古い仕様書を元にコーディングしていた。

諸々の事情によりローカルにコピーせざるを得ない場合もあるが、上記の例ではコピーして仕様書が重複したことにより問題が発生している。

Good
仕様書は共有のサーバーに1つしかないので、常に最新の仕様が見れる。

コードの重複を禁じるものではない

この項で問題にしているのはあくまで情報を重複して持たないことであり、同じようなロジックのコードを重複させないことではない
「コードをコピペして似たようなコードを複数作る」ようなケースはここで言う重複とは異なるので誤解なきよう。
同じようなロジックのコードを共通化するのはまた別の話で、もっと優先度の低い方針になる。

一つのフィールドに複数種類の情報を持たせない

逆に一つのフィールド(変数、DBカラム、テキストUIなど)に複数種類の情報を持たせるのも避ける。

一つのフィールドに複数種類の情報を入れると実装が複雑化し、様々なバグを生み出すリスクが発生する。

間違っても一つの変数を複数の用途で使い回すということはしないようにしよう。

:star:5】適切な名前をつける

クラス、プロパティ、関数、定数など、プログラムで定義する要素には適切な名前をつける。
特にクラスの名前は設計に大きく関わり重要性が高い。

クラス名、プロパティ名、関数名を適切につけることができれば、それはクラス設計、データ設計、インターフェース設計が適切になされたのとほぼ同義である。
ある意味プログラミングとは名前を付ける作業だと言っても過言ではない。

名前をつけるときは、以下を心がける。

  • 他人が見ても名前から目的や意味がわかる
  • 実際の用途や処理と名前が合っている
  • 名前に書かれている以外の処理や役目を持たない

また、関数や変数はスコープの広いものほど丁寧な名前をつけるようにする。

名前に書かれていること以上の役割を持たせない

変数は名前に書かれた以上の役割を持たせない。関数は名前に書かれたこと以外の処理をしない。

例えば、LoginViewController というクラスであれば、そこにはLoginのViewをControllする処理のみを記載し、それ以外の例えばログイン認証などの処理は原則として記載しない。
(ただし、短い処理であれば「関連するものは近くにおく」のルールに従い、同一ファイルに記載してもよい)

Javaでいう getHoge() のようなゲッターで何かの状態の更新をするなど、もってのほかである。

名付けのアンチパターン

数字やIDをつける

Bad
var code1 = "a"

func func001() {}

enum VieID {
  case vol_01, vol_02, vol_03
}

上記のような数字やIDは、知らない人には何を意味するのか分からないため、プログラム内で名前に使用するのは避ける。
またコードにIDを使うと、IDが変わった場合にプログラムの修正が必要になってしまう。

単語を省略する

Bad
func chkDispFlg(){}

分かる人も多いと思うが上記は checkDisplayFlag の略である。
このような単語の省略はプログラマーの伝統文化だが、現代ではIDEによるコード補完があることがほとんどなので、省略してもあまりメリットはない。
第三者に意味がわかりづらくなるため、単語は省略しないようにする。

意味のない名付け

Bad
let yen = "円"

値の具体的な内容をそのまま名前にするのは、拡張性が無いので避ける。

例えば、上記の"円"は"ドル"に変わると変数名の "yen" が嘘になってしまう。
このような場合は、この定数が何であるかではなく、どういった役割・意味を持つかから名前をつける。
上記の例で言えば、例えば金額の接尾辞という役割から "priceSuffix" としたり、通貨単位を意味することから "currencyUnit" などが考えられる。

Boolean変数の名付け方

Bad
var loadFlag = false

flagという言葉はBooleanであることを表すだけで、用途や意味を何も表現できないため、Booleanの変数名にflagを使うのは避ける。
Boolean変数の命名は以下のようなお決まりパターンがあるので、この形に従うと良い。

  • is + 形容詞 (isEmptyなど)
  • is + 過去分詞 (isHiddenなど)
  • is + 主語 + 過去分詞 (isViewLoadedなど)
  • has + 名詞 (hasParentなど)
  • can + 動詞 (canLoadなど)
  • 動詞 (exists、containsなど)
  • should + 動詞 (shouldLoadなど)

関連記事
boolean 値を返却するメソッド名、変数名の付け方

うまくメソッド名を付けるための参考情報

英語が分からない問題

英名を知らないものに変数名をつける場合、まずはネットで調べるケースが多いと思うが、Google翻訳などの機械翻訳は単語が適切に翻訳されない場合が多いので気をつける。
英語を調べる場合は、Google翻訳だけでなく、辞書、Wikipediaなどで例文も含めて調べるのが良い。

ただし、チームメンバーがみんな英語が苦手で、英語を調べるのに時間がかかるのなら、英語を諦めローマ字の日本語で書くのも一つの手である。
少し格好悪いが、大切なのはチームメンバー全員の可読性と生産性であると思う。

また、ユニットテストの関数名は説明的な文章になる場合が多いので、関数名に日本語を使える環境であれば、日本語で書くのも良い。

Javaのユニットテストの関数名例
public void 何々したときにこういう挙動をするか確認する試験() {}

汎用的な名前を避ける

汎用的な名前はなるべく避ける。
よくあるのはなんとかManagerなんとかController
汎用的な名前は様々な処理を押しつけやすくクラスが肥大化しがち。

辞書を作る

チームで開発をする場合は、アプリケーションで使う用語の辞書を作り、用語の認識をメンバーで合わせた上で開発を始めると良い。
辞書を作ることで、同じものが開発者によって別の名前で定義される不整合を防ぐことができるし、個々の開発者が同じものの名付けで別々に頭を悩ますという無駄を省くことができる。

関連記事
モデルやメソッドに名前を付けるときは英語の品詞に気をつけよう
クラスの命名のアンチパターン

コードスタイルの一貫性を重視する

クラスや変数の命名規則などのコードスタイルは、プロジェクト内で統一して一貫性をもたせる。
プロジェクトの一部に例外的な名前やスタイルを使うと、可読性が損なわれ誤解や見落としの原因となりやすい。

例えばView001View002View003というクラスが既にあれば、次にViewクラスを追加する場合は名前の付け方を統一してView004にするのが良い。
これは上に記載した「記号やIDを名前に使わない」の項に反するが、プロジェクト内で名前に一貫性を持たせることの方がより重要になる。

現在の命名規則やスタイルに問題があり変更したいなら、チームメンバーの了解をとった上で、一部だけでなく全てまとめて修正するのが良いだろう。

:star:5】function(object)の形よりobject.function()の形を好む

関数にオブジェクトを渡す形function(object)より、オブジェクトの関数を呼び出す形object.function()を好む。

Bad
if StringUtils.isEmpty(string) {}
Good
if string.isEmpty() {}

これにはいくつかの理由があるので以下に説明する。

可読性のため

関数にオブジェクトを渡す形function(object)は、複数の処理を重ねると括弧が入れ子になり読みづらくなるが、オブジェクトの関数を呼び出す形object.function()はドットで繋いで複数の処理を行えるため可読性がよい。

Bad
StringUtils.convertC(StringUtils.convertB(StringUtils.convertA(string)))
Good
string.convertA().convertB().convertC()

object.function()の方が再利用性が高い

一般的に function(object)の場合、functionは何らかのクラスやモジュールに定義され、処理を行うにはfunctionが定義されたクラスやモジュールと引数のobjectの2つが必要になる。
対して、object.function()の場合はobject単体で処理を行うことができるため再利用性が高い。

オブジェクト指向を身につける

object.function() の形はオブジェクト指向の真髄だ

オブジェクト指向と言うと、クラス、継承、カプセル化などがまず説明されがちだが、実はそれらはオブジェクト指向に必須のものではなく、オブジェクト指向に唯一必要なものはオブジェクトに対してメソッドを呼び出す object.function() の形だけだと思う。

初心者にオブジェクト指向を教えるのは難しい。
オブジェクト指向について教えるべきことはたくさんありそうに思える。
しかし、継承やカプセル化やポリモーフィズムなどはいったん忘れて、まず object.function() の形を体に染み込ませることが、オブジェクト指向を身につける最短ルートではないかと考える。

関連記事
オブジェクト指向は、メソッドディスパッチの手法の1つ

Computed propertyを積極的に使う

言語によってはない場合もあるが、Computed propertyの機能により、functionをpropertyとして扱うことができる。
以下のようなfunctionは積極的にComputed propertyにしていこう。

  • 引数が不要
  • 値を返す
  • 処理が重くない

当てはまらないケース

では、いついかなるときも function(object) より object.function() の形がいいのかと言うと、そうではない場合もある。

object.function() の形をとるべきでないのは、この形をとることによって object のクラスに必要のない依存関係や、負うべきでない役割を持たせてしまう場合だ。

以下の例は object.function() の形をとることにより、enum APIResult に不要なViewクラスへの依存を作ってしまっている。

Bad
class LoginView: MyView {
    // ログイン結果を受け取って次の画面に遷移する
    func onReceivedLoginResult(result: APIResult) {
        let nextView = result.nextView() // object.function() の形
        showNextView(nextView)
    }
}

enum APIResult {
    case success
    case warning
    case error

    func nextView() -> UIView {
        switch self {
        case .success: return HomeView()
        case .warning: return WarningView()
        case .error: return ErrorView()
        }
        // HomeView、WarningView、ErrorViewのクラスに依存してしまっている
    }
}

このような例では以下のように function(object) の形をとった方が良いだろう。

Good
class LoginView: MyView {
    // ログイン結果を受け取って次の画面に遷移する
    func onReceivedLoginResult(result: APIResult) {
        let nextView = nextView(result: result) // function(object) の形
        showNextView(nextView)
    }

    func nextView(result: APIResult) -> UIView {
        switch result {
        case .success: return HomeView()
        case .warning: return WarningView()
        case .error: return ErrorView()
        }
    }
}

enum APIResult {
    case success
    case warning
    case error
}

前者のような依存関係を作るべきでない理由は、下にある「依存の向きを意識する」の項で詳しく説明している。

:star:5】継承より包含とインターフェースを好む

クラスの継承には問題がある。
継承は強力な機能だが、その代りに多くのリスクもはらんでいる。
クラス継承は柔軟性がなく変更に弱い。

修正により無関係な機能に影響を与えてしまう

例えばBaseクラスを継承したAとBのクラスがあったとして、Aの機能を改修するためにBaseを修正したら、無関係なBの機能がバグってしまうようなケース。
これはクラス設計に問題があると言えるが、継承を使うとこのような問題は起こり得る。

複数の親を継承できない

C++など一部の言語を除き、複数のクラスを継承することはできない。
しかし、現実に存在するものには、複数の上位概念(親カテゴリー)が存在することが多い。

例えば、唐突だがインドカレーについて考えてみると、その親はカレーとすることもできるし、インド料理とすることもできる。

class インドカレー extends カレー
class インドカレー extends インド料理

もう少し実装の流れにそった例をあげると、例えばWEBでスマホの契約ができるシステムがあったとする。
契約画面は新規契約乗り換え(MNP)契約の2つに別れていて、それぞれに対応するクラスがある。
新規契約と乗り換え契約には共通する部分が多いので、共通の親クラスとして基盤契約クラスを作る。

スクリーンショット 2019-01-25 21.50.45.png

次に契約変更画面を実装することを考える。
契約変更用に新規契約クラスを継承した新規契約変更と乗り換え契約クラスを継承した乗り換え契約変更のクラスを作ったとすると、契約変更という観点では継承で処理を共通化できなくなってしまう。

スクリーンショット 2019-01-25 21.44.57.png

共通の親クラスが肥大化しがち

例えば、全てのControllerクラスの親クラスになるBaseControllerというクラスを作ったとすると、BaseControllerクラスは様々なControllerで使われる機能が盛り込まれ肥大化しがちだ。
必要最低限の機能であればよいが、基本的に共通の親クラスは機能を提供するのではなく、共通のインターフェースとして扱うために用意するのがよい。

関連記事
継承のデメリット
iOSアプリの設計でBaseViewControllerのようなのは作りたくない
クラスの「継承」より「合成」がよい理由とは?ゲーム開発におけるコードのフレキシビリティと可読性の向上

:star:4】分岐をシンプルにする

if文やswitch文によるロジックの分岐は、プログラムの可読性を損ないバグの原因になりやすいので、なるべくシンプルな形になるよう心がける。

ネストを深くしない

if文やfor文などのネスト(入れ子)が深くなるとコードが読みづらくなるため、なるべくネストを深くしない。ネストが深くなるのを防ぐには「早期return」と「ロジックの切り出し」を行うと良い。

以下のコードに「早期return」と「ロジックの切り出し」を適用するとどうなるか例を用いて説明する。

Before
if text != nil {
    if text == "A" {
        // 処理1
    } else {
        // 処理2    
    }
}

早期return

例外的なケースを先にreturnすることで、メインロジックのネストを浅くする。
以下のサンプルコードではtextがnilの場合を例外パターンとして先にreturnしている。

After(早期return)
if text == nil {
    return
}
if text == "A" {
    // 処理1
} else {
    // 処理2
}

ただし、早期returnは名前のとおり早期にreturnする必要がある。
基本的に関数は末尾行まで処理が実行されることが期待されるため、長い関数の途中にreturnがあると、見落とされてバグを生んでしまうリスクがある。

ロジックの切り出し

if文やfor文などのネストを含む処理をメソッドやプロパティに切り出して、メインロジックのネストを浅くする。
以下のサンプルコードではtextが”A”か判定して何らかの処理をする部分を、Textクラスのクラスメソッドとして切り出している。

After(ロジックの切り出し)
if text != nil {
   doSomething(text)
}

func doSomething(_ text: String?) {
    if text == "A" {
        // 処理1
    } else {
        // 処理2
    }
}

関連情報
ブロックネストの数を減らそう

呼び出し元による場合分けをしない

様々なところから呼び出される関数には、呼び出し元による場合分けを入れてはいけない。
例えば以下のケースは、画面によって処理の場合分けをしているが、この書き方では画面が増えるほど際限なく関数が大きくなってしまう。

Bad
class BaseViewController: UIViewController {
    func doSomething() {
        if viewId == "home" {
            // ホーム画面の処理
        } else if viewId == "login" {
            // ログイン画面の処理
        } else if viewId == "setting" {
            // 設定画面の処理
        }
    }
}

この書き方をすると一つの関数に様々な処理が詰め込まれ、読みづらく、バグりやすく、修正もしづらい巨大な関数になってしまう可能性が高い。

ポリモーフィズム

上記のような関数内の場合分けはポリモーフィズムを使って解消することができる。
ポリモーフィズムについての詳しい説明は長くなるので割愛するが、インターフェース(プロトコル)や継承によるメソッドのオーバーライドにより、場合分けされた各処理を子クラスにそれぞれ記載することができる。

class BaseViewController {
    func doSomething() {}
}
class HomeViewController: BaseViewController {
    override func doSomething() {
        // ホーム画面の処理
    }
}
class LoginViewController: BaseViewController {
    override func doSomething() {
        // ログイン画面の処理
    }
}
class SettingViewController: BaseViewController {
    override func doSomething() {
        // 設定画面の処理
    }
}

関数を引き渡す

また、関数を関数の引数にして渡すことによって、if文などの分岐を解消することもできる。
以下のように引数に関数を受け取るようにすれば、somethingの処理は呼び出し元で自由に設定できるので、分岐をなくすことができる。
この例では受け取った関数を実行するだけの意味のない関数になってしまっているが、呼び出し元によって異なる完了処理やエラー処理を引数として渡すようなことが多い。

class BaseViewController: UIViewController {
    func doSomething(something: () -> Void) {
        something()
    }
}

Javaのような関数をオブジェクトとして扱えない言語でも、インターフェースを使って同じようなことが実現できる。

分岐ブロック内の行数を少なくする

分岐を見やすくするため、if文などの分岐ブロック内の行数はなるべく少なくなるよう心がける。

if conditionA {
    // ここに長い処理を書かない
} else if conditionB {
    // ここに長い処理を書かない
}

コマンドとクエリを分離する

全てのメソッドは、アクションを実行するコマンドか、データを返すクエリかのどちらかであるべきで、両方を行ってはならない。

これはCQS(command query separation)と言われる考え方だ。

まず、データを返すメソッド(クエリ)の中でアクションを行ってはいけない。
ゲッターの中で更新処理をしないと言い換えると分かりやすいかもしれない。
これは当然のこととして心がけている人が多いと思う。

次に、アクションを実行するメソッド(コマンド)の中にクエリをなるべく記載しない。
こちらは前者よりわかりづらく、もしかするとCQS本家の人が言ってることとはズレているかもしれないが、例をあげて説明する。

例えば、ユーザーネームとセッションIDが両方ある場合にログイン済みとみなして次のページに進む関数を考える。

Bad
func nextAction() {
    if userName.isNotEmpty && sessionId.isNotEmpty {
        showNextPage()
    }
}

この関数は「次のページに進む」ことが主目的と考えると、「クエリ」 ではなく 「コマンド」 になるが、ログインしているかどうかを判定する部分はBoolの値を取得するクエリになるため、「コマンド」と「クエリ」が混在している。

この関数からログイン判定の部分をクエリとして別関数に切り出すと以下のようになる。

Good
// コマンド
func nextAction() {
    if isLoggedIn() {
        showNextPage()
    }
}

// クエリ
func isLoggedIn() {
   return userName.isNotEmpty && sessionId.isNotEmpty
}

if文の条件式がごく短いものであれば、それを別関数に切り出すかは迷うところだが、ある程度の長さのクエリであれば別関数やプロパティとしてアクションから切り出すのがいいだろう。

:star:4】クラス、ファンクションを大きくしすぎない

クラスは50〜350行程度、ファンクションは5〜25行程度を目安とし、これを超える場合はクラスやファンクションの分割を検討する。

行数はあくまで目安で、これを超える行数でも同じクラスやファンクションに収めた方が良い場合もあるが、キリがいい上限として1000行を超えるクラスはヤバイと思っておこう。

言語や書き方や機能によって変わるので一概にこのサイズが正解とは言えないが、クラスの行数に対するざっくりしたサイズ感を記載する。

行数サイズ感
50行未満小さい
50〜350行適正
350〜700行大きい
700〜1000行とても大きい
1000行超ヤバイ

とはいえ、基盤となる機能や、要素の多いUIクラスなどは上記のサイズ感を逸脱する場合もある。
例えば、AndroidのViewクラスは2万7千行あるし、極端な例だが異なる処理をするボタンが1000個ある画面は、簡潔に書いても1000行を超えてしまう。

関連記事
プログラミング中級者に読んでほしい良いコードを書くための20箇条

:star:4】フラグ(Booleanのプロパティ)を作らない

「変数のスコープを小さくする」の項に記載したインスタンス変数をなるべく持たない方針と近いが、Bool値のフラグはさらに厳しくインスタンス変数に保持しないよう心がける。
フラグ(Boolの変数)が持つ情報はある一時点での判定結果であり、基本的に過去の情報になる。
そのため時間が経つほど実際の状態と乖離してしまう危険性がある。

Bad
class Foo {
    var hasReceived: Bool = false // 不必要なフラグ
    var data: Data?

    func setData(data: Data) {
        self.data = data
        hasReceived = true
    }
}

上の例ではデータを受け取ったかどうかをフラグで保持しているが、data変数を見ればデータを受け取ったことは分かるのでフラグは必要ない。
データの多重管理になってしまうため、このようなフラグの作成は避ける。

フラグをなくすためには、まずフラグの代わりに他の状態で判断できないかを検討する。
フラグは何らかの状態を判定するものだが、フラグがなくても他のものを見て状態を判定できることは多い。

複数のフラグを1つの状態変数にまとめる

複数のフラグがある場合、状態を表すEnumを作ることで複数のフラグを1つのEnum変数にまとめることができる。

Bad
class User {
    var isAdmin = false
    var isSuperUser = false
    var isGeneralUser = false
}
Good
enum UserType {
    case admin       // 管理者
    case superUser   // スーパーユーザー
    case generalUser // 一般ユーザー
}

class User {
    var userType: UserType = .admin
}

ただし、Enumにまとめるのは必ず一種類の情報だけにし、複数種類の情報を一つのEnumにまとめてはいけない。

上の例で言えば、「ユーザータイプ」に「ログイン状態」を加えて、以下のようなEnumを作ることは避ける。

Bad
enum UserLoginType {
    case adminLoggedIn          // 管理者(ログイン済)
    case adminNotLoggedIn       // 管理者(未ログイン)
    case superUserLoggedIn      // スーパーユーザー(ログイン済)
    case superUserNotLoggedIn   // スーパーユーザー(未ログイン)
    case generalUserLoggedIn    // 一般ユーザー(ログイン済)
    case generalUserNotLoggedIn // 一般ユーザー(未ログイン)
}

:star:4】 数字や文字列をデータのキーにしない

数字を条件分岐の判定に使わない

0、1、2などの数字を条件分岐の判定に使うのは避ける。
代替方法はケースにより様々だが、配列やListを使うことで数字の使用を避けられることが多い。

Bad
func title(index: Int) -> String {
   switch index {
      case 0:
         return "A"
      case 1:
         return "B"
      case 2:
         return "C"
      default:
         return ""
   }
}
Good
let titles = ["A", "B", "C"]

func title(index: Int) -> String {
    return titles.indices.contains(index) ? titles[index] : ""
}

ただし、0、1、-1は最初の要素を取得したり、APIのエラーや成功を表したりと、特殊な役目を持つ場合が多いため、環境によっては使わざるを得ない場合もある。

配列で複数種類のデータの受け渡しをしない

静的型付け言語では配列(Array、List)を使って複数種類のデータの受け渡しをしない。

以下の関数は数字を使ってListの1番目に名前、2番目に郵便番号、3番目に住所の情報を格納しているが、何番目に何の情報が入っているかはList型からは判別できないため、可読性が悪くなり間違いも起こりやすくなる。

Bad
func getUserData(): [String: String] {
   var userData: [String] = []
   userData[0] = "山田 正男"
   userData[1] = "171-0001"
   userData[2] = "東京都豊島区"
   return userData
}

決まったデータ構造を受け渡す場合は、以下のようにstructやclassを作ってデータを受け渡す。

Good
struct UserData {
   let name: String
   let postalCode: String
   let address: String
}

func getUserData(): UserData {
   return UserData(name: "山田 正男", 
                   postalCode: "171-0001", 
                   address: "東京都豊島区")
}

ただし、名前の一覧を受け渡したり、ファイルの一覧を受け渡したり、同じ種類のデータの一覧を受け渡す場合は配列やListを使って良い。

Mapで複数種類のデータの受け渡しをしない

全く同じ理由で、Map(Dictionary)で複数種類のデータを受け渡すのも避ける。
静的型付け言語であれば、配列の例と同様にデータ受け渡しのためのstructやclassを作成する。

Bad
func getUserData(): [String: String] {
   var userData: [String: String] = [:]
   userData["name"] = "山田 正男"
   userData["postalCode"] = "171-0001"
   userData["address"] = "東京都豊島区"
   return userData
}

:star:4】コメントアウトしない

不要になったコードはコメントアウトせず削除する。
ローカルでの修正中に一時的にコメントアウトするのは構わないが、基本的にコメントアウトしたものはコミットしないようにしよう。

コメントアウトした行が増えると、コードが読みづらくなる、検索時に使われていない箇所が引っかかるなど結構な害がある。
削除や変更の履歴はGitなどの管理ツールで分かるので、不要なコードは消すことを心がける。

:star:3】依存の向きを意識する

プログラム間の依存関係にはルールを設けて、無計画に依存をしないようにする。
抽象的な説明になってしまうが、以下の向きで依存して、この逆方向の依存を避ける。

  • 具象から抽象に依存する
  • 大きな機能から小さな機能に依存する
  • 専用的な機能から汎用的な機能に依存する

依存とは何か?

あるプログラムが、何らかの別のクラスやモジュール、ライブラリなどを使っており、それがないとコンパイルできなかったり、動作できないことを依存していると言う。
また、コンパイルレベルの依存の他に、特定の仕様を前提として作られており、その仕様がなければ動かない場合なども、仕様に依存していると言える。

専用的な機能から汎用的な機能に依存する

汎用的に使われるクラスは、専用的なクラスに依存しないようにする。

例えば極端な例だが、文字列を表すStringクラス(汎用的な機能)が、ログイン画面のクラスLoginView(専用的な機能)に依存していたとすると、Stringを使う全てのシステムはLoginViewも無駄に取り込む必要ができてしまい、Stringクラスが再利用しづらくなってしまう。

もっとありがちな別の例を挙げると、通信処理を行うための HTTPConnection クラス(汎用的な機能)があったとして、このクラスの中でアプリ独自の画面クラス(専用的な機能)を処理や判定に使ってしまうと、HTTPConnection クラスは別のアプリに移植することができなくなってしまう。

汎用的な機能のクラス内で、アプリ独自の専用的な画面クラスやIDを判定に使うことは、汎用クラス内に過剰な分岐を生み出して複雑化させる弊害もあるので避ける。

ライブラリに依存するところは一箇所にまとめる

現代ではアプリの実装に何らかのオープンソースライブラリを使うのは当たり前だが、特定のライブラリを使った処理は一つのクラスやファイルにまとめて、様々なクラスがライブラリに依存するのは避ける。
面で依存するのではなく、点で依存するようなイメージだ。

ライブラリを使ったコードを一箇所にまとめることにより、以下のような変更があった場合の影響や修正を最小限にすることができる。

  • 使っているライブラリを別のライブラリに差し替えたい
  • ライブラリに仕様変更が入り使い方が変わった

データクラスは具体的な機能や仕様に依存させない

「専用的な機能から汎用的な機能に依存する」の項に書いたことと若干被るが、DTOのようなデータの保持を目的としたクラスはなるべくシンプルにして、別の機能に依存したり、特定の仕様に依存させない方が良い。

// シンプルなデータクラスの例
struct UserInfo {
    let id: Int
    let name: String
}

このようなシンプルなデータクラスは多くのレイヤーをまたいで使われたり、ときには別のアプリケーションに移植されたりするが、その際に余計な依存があると色々と弊害が出ることがある。

インターフェースを使って具象への依存を断ち切る

多くの静的型付け言語ではインターフェース(プロトコル)を使って具体的な実装クラスへの依存をなくすことができる。(動的型付け言語ではインターフェースがない場合が多いのでこの項は当てはまらないケースが多い)

例えば、以下の例では Example クラスが通信を行う HTTPConnector クラスに依存している。

Before
class Example {
    let httpConnector: HTTPConnector

    init(httpConnector: HTTPConnector) {
        self.httpConnector = httpConnector
    }

    func httpPost(url: String, body: Data) {
        httpConnector.post(url: url, body: body)
    }
}

class HTTPConnector {
    func post(url: String, body: Data) {
        // HTTP通信の実装
    }
}

HTTPConnector クラスが何らかの通信ライブラリを使っていると、Exampleクラスは間接的にその通信ライブラリに依存することになってしまう。
だが、以下のようにHTTPConnectorをインターフェース(プロトコル)に変えれば、Exampleクラスが通信ライブラリに依存しないようにすることができる。

After
class Example {
    let httpConnector: HTTPConnector

    init(httpConnector: HTTPConnector) {
        self.httpConnector = httpConnector
    }

    func httpPost(url: String, body: Data) {
        httpConnector.post(url: url, body: body)
    }
}

protocol HTTPConnector {
    func post(url: String, body: Data)
}

外部システムとの連携は一箇所にまとめる

アプリケーション外部のシステムとの連携やインターフェースに関わるコードは、なるべく一箇所にまとめる。
外部システムは色々なものが考えられるが、よくあるものとしてはHTTPのAPI、データベースなどがある。

オープンソースライブラリへの依存について書いたことと同じだが、外部システムとの連携を一箇所にまとめることにより、外部システムの変更の影響や修正を最小限にすることができる。
こちらもまた、インターフェース(プロトコル)を使って具体的な実装クラスへの依存をなくせると良い。

:star:3】コード値はenumにする

数値定数、文字列定数、コード値などで複数の値があるものはEnumを定義する。
※ Enumがない言語であれば定数にする

Bad
func setStatus(status: String) {
    if status == "0" {
        // 成功時の処理
    }
}
Good
enum Status: String {
    case success = "0"
    case error = "1"
}

func setStatus(status: Status) {
    if status == .success {
        // 成功時の処理
    }
}

想定外の値を考慮する必要があるとき

Enumはあらかじめ定めた値しか取り扱うことができないが、APIのステータスコードなど想定外の値に対しても何らかの処理が必要な場合がある。
そのような場合は、以下のように生のコード値を保持して、ゲッターやcomputed propertyでEnumを取得できるようにするとよい。

struct Response {
    var statusCode: String
    var status: Status? {
        return Status(rawValue: statusCode)
    }
}

func setResponse(response: Response) {
   if response.status == .success {
      // 成功時の処理
   } else if response.status == .error {
      // エラー時の処理
   } else {
      // 想定外のコード値がきた場合はコード値をログ出力
      print("ステータスコード: \(response.statusCode)")
   }
}

:star:3】コメントを書く

分かりづらいコードを説明する

第三者が見て分かりづらいコードにはコメントを書く。
特殊な仕様を満たすためのコードや、バグ修正のためのトリッキーなコードなど、他人が読んだときすぐに意味が理解できないコードには、コメントでコードの説明を記載する。

バグ修正のための特殊なコード

正攻法でバグを修正できず、仕方なく汚いコードをいれざるを得ないときはままある。
そのような場合は、後で見る人のためになぜそうしたのかコメントを記載しておく。

例えば以下のようなケースは、コメントで説明を書いておいたほうが良い。

  • 普通に実行すると上手く動かないので一瞬遅延して実行する
  • なぜか上手く動かないので同じプロパティに2回値を代入する
  • 特殊なケースに場合分けして処理をするため、Booleanのプロパティを作る
  • 要件とは関係ないがシステム的に問題があったためif文で場合分けする

マジックナンバー

以下のコードは3が何を意味するか第三者には分からないため、コメントが必要になる。

Bad
if status == 3 {
   showErrorMessage()
}
Good
// ステータス3は通信エラー
if status == 3 {
   showErrorMessage()
}

コメントに適した文章の書き方

情報のない言葉や重複する言葉を削る

日本語はそれなりに回りくどく、何も考えずに文章を書くとたいてい情報量のない言葉がいくつか含まれる。
文章を書いたら一度見直して、重複する言葉や必要のない言葉がないか確認し、あれば削除して短くする。

例えば、以下のような言葉は言い回しを簡潔にして、意味を変えずに短くすることができる。

  • 「短くすることができる」→「短くできる」
  • 「〜を調整するように修正」→「〜を調整する」

敬語や丁寧語を使わない

日本語は丁寧な言い回しになるほど文字数が増える。
「した」が2文字なのに対して「しました」は4文字になる。
好みの問題もあるが、必要なければ丁寧語や敬語を使わない方がコメントを短くできる。

※お客さん向けのドキュメントなど、TPOに合わせて敬語が必要な場合も当然ある

小学生や外国人に言い聞かせるように

なるべく難しい言葉、言い回し、漢字、専門用語を使わず、簡単でより多くの人に伝わる文章を心がける。
カタカナの外来語も逆に英語圏の人に伝わりづらいことが多いので注意する。

また、専門用語はなるべく使わない方がいいが、プログラムを説明するにあたり必要なものは使っても問題ない。
例えば「Socket通信」という言葉は専門用語だが、「Socket通信」関連のプログラムのコメントでは使わざるをえない場合もある。

様々な国籍の人が参加するプロジェクトであれば、コメントを英語で統一するのも良い。

カッコつけなくていい

一番大切なのは、読む人が理解しやすいこと。
極端な話、読み手が理解できるなら、文法が間違っていても、英語が間違っていても問題ない。
改まって綺麗な文章を書くことに時間をかける必要はなく、フランクでもいいので読み手にとって役立つコメントを書くことが大切。

無責任なコメントもないよりはマシ?

以下のような目を疑うようなコメントも、一応情報ではあるのでないよりマシかもしれない。

// ここでメモリリークします。

// ここの分岐はあまり意味ないかも。。

// なんで動作してるのかわかんないけど、とりあえずここの問題はこのコードで解決した

:star:3】サービスの可用性を意識する

アプリケーションをクラッシュさせない

大前提として、アプリは落ちない方がいい

当たり前のことだと思われるかもしれないが、現代のプログラミング言語は簡単にクラッシュする。
例えば要素数3の配列の4番目にアクセスすれば多くの言語はクラッシュする。
この手のバグは容易に発生するし、ものによっては気づかれないままリリースされることも珍しくない。

不正な状態ならExceptionを吐いてしかるべきという考えも一理あるが、それによりアプリケーション全体がクラッシュするのはよろしくない。

APIやバッチなどバックエンド系のプログラムでは、Exceptionがシステム全体を止めてしまうことはあまりないのでExceptionを積極的に活用するのも良いが、フロントエンドアプリケーションでは、一つのExceptionがアプリを完全停止させてしまうことが多いので、なるべくExceptionを吐かないようにした方が良い。

このようなクラッシュをできる限り防ぐには、以下のような工夫が必要になる。

適切なガードを実装する

クラッシュする可能性があるコードにif文などを加えて、クラッシュする場合は処理を行わなくする。
JavaのようなNULL安全でない言語でのNULLチェックなどがこれにあたる。

JavaでのNULLチェックの例
if (hoge != null) {
    hoge.fuga();
}

しかし、このようなガードはバグを握りつぶして隠蔽してしまうリスクがあるため、開発環境でのみ不正時に処理を止めるassertの仕組みがあれば代わりにそれを用いるのが良い。
例えば、Swiftのassert関数はDEBUGビルドの場合のみ、条件チェックを行い不正な場合にクラッシュさせることができる。

Swiftでのassertの例
func hoge(text: String?) {
    assert(text != nil) // DEBUGビルドならtextがnilの場合ここでクラッシュする
    // 通常時の処理
}

もしくは、if文でガードを入れるとしても、不正な状態が発生したら最低限ログ出力くらいはしておいた方が良いだろう。

そもそもクラッシュしないようにする

可能ならクラッシュするコードがそもそも書きづらい開発環境を作るのが良い。
これはガードを実装するより根本的な解決になる。
例えば、NullPointerExceptionに対する一つの根本解決として「JavaをやめてNULL安全なKotlinを使う」ことができる。

また、言語を変えるまでいかなくても、NULL安全でない言語にOptionalクラスを追加するなど、安全にコードを書くための拡張を追加することはできる。

以下はSwiftのCollectionを拡張して、範囲外のインデックスを指定してもクラッシュせずnilを返すgetter(subscript)を追加した例。

extension Collection {
    subscript (safe index: Index) -> Element? {
        return indices.contains(index) ? self[index] : nil
    }
}

let list = [0, 1]
print(list[safe: -1]) // クラッシュせずnilになる

NULLアクセス、配列の範囲外アクセスなど一般的でよく起こるクラッシュについては、このような関数などを追加することで対策できる。

不正なデータがあってもできる限りのことはする

データの一部に不正な状態や想定外の状態があっても、問題のない部分についてはなるべく通常通り処理が行われるようにする。
例えば、銀行口座の明細が見れるシステムを考える。

利用明細データの1つがおかしいと何も見れなくなるシステムと、利用明細データの1つがおかしくても他の明細と口座残高は見れるシステムを比べると、ユーザーにとっては当然後者の方が使い勝手が良い。

アプリケーションはユーザーをサポートする優秀な執事のように振る舞うのが理想だ(少なくとも私はそう思っている)
一つ予想外のことが起こっただけで全ての職務を放棄するようでは優秀な執事とは程遠い。

ただし、この方針が当てはまるのは情報を見る機能に限定される。
後戻りのできない更新処理などの場合は、異常を検知したらただちに処理を取りやめるのが無難だ。

:star:3】同じ処理を何度も書かない

複数箇所に書かれた同じ処理を一つにまとめることで、コード量を減らし可読性を高めるとともに、修正のコストを少なくすることができる。

汎用的な機能は共通クラスや関数に切り出す

ビジネス要件に左右されない汎用的な機能は、共通のクラスや関数として切り出して利用する。
例えば以下は、文字列をURLエンコードする機能を共通関数として定義している。
※厳密にはcomputed property

extension String {
    var urlEncoded: String {
        var allowedCharacterSet = CharacterSet.alphanumerics
        allowedCharacterSet.insert(charactersIn: "-._~")
        return addingPercentEncoding(withAllowedCharacters: allowedCharacterSet) ?? ""
    }
}

if文やswitch文内の記述は最低限にする

if文やswitch文で処理を分岐させる場合、分岐内に同じ処理があるなら、重複部分はif文やswitch文の外に出すようにする。
例えば、以下は label.text = の部分がifブロックとelseブロックの両方にあり重複している。

Bad
if flag {
    label.text = "aaa"
} else {
    label.text = "bbb"
}

このケースでは重複部分を外に出すとif文自体を消すことができる。

Good
label.text = flag ? "aaa" : "bbb"

共通化のデメリット

ロジックやデータ構造の共通化にはデメリットもある。
同じ処理をむやみやたらと共通化することは、むしろ保守性の低下を招くことも多い。

処理を共通化することには以下のようなデメリットがある

  • ロジックが複雑になり分かりづらくなる
  • 仕様変更により共通化したロジックを場合分けしなくてはいけなくなった場合に修正がたいへん
  • 共通化したロジックの修正が、意図しないところに影響を与えてしまう

特にクラス継承による共通化は強い密結合を生み出し、プログラムの柔軟性を失わせてしまうこともあるので十分注意する。

関連記事
「コードを共通化するために継承しよう」なんて寝言は寝て言えとゆ話
共通化という考え方はアンチパターンを生み出すだけ説

共通化すべきものとするべきでないもの

では、どんなときに共通化するべきで、どんなときに共通化すべきでないか。

最初に挙げたように、ビジネス要件に左右されない汎用的な機能は積極的に共通化を目指す。
ビジネス要件に絡むロジックやデータの場合、共通化するかしないかはケースバイケースだが、以下を満たしていることが共通化の指針になる。

  • 将来的な仕様変更によりロジックの分離が必要になる可能性が低い
  • 分かりやすく実態にあったクラス名や関数名をつけられる
  • クラス継承をする場合、特定のサブクラスでしか使わない関数やプロパティが親クラスに存在しない
  • 呼び出し元や使い方によって、ロジックに場合分けがあまり発生しない
  • 一部の呼び出し元だけに必要なライブラリやフレームワークがない

:star:2】 コードを書く前にスケッチを作る

アプリケーションを作るとき最初にワイヤーフレームやラフスケッチを作るように、プログラムもコードを書き始める前にワイヤーフレームやラフスケッチを作ると効率良く進めることができる。

開発が始まると目に見えた進捗が欲しいため、とりあえずコードを書き始めてしまいがちだが、深く考えずに書いたコードは考慮漏れ、仕様不備、設計不備などにより手戻りが発生することが多い。
とりあえずコードを書けば一見進捗しているように見えるが、下手なコードは最終的に全くの無駄であったり、むしろ害悪であることすらよくある

そのため、最初はコードを書きたい気持ちを我慢して、 プログラムのラフスケッチを作る方がプロジェクト全体で見ると効率が良い。

プログラムのラフスケッチとは

ここで言うプログラムのラフスケッチはクラス図や概略のコードなどを想定している。
ただし、必ずそうある必要はなく、以下のことがわかれば別の形でもいい。

  • どういうクラスがあるか。各クラスの役割と名前
  • クラス間の包含関係と参照関係
  • 各クラスが保持するデータ(プロパティなど)と、他のクラスから呼び出される関数のインターフェース
  • 各クラスがどのようにインスタンス化され、どのように他のクラスと連携するか
  • 各画面遷移やUIイベントで何をするか

クラス図を書く

最初はクラス間の包含関係と参照関係を考えるためにクラス図を作るとわかりやすい。
この段階のクラス図はルール無用、思うがままのスケッチでいい。
クラスのプロパティや関数を網羅する必要もなく、最低限クラスの名前、包含関係、参照関係と役割が分かればよい。

試行錯誤しながら書き直すため手書きで作るのがおすすめだ。
一対NのN側を複数書いたり、包含を線で囲んで表したりして、UML図をもっと簡単で直感的にした形にするとわかりやすい。

クラスの保持するデータとインターフェースを決める

クラスの構成が決まったら、次は各クラスが保持するデータと外部との連携に必要な関数のインターフェースを決める。

保持するデータというのは具体的にはインスタンス変数(メンバー変数)のことだ。
クラス図と異なり、この段階でインスタンス変数はなるべく網羅しておこう。

最初の方の項に書いたが、クラスのインスタンス変数はなるべく少なくする必要がある。
どうしても必要なものだけをインスタンス変数にして、設計後はなるべく予定外のインスタンス変数を追加しないよう心がける。

ラフコードを書く

クラスの構成が決まったら、詳細を省略したラフコードを書いていく。
ただし、ここでは大筋を決めることが目的なので、細かな文法にとらわれず自分が分かりやすい書き方書く。
そのためコンパイルエラーがチェックされるIDEより、シンプルなエディターに書くのがお勧めだ。

ラフコードでは処理の流れと、他クラスの呼び出し方や情報の受け渡しのインターフェースが大まかに決められれば良い。

ラフコードの例
func createSession(request) -> Session? {
    let user = userTable.getByUserName(request.userName)

    if user.アカウントロックされてる {
        throw なんらかのエラー
    }

    if user.passward != request.password {
        userTable.ログイン失敗回数をカウントアップ
        return nil
    }

    ログインしたユーザーの情報をログ出力
    return Session()
}

インターフェースを使ってラフコードをプロダクトコードにする

インターフェースを使うと、細かな実装や未決事項を保留して骨組みになるビジネスロジックを実装することができる。

つまり、コンパイルが通り、そのままプロダクトで使える形でラフコードを作ることができる。
この方法はDIコンテナのあるバックエンドプログラムで非常に有効だ。

以下はDIコンテナを持つSpring bootでインターフェースを活用したラフコードの例(言語はJava)。

このプログラムはファイルサーバーに写真をアップロードして、ファイルのURLをDBから取得したユーザーのメールアドレスにメールで送信する。
インターフェースを使うことで、データベースもファイルサーバーもメールサーバーも、使うライブラリも決めずにビジネスロジックを実装することができる。

Javaのインターフェース活用例
@Service
class FileService {
    private final IMailer mailer;
    private final IUserTable userTable;
    private final IFileUploader fileUploader;

    // Springではコンストラクターの引数に自動でDIコンテナから値がセットされる
    public FileService(IMailer mailer, IUserTable userTable, IFileUploader fileUploader) {
        this.mailer = mailer;
        this.userTable = userTable;
        this.fileUploader = fileUploader;
    }

    void uploadUserPhoto(int userId, byte[] photoData) {
        String fileURL = fileUploader.upload(userId, photoData);
        User user = userTable.get(userId);
        mailer.send(user.emailAddress, "アップロード完了", fileURL);
    }
}

interface IMailer {
    void send(String toAddress, String title, String body);
}
interface IUserTable {
    User get(int userId);
}
interface IFileUploader {
    String upload(int userId, byte[] fileData);
}

このようなインターフェースに簡単なモック実装を入れてシステムのプロトタイプを作ると、DBやサーバーをひとまず無視して柔軟で迅速な開発をすることができる。

仕様書の不備を見つける

仕様書の不備を見つけることもラフスケッチを作る目的の一つだ。

仕様書や設計書をもとにプログラムを書く場合、仕様書や設計書には必ず間違いがある ということを念頭に置く必要がある。
仕様書の間違いに実装の後に気づくと、それまでの作業が無駄になってしまう。

そのためラフスケッチを書く際には、仕様書に不備がないかを意識する。

:star:2】関連するものは近くにおく

関連するコードを同じディレクトリや同じファイルに配置すると、コードリーディングの際にファイルを探したり切り替えたりする手間を減らせる。
例えば、同じ機能で使うファイル群を同じディレクトリに置いたり、特定のクラス内でしか使わないクラスをインナークラスにしたりと、関連するコードは近くに配置してコードを探しやすくする。

ディレクトリを機能で分けるか種類で分けるか

ディレクトリやパッケージにファイルをまとめる場合、大きく分けて機能単位でまとめる方法と、ファイルの種類でまとめる方法がある。

1.ファイルの種類でまとめる例

Storyboard
├ LoginViewController.storyboard
└ TimeLineViewController.storyboard

View
├ LoginView.swift
└ TimeLineView.swift

Controller
├ LoginViewController.swift
└ TimeLineViewController.swift

Presenter
├ LoginViewPresenter.swift
└ TimeLineViewPresenter.swift

UseCase
├ LoginViewUseCase.swift
└ TimeLineViewUseCase.swift

2.機能でまとめる例

Login
├ LoginViewController.storyboard
├ LoginView.swift
├ LoginViewController.swift
├ LoginViewPresenter.swift
└ LoginViewUseCase.swift

TimeLine
├ TimeLineView.swift
├ TimeLineViewController.storyboard
├ TimeLineViewController.swift
├ TimeLineViewPresenter.swift
└ TimeLineViewUseCase.swift

上記はどちらの例も同じファイルを持っているが、ディレクトリの分け方が異なっている。

ファイルの種類でまとめる方法はレイヤー設計がわかりやすく、複数機能で使われるファイルも矛盾なく配置できるが、1つの画面のコードを読むために5ディレクトリにまたがってファイルを探す必要があり煩雑になるというデメリットもある。

この2つの分け方はどちらが良いというものではなく状況に応じた使い分けが必要だが、他の機能から使われることのないファイル群であれば、機能によってディレクトリをまとめた方が開発は楽になるケースが多い。

疎結合と密結合のバランス

一般にプログラムは疎結合である方が汎用性やメンテナンス性に優れるが、本項の方針はそれに反してプログラムの結合度を高めるものだ。
結合度を高めすぎると、1つのファイルが肥大しすぎたり密結合により汎用性やメンテナンス性を損なってしまうが、不必要で過剰な疎結合もまたメリットよりコストが大きくなってしまうというデメリットがある。

何がベストかはケースバイケースで正解のない課題ではあるが、適切なバランスでコードをまとめることは意識する必要がある。

:star:2】 UnitTestを作る

ここで言うUnitTestは手動のものではなく、JUnitなどのプログラムテストを指す。
データを加工などの小さな機能に対して、開発初期からUnitTestを積極的に作成する。

UnitTestの効能

事前確認をすることで全体コストを減らす

システムに問題が見つかった場合、規模が大きい機能ほど原因調査に時間がかかる。
また、後ろの工程になるほどプログラムの修正は他機能への影響や考慮しなければならない点が大きくなる。
小さな機能に対して早い段階でUnitTestによる動作確認をしておくことで、これらのコストを減らすことができる。

実際に動かしてテストするのが難しい処理の動作確認をする

イレギュラーなデータや状態に対する処理は、実際にアプリケーションを動かしてテストするのが難しいケースも多い。
そのようなテストでも、UnitTestであればイレギュラーな状況をプログラムで作り出してテストをすることができる。

クラスや関数の使い心地を確認する

UnitTestを書くことでプログラマーはテスト対象のクラスや関数を実際にプログラムで使うことになる。
このときにクラスや関数の使い心地を体験することで、クラスや関数のインターフェースをより使いやすい形にブラッシュアップすることができる。
また、副次的な効果だがUnitTestを実行するにはプログラムが疎結合であることが求められるので、クリーンな設計の勉強にもなるかもしれない。

仕様を明確にする

テストを作るには何が正しいかを定める必要がある。
テストを考えることにより、イレギュラーケースでどうあるべきかなどの仕様や課題がより明確になる。

UnitTestは開発スピードを上げるためにやる

UnitTestは品質を担保するためではなく、全体の開発スピードを上げるために行う。

そのため、全てのコードを網羅する必要はないし、実装が難しいテストを無理に作る必要もない。
特にUI、通信、DBなどが絡むとUnitTestは様々な考慮が必要になるので、そのようなUnitTestを作成すると開発効率が下がるリスクがある。

また、UnitTestをやったから品質が担保されるという考えもやめた方がいいだろう。
どれだけUnitTestをやろうが、最終的に人力でのテストは必要になる。

小さな機能をテストする

UnitTestは基本的に小さくて独立した機能に対して行うようにする。
大きな一連の操作を自動で確認するプログラムを作る場合もあるが、そういうものはUnitTestとは目的が異なる。

:star:2】 業務ロジックの計算には数値クラスを使う

業務ロジックの計算にはInt、Float、Doubleなどの基本データ型を使わず、JavaならBigDecimal、SwiftならNSDecimalNumberやDecimalなどの数値クラスを使う。

Double、Floatなど浮動小数点数は基本的に業務ロジックに使わない

Double、Floatなどの浮動小数点数は、誤差が生じるので安易に使ってはいけない。
浮動小数点を使うのは、おおむね座標計算などの描画処理と、性能を重視する科学計算に限られる。
間違っても金額の計算にDoubleやFloatを使ってはいけない。

金額の計算に32bitのIntを使用しない

例えばJavaのIntは32bitで最大値が約21億だが、これは金額を扱うには小さすぎる。
21億円というと大金だが、会社レベルの経理ならこれを超える金額は普通にあるし、個人資産でもあり得ない金額ではない。

Intなどの組み込み整数型は桁数に上限があるため、なるべく業務ロジックでの使用は避け、使用する場合は桁あふれするケースがないかを十分考慮する。
金額であれば64bitの整数値(JavaならLong型)を使えばほぼ十分だろう。
64bit整数値の最大値は約900京なので、アメリカの国家予算(約400兆円)の100倍でも余裕でおさまる。

関連記事
なぜBigDecimalを使わなければならないのか

:star:2】クラス、インターフェースを増やしすぎない

クラスは大きくなり過ぎないように心がけるべきだが、過剰にクラスを分割してクラスやインターフェースが増えすぎるのにも以下のようなデメリットがある。

  • クラス結合のためのコードが必要になり全体のコード量が増える
  • クラス同士の関係が複雑になりコードが難しくなる
  • コードを読解・編集する際にファイル切り替が必要になり作業効率が落ちる

クラスやレイヤー構成を設計する際は、これらのデメリットを考慮した上で、メリットがデメリットを上回る設計をする必要がある。

クラスを切り出すポイント

何らかの機能をクラスに切り出す場合、その機能は以下のどれかである可能性が高い。

  • 再利用性(汎用性)がある
  • 単独でテストする必要がある
  • 単独でデプロイ(リリース)する必要がある

逆に、再利用性がなく単独でテストもデプロイもしない機能群は、クラスを分けずに一つのクラスとして提供した方が良い可能性が高い。
機能を疎結合にすることはシステムの保守性を保つのに役立つ考え方だが、そこにはコストやデメリットもあり疎結合が常に密結合より優れているわけではない。

例えばiOSアプリでは、特定の画面に対してViewControllerとカスタムのViewをそれぞれ作成する設計パターンがあるが、そのようなViewControllerとViewは必ず1対1で紐付き、再利用性がない上、単独でテストやデプロイもしないので、ViewとViewControllerに分けずに1つのクラスにした方が良いケースが多い。
※LoginViewControllerに対してLoginViewを作成するようなケース

有名な設計パターンを機械的に模倣しない

過剰なクラスの細分化は、しばしばDDDやクリーンアーキテクチャーなど有名な設計パターンの機械的な模倣によって生まれる。
設計パターンを紹介する書籍や記事は、設計パターンの良いところばかりを強調し、悪いところや面倒な点には触れない傾向にある。
プロダクトやチームの規模によって最適な設計は変わってくるので、既存の設計パターンを機械的に模倣するのではなく、実際に自分たちのプロダクトに適用してメリットがデメリットに勝る形であるかを十分検討する。
とはいえ、特定の設計パターンがどのようなデメリットを秘めているかは一度試してみないと把握するのが難しい。
最適な設計パターンは、検討、実行、フィードバック、改良といったPDCAサイクルを回して徐々に洗練させていく必要がある。

アプリケーション全体に画一的に同じレイヤー構成を適用しない

例えば、Hello Worldを出力するだけのプログラムにたくさんのクラスや複雑なアーキテクチャーを適用するのは無駄である。
やることがあまりないので、何もすることのないクラスやレイヤーが出てきてしまう。

どういったクラスやレイヤーを用意するのが妥当かは、実装する機能の性質や複雑さによって変わる。
何にでもマッチする都合のいいクラス構成やアーキテクチャーはないのだ。

一般的なアプリケーションには複数の画面や機能があり、性質や複雑さはそれぞれ異なるので、それら全てに同じレイヤー構成を当てはめれば、無駄やミスマッチが生じることになる。
そのため、すべての機能に同じ構成を適用するのでなく、もっと柔軟に、機能ごとに適切な設計を選択していく方が良いと思われる。

また、同じ機能であっても、要件や仕様の変化にともない最善の構成は変わっていく。
良い設計を保つためには、大胆なリファクタリングや、既存のコードを完全に捨ててしまうこともときには必要になる。

レイヤーのショートカット

クリーンアーキテクチャーなどレイヤーが細かく分かれたアーキテクチャーでファイル数を抑える1つの方法は、レイヤーのショートカットを許容することだ。
例えばプログラムが以下のようなレイヤー構成を持つ場合でも、UseCaseとPresenterでやることがなければ、Controllerが直接Entityを参照すれば良い。

  • Entity
  • UseCase
  • Presenter
  • Controller
  • View

前項に記載したとおり、レイヤーの構成はアプリケーション内で統一する必要はなく、もっと自由に機能によって変えてしまって良いと思う。
ただし「依存の向きを意識する」の項に記載したとおり、EntityからViewを参照するような通常と逆向きの参照はしないようにしよう。

インターフェース(プロトコル)をクロージャに置き換える

関数型言語など関数を引数として受け渡しできる言語であれば、メソッドが一つしかないような簡単なインターフェース(プロトコル)を関数渡しやクロージャに置き換えることができる。
以下はPresenterのOutputをprotocolから関数渡しに置き換えている。

Before
class LoginPresenter {
    let output: LoginOutput
    init(output: LoginOutput) {
        self.output = output
    }
    func login() {
        output.loginCompleted()
    }
}
protocol LoginOutput {
    func loginCompleted()
}
After
class LoginPresenter {
    func login(onComplete: () -> Void) {
        onComplete()
    }
}

同じようなデータモデルを複数作らない

レイヤーの分離に執着し過ぎると、同じようなデータモデルが無意味に複数できてしまう場合がある。
例えば、ドメイン層にUserクラスがあり、その情報をUseCaseに渡すためのUserDTOがあり、さらにViewで使うためのUserViewModelがあるが、3つがほとんど同じコードになってしまうようなケースだ。
ほとんど同じでも分離しなければならないケースもあるが、分けずに使い回して良いケースもある。
以下の条件を満たすなら複数のレイヤーをまたいで上層のデータモデルを使っても良い。

  • データモデルがDB、フレームワーク、ライブラリなど特定のアーキテクチャーに依存しない
  • 上層のデータモデルが下層のクラスに依存しない(「依存の向きを意識する」の項に記載のとおり)

:star:2】Exceptionをcatchして握りつぶさない

基本的にExceptionはcatchせず呼び出し元にthrowし、上層のレイヤーで一括してエラー処理をするようにする。
Exceptionをcatchする場合は以下を心がける。

  • 適切なエラー処理をする
  • エラー処理ができないのであれば、エラーログを出力する
  • 明確な意図がありログすら出さずExceptionを隠蔽する場合は、その理由をコメントで記載する

この項に記載する内容は「サービスの可用性を意識する」の項に書いていることと少し矛盾するが、上層のレイヤーで適切にExceptionを処理できるならそれが一番良い。

「サービスの可用性を意識する」の項に記載した方針は、上層レイヤーでExceptionを適切に処理しきれない可能性があり、それがシステム全体のクラッシュにつながるなら、Exceptionを吐かないようにしようというものだ。

例外をロジックの分岐に使わない

例外を上層で処理するとはいえ、通常の制御フローで例外をif文のように条件分岐に使うのはやめよう。
例外はあくまで例外であり、例外的な状況以外では使わないよう心がける。

Javaのチェック例外(検査例外)を大人しくさせる方法

Javaの標準のExceptionはチェック例外(検査例外)になり、throw すると呼び出し元の関数はcatchするか再throwするかのどちらかを強いられる。

そのため、最上層で例外を処理させようとすると、例外が発生する関数の呼び出し元に芋づる式に throws をつける必要が出てきてしまう。

ときにこれは鬱陶しい仕様だが、かといって例外をcatchして隠蔽したくもない場合、RuntimeExceptionにラップしてthrowすることで、呼び出し元に変更を行わず例外をthrowすることができる。

try {
    "ABC".getBytes("UTF-8");
} catch (UnsupportedEncodingException e) {
    // IllegalStateException は RuntimeException なので上層にcatchを強制しない
    throw new IllegalStateException(e);
}

:star:1】 外から参照されない変数や関数はprivateにする

クラスの外から使われないプロパティや関数はprivateやprotectedにする。

クラスはコードやドキュメントを見なくてもコード補完で何となく使い方が分かるのが望ましいが、privateやprotectedにしないと外から使われる前提でないプロパティや関数もコード補完で候補として出てしまい、使いづらくなる。

ただし、privateなどのアクセス修飾子がないプログラミング言語もあり、その場合は特殊な実装でprivateを実現するより、シンプルで標準的なコードであることを優先して、privateを諦める方が良い。
どうしてもprivateを導入したいなら、そもそも言語を変えることをお勧めする(JavaScriptからTypeScriptにするなど)

また、privateメソッドは別クラスのpublicメソッドに処理を切り出すことで実装がすっきりすることが多いので、汎用的な処理であったりクラスサイズが大きい場合はクラスの切り分けも検討する。

関連記事
privateメソッドは不要

:star:1】 三項演算子に複雑な記述を組み込まない

三項演算子のネストは分かりづらいので避ける。

Bad
flag ? subFlag ? a : b : c

ネストが必要になる場合は、途中の演算結果をローカル変数(説明変数)に入れてネストを解消すると良い。

Good
let aOrB = subFlag ? a : b
flag ? aOrB : c

また、三項演算子内に長い式や長い関数のチェインなどが含まれる場合も、それぞれの結果をローカル変数に入れてから、三項演算子で使う。

Bad
flag ? (1 + 2) * 3 - 4 : 5 % 6 * 7
Good
let a = (1 + 2) * 3 - 4
let b = 5 % 6 * 7
flag ? a : b

関連記事
三項演算子?:は悪である。

:star:1】 true/false をベタ書きしない

初心者は以下のように true false をコード内にベタ書きしがちだ。

Bad
var isZero: Bool {
   if number == 0 {
      return true
   } else {
      return false
   }
}

しかし上記は number == 0 がBoolを返すのでtrue/falseを書く必要はなく、より簡潔に以下のように書ける。

Good
var isZero: Bool {
   return number == 0
}

必ずtrueを返すケースや必ずfalseを返すケースではtrue/falseをべた書きする必要があるが、上記の例のように何らかの判定結果を返す場合は、true/falseをベタ書きするのは避ける。

ただ、この書き方は初心者には分かりづらい。チーム内に初心者がいる場合は説明してあげるのが良いだろう。

関連記事
読み手にやさしい if 文を書きたい

:star:1】 enumの持つコード値を使わない

SwiftのenumのrawValueのように、enumは何らかのコード値を保有することが多い。
しかし、このコード値を判定などに使ってしまうと、enumの存在意義が半減してしまう。

Enumのサンプル
enum Status: String {
   case success = "0"
   case error = "1"
}

例えば上記のようにコード値をもったEnumがある場合、コード値を判定などに使わず、Enumを直接使うようにする。

Bad
if status.rawValue == "0" {
}
Good
if status == .success {
}

引数で渡す場合もコード値は使わず、Enumを直接渡す。

Bad
checkStatus(status.rawValue)
Good
checkStatus(status)

そもそもenumにコード値を持たせるべきではない?

「コード値はenumにする」の項と矛盾するが、そもそもenumがコード値を持っていなければ、このような間違いは生まれない。

DBやAPIのコード値は外部仕様であり、アプリケーションの仕様とは分離すべきものと考えると、コード値は Repository レイヤーでenumに変換してやりenumにはコード値をもたせない方が綺麗なプログラム設計になる。

:star:1】静的コードチェックツールを活用する

Lintなどの静的コードチェックを積極的に活用する。

静的コードチェックは人の手を使ずローコストでコードをチェックできるので、やっておいた方がお得である。
問題を検知するだけでなく、どのようなコードに問題があるかを知る勉強にもなる。

ただし、静的コードチェックにもデメリットはあり、大量のワーニングが出たとき真面目に全てをチェックすると非常に時間がかかる場合がある。
ワーニングが出たからといって直ちに問題があるわけではないので、時間を使ってワーニングを潰してもアプリケーションのクオリティはさほど変わらないということもある。

静的コードチェックはあくまで問題を見つける手助け程度に考えるのが良いかもしれない。

:star:0】その他雑多なプラクティス

詳細な説明は省くが、その他雑多なプラクティスを列挙する。

  • 関数の引数で渡す情報は必要最小限にする
  • パフォーマンスの最適化を早期にしすぎない
  • 大小比較には<を使い>を使わない
  • 関数の引数を出力に使わない
  • コードのインデントを揃える
4243
ユーザー登録して、Qiitaをもっと便利に使ってみませんか。
  1. あなたにマッチした記事をお届けします
    ユーザーやタグをフォローすることで、あなたが興味を持つ技術分野の情報をまとめてキャッチアップできます
  2. 便利な情報をあとで効率的に読み返せます
    気に入った記事を「ストック」することで、あとからすぐに検索できます
alt_yamamoto
株式会社アルトノーツ代表。 https://recruit.altonotes.co.jp

コメント

@matsuda-hiroki ありがとうございます!神って。。

typo修正 by matsuda-hiroki 2020/01/21 19:48

1

@htsign ありがとうございます!コピペミスかな。

自己言及的なアレですか? by htsign 2020/01/21 22:27

0

@daikikatsuragawa ありがとうございます!最後の長音は基本つける派です。

表記ゆれの修正 by daikikatsuragawa 2020/01/22 00:01

0

@mui_nyan
編集リクエストありがとうございます。

内容と比べると、 「少なくする→スコープを小さくする」 が適切かなと思いました

「スコープを小さくする」も言いたいことの一つなのですが、この項で言いたいのは「変数はなるべく使わない、使うならスコープを小さくする」ということで、「少なくする」方が優先度が高いのです。

確かに現状タイトルがしっくりこない部分もあるのですが、いい文言が思いつかないのでいったん現状のままでいかせてもらいます。

1

@alt_yamamoto
コメントありがとうございます。
そういう意図であれば納得いたしました。

現状だと内容のほとんどを「グローバルは悪」的な節が占めているので、
「変数より定数」(jsなら可能な限り const を使う とか) などのような節もあるといいかもと思いました。

1

「変数を少なくする」とはいったいどういうことで、どうやって実現するのかに対する説明も不足してる気がしますね。。。
時間ができたら追記するかもしれません。

0

@mui_nyan
やはりいったんご提案頂いた「変数のスコープを小さくする」を採用しました。
「少なくする」だと使いまわしたりする、みたいな誤解を招きそうだなと。

そのうち本文含め修正するかもしれません。

1

「【:star:4】フラグ(Booleanのプロパティ)を作らない」があんまり汎用的な内容になっていないような気がしてちょっと気になっちゃいました。
記載の例がたぶんスマホアプリの ViewController の実装で出てくるパターンだと思うのですが、例として記載のものをあげつつ「状態を多重で管理しない」という趣旨になれば汎用的になるかと思いました。

同じ節の「複数のフラグを1つの状態変数にまとめる」はまとめちゃうフラグ同士が直交している場合は問題ないですが、直交していない場合は「一つのフィールドに複数種類の情報を持たせない」のアンチパターンになるので、汎用的なテクニックとして紹介してしまうのは微妙な気がしました。

0

@nirasan ご意見ありがとうございます。

おっしゃるとおり、スマホアプリの例はプラットフォームが限定的なのにその説明なくて汎用的じゃないですね。。。
他もそうなんですが例を考えるのが難しくて、結構頭を悩ませてます。

直交してない場合にアンチパターンになるのもそのとおりです。
修正しようと思いますが、どう直すかはちょっと考えます。

2

フロントエンジニアです。
素晴らしい知見の共有をありがとうございます。
いいね100個つけたいです。

1箇所だけ情報更新したほうがいいと思われる箇所があるので、日本のエンジニアの皆さんのために、確認をお願いします。

(少し古い※2015年以前)
「グローバル変数を全く使わない設計は可能か?
必要な情報を全てバケツリレーのように引数で渡していけば、グローバル変数を全く使わずにアプリケーションを作ることもできる。
(中略)
グローバル変数の使用を避けろとは言うものの、全く使わないのも現実的ではなく、適切な方針のもとでグローバル変数を使用することが最善」

(2018年からの方法)
ESModuleがIEを除く全ブラウザで使用可能です。※IEも、BabelでコンバートしてWebpackでバンドルすれば可能です
https://developer.mozilla.org/ja/docs/Web/JavaScript/Guide/Modules
【メリット】
・モジュールを使えば、グローバル変数は全く使う必要がなくなります
・変数のバケツリレーも不要です
・メンテナンス性・可読性も向上します
・コードの再利用可能性も高まります
・さらに、チームで開発する場合は、コンフリクトも回避できて、大規模なプロジェクトをスムースに進めることに貢献します
【デメリット】
・実行スピードが遅い(Webpackでバンドルしたソースを使うようにすれば、このデメリットは回避できます)

1

@teraoka_kennosuke さん
横からで恐縮ですが、

どこからでもアクセス可能なシングルトンや共有オブジェクトなどもグローバル変数として話を進める。

とあるため、JSにおける globalThis もグローバル変数であると言えると思います。

0

@teraoka_kennosuke
ご意見ありがとうございます。

確かにJavaScriptにおける「グローバル変数」を使わない設計は可能です。
ただ、@htsign さんのコメントの通り、本記事での「グローバル変数」はJavaScriptのグローバル変数よりもっと広い意味で使っています。

「グローバル変数」を「どのプログラムからでも読み書きできるデータ保存領域」くらいに読み替えてもらえると伝わりやすいかと思います。

0

マジックナンバーはenum化したらもっと読みやすくなります。

0
(編集済み)

チーム内に初心者がいる場合は説明してあげるのが良いだろう。

私ケースの場合、(自分も含め)これが強いので、多少理想から外れても、初心者が取っつきやすいコード寄りにすることが多いです。
今後少子化によりマンパワー不足が考えられるので、この比重が大きくなるんじゃないかなぁとふと思いました。

0

@shoridevel

マジックナンバーはenum化したらもっと読みやすくなります。

同意です!
「【:star:3】コード値はenumにする」に似た内容を書いているのでそちらもご覧ください。

0

@tatsubey

私ケースの場合、(自分も含め)これが強いので、多少理想から外れても、初心者が取っつきやすいコード寄りにすることが多いです。

私もそれは重要な観点だと思います。
プログラムは目的を達成するための道具なので、綺麗なコードを書くことはあくまで一手段であり、それ自体が目的になってはいけないと思います。

例えばどんだけ破茶滅茶なプログラムでも目的を達成できてみんながハッピーになるなら、それはそれで良いですよね。

ただ、私は仕事で色んな人のコードレビューをしていて、中には炎上案件も多くあり、こういう風にしたら苦労しなくて済むのにと思うことが多くあったので、それがモチベーションになってこのガイドを書きました。

どこの誰を基準にするべきかというのはよく悩みます。

一生懸命教えてもすぐ辞めてしまう人もいれば、一人で勝手に吸収して成長していく人もいます。
初心者をケアするのは大切なことですが、人には向き不向きもあるし、それとは別にプロジェクトを成功させることも大事だし。

なんかまあ別に結論はないんですが、こういうガイドがみんながいい感じにやれる一助になればいいなと思います。

6

リーダブルコードですね!

↓これを見ていて、ふと考えていたのですが、

var isZero: Bool {
return number == 0
}

必ずtrueを返すケースや必ずfalseを返すケースではtrue/falseをべた書きする必要があるが、上記の例のように何らかの判定結果を返す場合は、true/falseをベタ書きするのは避ける。

ただ、この書き方は初心者には分かりづらい。チーム内に初心者がいる場合は説明してあげるのが良いだろう。

boolean isZero = number==0;
return isZero;

こんな書き方なら何をやりたいコードなのか初心者でも伝わりやすい?
特にbooleanはreturnやifの中に直接演算式を書くより、ワンクッション置いてあげたほうが可読性が高くなると思っているので、僕はよくやる手法なんですが、
無駄に変数を作るのは、スコープ問題とトレードオフなんでしょうか?

1

@sakamoto_t

isZeroみたいなのを「説明変数」と言いますが、説明変数はちゃんと意味があるのでガンガン使って良いと思います。
説明変数は「変数」という名前に反して書き換えることがないので、増やしてもあまり害がないです。

できれば書き換え不可な定数にしたいところですが、定数にするのにfinalとかconstとか足さないといけない言語だと面倒なので、そういう場合は普通の変数でもいいかなと思います。

2
(編集済み)

概ね同意の嵐でしたが、一点だけ。

外から参照されない変数や関数はprivateにする

つまりオブジェクト指向における実装の隠蔽・カプセル化ですが、その主な目的は

  • オブジェクトの内部状態を外部から不正に操作されるコストをなくす
  • オブジェクトの内部実装変更の際、オブジェクト使用側の既存コードの変更を不要に(少なく)する

であり、個人的には設計上スコープ管理と同レベルに重要な点だと考えているのですが、いかがでしょうか。

0

素晴らしい記事でした。得るものが多かったです。

ところで一つ、ここのリスト内では言及されていないものですが、ご意見を頂きたいことがあります。

私はコードを書くときに「基本的に既存のプログラムのスタイルや文化を踏襲する」ってことを気をつけています。たとえここのリストのルールとバッティングするような場合でも大抵は既存のプログラムの文化を優先しています。

というのも、各所でいろんな書き方をしているプログラムっていうのは、俯瞰しずらくまた読みづらいものです。またコードの検索するときに検索する文字列が一様でなくなるため、探しづらくもあります。つまり、メンテナンス性が低くなります。

例えば、フレームワークを使うならそのコードの書き方、命名規則、ファイルの置き方のルールをできるだけ曲げないようにしています。誕生した時代が古いものや、プログラムの作り方に対する考え方の違いで、ここのルールに合わないものも結構あると思いますが、フレームワークのマニュアルやそこのコミュニティの書き方を優先してます。
また、前任者がいるプログラムを編集するなら、それがたとえ悪い書き方と思ってもその書き方を真似ます。

そうやって、できるだけそのプログラムの全体の一貫性を保って、全体像を想像しやすい状態にすることは、星いくつの価値があるのでしょうか。
それとも、このルール自体に問題があるでしょうか。

1

@Rijicho_nl

不特定多数の人に使われるライブラリーなどを作る場合、記載された通りカプセル化を意識することはとても重要だと思います。

ただ、チームでアプリケーションを作るケースに置いてはそこまで大きなメリットはないのでは、というのが重要度星1の理由です。

  • 外から呼ばない想定でprivateにしても、将来的にprivateが邪魔になることはよくある
  • private をつけても開発メンバーは誰でも簡単に消せてしまう
  • 何をprivateにするべきかを初心者に教えるのは難しい
  • privateメソッドはUnit testしづらい

などなどコストに対してプロジェクトでの実利が少なく感じるのですね。

まあ重要度の星はあくまで私の主観なのでそんなに厳密ではなく、星2くらいで良かったかなとも思います。

1

@oskbt

まず、フレームワークのルールに従うことはとても重要です。
フレームワークのルールはみんなの道標になりますし、ルールに従わないと正しく動かないこともあり得ます。
この記事のバッドプラクティスに該当するとしても、フレームワークのルールを優先すべきと思います。

星で言えば最高の5よりもっと重要かもしれません。

そうやって、できるだけそのプログラムの全体の一貫性を保って、全体像を想像しやすい状態にすることは、星いくつの価値があるのでしょうか。

これも結構重要だと思います。状況によりますが星5か4くらいですかね。

例えば「名前にIDをつけるな」と書きましたが、View001、View002、View003ってクラスが既にあって新しくViewを足すなら、私もView004にすると思います。

もし気に入らなければ、ルールを破るではなく、ルールを変える必要があると思います。

View001の例で言うなら、

  • IDがついた既存クラスを全てリネーム
  • 今後名前にIDを使わないことをチームメンバー全員で合意する

ここまで徹底できるなら変更もありですが、中途半端に一部だけ変えて一貫性を崩すのは、おっしゃる通り可読性を損なうのであまりよろしくないと思います。

1

@alt_yamamoto 返信ありがとうございます!

private をつけても開発メンバーは誰でも簡単に消せてしまう
何をprivateにするべきかを初心者に教えるのは難しい

確かに、アクセシビリティの扱いを初心者に徹底させるのは現実的ではないかもですね……
初心者教育に重点を置くと☆は下がりそうです。

ただ、他のメンバーの書いたコードにむやみに手を加えるのはそもそも作業領域の分割に失敗しているのでは、とも思いました。
「既存コードのprivateには触れるな」と定めてしまえばいいような気がしますが、これについては開発形態やリリース後の運用にもよるのでしょうか。

外から呼ばない想定でprivateにしても、将来的にprivateが邪魔になることはよくある
privateメソッドはUnit testしづらい

外に公開することにした場合のpublic化の手間は必要経費かなと。

Unit testでprivateが邪魔になるという視点はなるほど、考えが及んでいませんでした。(テスト駆動開発の経験の少なさが露呈してしまった……)
ただ素人考えとしては、privateな部分のテストはクラス内に書けばよいですし、複数のオブジェクトを外側から見るテストでprivateな部分の呼び出しが必要になるのはクラス設計がおかしいのでは? とも思います。そう簡単な話でもないのでしょうか。

この点については言語や開発領域の差もある気がしますね。
私はもっぱらUnity/C#でゲーム作ってる人ですが、最近Unit test用のパッケージ(NUnit)の存在に気づいたので、触れてみつつ考えてみます。

0

@Rijicho_nl

ただ、他のメンバーの書いたコードにむやみに手を加えるのはそもそも作業領域の分割に失敗しているのでは、とも思いました。

たいがい作業分担は決めるものなので「誰でも簡単に消せてしまう」はちょっと言い過ぎだったかもしれません。

ただ、私の周りのプロジェクトでは、最初にコードを書いた人と別の人がそれを修正するケースは普通にあります。
理由は様々で、担当換えすることもありますし、人の入れ替わりもありますし、A社が作ったものを、B社が引き取るとかもあります。極端なケースだと誰が作ったか分からないものを直す場合もあります。

その点については、@Rijicho_nl さんの周りのプロジェクトとは結構状況が違うかもしれません。

「既存コードのprivateには触れるな」と定めてしまえばいいような気がしますが、

他人の作ったコードのprivateを外す必要がでてきた場合、作った人と相談するのが真っ当だと思いますが、そもそも作った人がもういなかったり、すぐに連絡がつかなかったりということもよくあります。
privateを外すときは「リーダーに確認する」みたいなルールを作ったとすると、リーダーの時間を結構削ってしまう可能性もあります。

そういうことを考えると「触れるな」はちょっと厳しいかなと思います。
適切なカプセル化を自分で設計できる人にとっては無用な足かせにもなってしまいますし。

ただ素人考えとしては、privateな部分のテストはクラス内に書けばよいですし、複数のオブジェクトを外側から見るテストでprivateな部分の呼び出しが必要になるのはクラス設計がおかしいのでは? とも思います。そう簡単な話でもないのでしょうか。

privateな部分のテストをクラス内に書くのは絶対なしではないですが、テストコードが増えるとクラス内に本来の責務ではない機能が多く入ってしまうので、私はできればテストを別クラスに切り出したいですかね。
あと、テストフレームワークはテストクラスを別途作ってテストする仕様になっていることが多いというのもありますね。

複数のオブジェクトを外側から見るテストでprivateな部分の呼び出しが必要になるのはクラス設計がおかしいのでは?

想定してたのは複数オブジェクトを使うような場合ではなく、privateな1メソッドを単体でテストしたいような場合です。そのような場合は、privateメソッドを別クラスのpublicメソッドに切り出してUnit testするのがベストプラクティスと思ってます。

そういえば、以前コードレビューしていてこんなやり取りがありました。

私 「ここなんでprivate消したんですか?」
Aさん 「Unit testできなかったので」
私 「そういう場合は別クラスに切り出してpublicメソッドにした方が綺麗ですよ」
Aさん 「しかし時間もないですしリスクもあるので今からの修正は避けたいですね」
私 「そうですか。。。ではしょうがないですね」

内心メソッドに切り出すくらい簡単だろと思いましたが、明確なリスクがあるわけではないので、その言葉は飲み込みました。

2

@alt_yamamoto 丁寧なご回答ありがとうございます!

なるほど、人の入れ替わり……そうなってくるといろいろ難しそうですね。

丁寧な設計をするには時間がかかる、これは私も常々感じているジレンマですが、お仕事だと納期がありますもんね……

テストコードが増えるとクラス内に本来の責務ではない機能が多く入ってしまう

確かに、失念していました。
うーん、privateを一時的に無視できる言語機能やコンパイルオプションが欲しいところですね。濫用のリスクが高いですが。
C#だとReflectionがありますが、毎回いちいち書くには煩雑ですし……

趣味で開発しているだけでは得られない視点を多く勉強させていただきました。
お付き合いいただき、ありがとうございました。

0

フラグ(Boolの変数)が持つ情報はある一時点での判定結果であり、基本的に過去の情報になる。

フラグ(Boolの変数)はある一時点での判定結果とのことですが、
なぜそうなのかが分からなかったので、ご教示をお願いできますでしょうか。
例えば、小売店と1年契約を行い、その小売店の扱う支払方法は、口座振替は有り、クレジットカードは無し、ソフトバンクでの支払いは有りにした場合、その3つのフラグは1年間変わらないと思います。
このようなものでも一時点での判定結果として扱うのでしょうか。

0

@tacchi

挙げられらた例で考えてみます。

小売店の支払い方法はデータベースに保存されているとします。
プログラムからデータベースを読み込むと、データベースの情報がフラグ変数に保存されます。

しかしその後データベースの支払い方法が変わっても、先ほど保存したフラグの値は自動的には変わりません。
フラグが一時点の判定結果というのはそういう意味です。
プログラムで保存したフラグ変数の値はマスターデータではなく、そのコピーになるということです。

この例では、一時的にフラグ変数を使う分にはさほど問題はないと思います。
ただ、例えばデスクトップアプリでフラグ変数をずっと持ち続ける場合、アプリを1ヶ月起動し続けたりすると、データベースの情報と齟齬が生じて問題が起こる可能性があります。

この問題はフラグに限らず変数全てに当てはまるもので、問題の本質は別の項に書いた「データを重複して保持しない」「変数のスコープと寿命を小さくする」と同じなのですが、フラグは頻繁に変わる情報を保持するのに使われがちで、特に問題になりやすいため一つの項目として記載しています。

0

ご説明をありがとうございます。

まだちょっと分からなかったので質問させていただけますでしょうか。

boolean型は自動的にメンバ変数から外すのでしょうか、
それともboolean型でもメンバ変数に入れても良い場合があるのでしょうか。

0

@tacchi

boolean型でもメンバ変数に入れて良いときはあります。

どんなときなら良くて、どんなときならダメなのか、本文に説明が不足していると感じたので追記を検討してみます。

0

ご説明をありがとうございます。

どんなときが良いのかという記事をご検討とのことで、期待しております。

0

三項演算子ですが、早期returnの応用で早期に除外条件を書くと読みやすく、なるようで、自分は工夫して使っています。

flag ? subFlag ? a : b : c

!flag ? c
: !subFlag ? b
: a

よろしければお試しください。

0

良記事ありがとうございます。
鋭いテクニックも多く含まれていて、大勢の方の役に立つ記事ですね。

気になる所があったので追加でコメントします

function(object)の形よりobject.function()の形を好む

星5なのが、自分にはちょっと残念なのですが

以前ははやったと思いますが、最近では stringなど既定の型に対するメソッド拡張=prototype拡張などは,ライブラリの名前の衝突などが起きるために、現代的にはバッドパターンなテクニックになっています。prototype汚染とか言われてさけられているテクニックです。

誰もやらないと思いますし、私も他の人がやってたら止めます。

stringをグローバルな名前空間としてとらえると、そこに拡張メソッドを入れるとグローバル関数と同じようなぶつかる危険性があるからです。そして特定のstring.xxxがないと動かないコードは、それを組み込む必要があるため、再利用性低くなってしまいます。

object.function()の方が再利用性が高い

こちらも、書いていることがちょっと変かな、と思いました。

参照透明性とテスタブルな視点からいうと、object.function形式で、オブジェクトに依存する関数は比較的テスト書きにくいです。シングルトンのデメリットと似ています。

可読性ですが

string1 = StringUtils.convertA(string1);
string1 = StringUtils.convertB(string1);
string1 = StringUtils.convertC(string1);

これは冗長かもしれませんが読みにくくはないです。単に文字数は多いかもしれませんが、だからといって読みにくい事はないです。

デバッグなどでの動作調査時には、メソッドチェーン方式で組まれたコードだと、convert内にconsole.log 埋め込みなどをせざるおえません。素早く不具合みつけたいのに、convertA/B/Cのどれに問題があるのかをコードジャンプして様々な所にlogを仕込むよりかは、

関数に対する入出力を呼び出し側のほうで記録できる関数タイプのほうがメリットがあります。

メソッドチェーンが可読性が高いというのは、既存コードをちょっと変えたい気分的な流行というか思い込みな部分が多く、実際には使いにくい場面が多いように思います。

JSではPromiseが.then.catchのメソッドチェーンを採用していますが、これは非同期処理だからこうなっている面が大きく、非同期ではないのにメソッドチェーンの方が望ましい場面はほぼみかけたことがないです。

0

もうひとつ。

if文やswitch文内の記述は最低限にする

ですが、

if (flag) {
  return {
    result: true,
    message: 'aaa'
  };
} else {
  return {
    result: false,
  }
} 

このような戻り値にしたい場合は、三項演算子では対応しにくいので、あえて、はじめから if で書いて分岐させておいたままにしておく、という対応もよくやったりします。

分岐後の一方だけに何らかの処理をさせたいという変更が発生する可能性はよくあります。

テクニックの理由が重複が減って文字数が減るから、となってるのですが、どうせコードはコピペできるのですから、入力文字数が少ない=正しいということはなく、可読性や、変更対応性を考えるべきかと思いました。

1

@standard-software

だいぶ反応遅くなってしまいましたが、、

三項演算子ですが、早期returnの応用で早期に除外条件を書くと読みやすく、なるようで、自分は工夫して使っています。

確かに除外するものから判定していける場合は、その方が読みやすくなりそうですね。

以前ははやったと思いますが、最近では stringなど既定の型に対するメソッド拡張=prototype拡張などは,ライブラリの名前の衝突などが起きるために、現代的にはバッドパターンなテクニックになっています。

これは私がStringで例を書いてしまったのが悪かったのですが、記事の内容は既定の型に対するメソッド拡張というよりは、カスタムクラスに対するメソッドを主に意識していました。

prototype汚染という言葉もJavaScriptでは忌避されていることも知っておりますが、SwiftやKotlinはこれに対してやや寛容な文化のように感じます。
(もちろん名前がぶつかる危険性はありますが)

1

@standard-software

ちなみに object.function()の方が再利用性が高い これはこの記事の中では一番反対意見が多そうな感じがします(笑)

デバッグなどでの動作調査時には、メソッドチェーン方式で組まれたコードだと、convert内にconsole.log 埋め込みなどをせざるおえません。

これまたミスリーディングな例で申し訳ないですが、メソッドチェーンを推す意図はなく、関数が入れ子になるよりは見やすいという程度の意図でした。
メソッドチェーンが実際には使いづらい場面も多いというのは同意です。

メソッド形式であっても、チェーンせず以下のように入出力を保存することはできると思います。

string1 = string1.convertA();
string1 = string1.convertB();
string1 = string1.convertC();
1

参照透明性とテスタブルな視点からいうと、object.function形式で、オブジェクトに依存する関数は比較的テスト書きにくいです。シングルトンのデメリットと似ています。

これについては後ほど。。。

0

記事の内容は既定の型に対するメソッド拡張というよりは、カスタムクラスに対するメソッドを主に意識していました。

なるほど。そうすると、prototype は普通につかいますよね。
また、最近のJSもTSも class構文などで prototype を意識せずに継承やメソッド拡張がしやすいかもしれません。

SwiftやKotlinはこれに対してやや寛容な文化のように感じます。

JSはなんか、ある時期から一気にprototype汚染をさける文化になったような気がします。
npm がはやって、どんどんライブラリが増えまくりすぎたから、なのかもです。

こうじゃない、ああじゃない、といくつか書きましたが、
他のところは、多々、そのとおり!と納得させていただいたり、参考にさせていただいています。
よいガイドライン、ありがとうございます。

0
(編集済み)

本記事での「グローバル変数」はJavaScriptのグローバル変数よりもっと広い意味で使っています。

JavaScriptにおいてはグローバル変数は使ってもよい(極端に避けなくてよい)という認識でよいでしょうか?

たとえばVue.jsはWindowプロパティを汚染すべきではない思想のもとで作成されていますが、
Windowプロパティを利用しないためにVuexという状態管理ライブラリを導入する必要があるように、
「Window汚染を避けることを貫くためにグローバルに参照できるオブジェクトを作る」ような本末転倒な行為がJavaScriptでよく見られる気がしてて参ってます…。


変数の名前付けですが、もうこれは本当にセンスもあると思うのですが
基本的に「なにをする関数か(抽象的に)」=「目的」を名前にしてほしいと思ってます。
「どういう工程を実行する関数か(具体的に)」=「手段」を名前にされると「それは読めばわかる(ように書いて)」って思います。

「どういう時に発動する関数か」という名前も、いちいち中身を見に行かなければならないので汎用は避けてほしいです…
もちろん「こういう時に発動する」という処理をまとめるための関数であれば問題ないのですが、
そういう場合も

if( 条件 ){
    handleこういう時に発動する()
}

みたいな書き方をせずに、
ifこういう時に発動する()みたいな名前にして、条件式は関数の中にいれてくれたほうがスッキリするのになあって思うことが度々あります。

0

実際の事例なしに言うのは勘違いしているかもしれず申し訳ないのですが

ifこういう時に発動する()

だと、本来不要な関数呼び出しが行われ、その関数内にジャンプして条件式の正確な実装を読まなければ挙動がわからないので、可読性的に、よいパターンだとは思えないです。

名前は嘘つかれたり正確でない場合が往々にしてありますが、実装は嘘つかないので実装での条件式をその場で見れた方が楽だったりします。

あと、グローバル汚染を防ぐために単独名のオブジェクト配下に全て機能をつめるというのは望ましい動作だと思います。別名定義もできるようになっていて、オブジェクト名というか名前空間がぶつかることがなくなります。ほとんどの有名ライブラリがそのように作られているのではないでしょうか?

jQueryとか、lodashなどなど。

0

@standard-software さんが似た話をされていますが、Vue.jsのようなJavaScriptライブラリがWindowプロパティの汚染を避けるのは、他のライブラリとの名前の衝突を防ぐことが主目的だと思います。

ライブラリがWindowのプロパティを自由に定義してしまうと、偶然同じ名前のプロパティを定義した別のライブラリがあった場合に競合してしまい、同時に使うことができなくなってしまいます。

なので、アプリケーションのプログラムであれば極端に避けなくても良いと思いますが、ライブラリとして広く提供するプログラムではグローバル変数(Windowプロパティ)は避けた方が良いかと思います。

1

@standard-software @alt_yamamoto
ご意見ありがとうございます!

条件式の正確な実装がわからなくなる

自分がコーディングするとき、複雑な条件式は、できるだけ「つまりどういうことか」という名前の説明変数に代入していたので、そのノリで関数名にしてしまえば良いかなーと思ったのですが、周りからみると逆にわかりにくいのですね。
今後やってしまうことがあるかもしれないので、そのときに、メンバーの意見も交えてより具体的に受け止めようと思います!

オブジェクト名というか名前空間がぶつかる
ライブラリとして広く提供するプログラムではグローバル変数(Windowプロパティ)は避けた方が良い

Vuexで管理するのはアプリケーションのプログラムで利用する変数だと思うので、
名前空間がぶつかるかどうかはそのプロダクトのやり方次第で、
もともとVueがWindowプロパティを使えるようになってればVuex使う必要なくてよかったんじゃないかなーと思ったのですが、悪手でしたか…すみません。
おそらくまだ、お二人の危惧しているような問題に自分が直面したことがないから配慮がたりないのだと思いました。
記事を読むだけでなく実際に失敗経験を積めるようにもっといっぱい試行錯誤していきたいと思います。

1
(編集済み)

大変素晴らしい内容かと感じました。。
いろいろ参考にさせて頂きます。

ただ、『大小比較には<を使い>を使わない』このイメージが湧きません。
簡単にでも一例を示して頂けますと幸いです。

0
あなたもコメントしてみませんか :)
ユーザー登録
すでにアカウントを持っている方はログイン

0 コメント:

コメントを投稿