mutableな配列を返すその前に

17078 ワード
テキストリンク

どうも。株式会社プラハCEO兼エンジニアの です。

先日プラハチャレンジの一環で「DDDに基づいてコードを書いてみよう！」という課題のコードレビューをしていた際、こんなコードを見かけました：

class User {
  constructor(private readonly _articleIds: string[], private readonly _name: string) {
    // 何らかの不変条件
  }

  public get name() {
    return this._name
  }
  public get articleIds() {
    return this._articleIds
  }
}

Userインスタンスのpropertyに対してsetterを定義しなければドメインエンティティの不変条件が守られる！というのが狙いだったのですが、配列の扱いに起因する注意ポイントがあったので記事にしてみようと思いました

TL;DR

  • コピーを返すかimmutableにしておこう

mutableな配列を気軽に返すと何が起きるのか

今回のコードは「nameもarticleIdsも外からは変えられない」という前提に立っていました。確かにnameは変更不可なのですが:

const user1 = new User([], 'hoge')
user1.name = 'fuga' // これはread-only propertyなので不可

配列はsetterを定義していなくても中身を変更できます:

const user1 = new User([], 'hoge')
user1.articleIds.push('fuga', 'piyo') // これは可

一時的に変数で保持しておいた配列を別のコードで誤って変更してしまう可能性も：

const user1 = new User([], 'hoge')
const articleIds = user1.articleIds

// ...たくさんのコードが書かれている

articleIds.push('fuga') // 意図せずuser1.articleIdsも変わってしまった！

可変配列を返すとuserのpropertyに対して直接的な配列操作（popとかspliceとか）を許容してしまうので、「特定の条件を満たした時のみ配列操作を許可する」といったドメインロジックを強制できず、Userの不変条件が満たされていること（例えばarticleIdsが常に1件以上5件未満である、など）が保証できない状態に陥りがちです

// 呼び出し側からやりたい放題にされてしまい、全くロジックの整合性が取れなくなったuser1
user1.articleIds.push('1', '2' ,'3', '4', '5') // 本当はarticleIdsに5件以上登録させたくなくても、登録されてしまう
user1.articleIds.splice(1) // 勝手に中身を消されてしまう
user1.articleIds.pop() // 本当はarticleIdsに1件以上は登録しておかなければいけないのに、最後の1件も消されてしまった...

対策: 配列のコピーを返す

userの外から直接配列を操作されることをどのように防げば良いのか、見ていきましょう。

まず配列そのものではなく値をコピーした新しい配列を返せば、直接user内の配列を操作される事がなくなるので安心です：

class User {
  constructor(private readonly _articleIds: string[], private readonly _name: string) {
    // 何らかの不変条件
  }

  public get name() {
    return this._name
  }
  public get articleIds() {
    return [...this._articleIds] // 新しい配列を作成して返す。this._articleIds.slice(0)でも可
  }
}
const user1 = new User([], 'hoge')
user1.articleIds.push('hoge') // 新しい配列にpushしているだけなのでuser1の_articleIdsは外から変えられない

呼び出し側が求めているのがarticleIdsの中身だけであれば（更新することでなければ）コピーを返すだけで事足りるはずですよね。更新に関するロジックはuserクラスの中にだけ定義すれば、配列が変更される前にバリデーションが行われる事を保証しやすくなります：

class User {
  .
  .
  .
　　　　public removeLastArticleId() {
      if (this._articleIds.length > 1) { // 更新する前に必ず残数を確認する
        this._articleIds.pop()
      }
  }
}

データと、データを変更する処理を意味的に近いところに定義できたらコードの可読性が高まる 、あるいは配列そのものを公開して呼び出し側に自由を与え過ぎるより限定的なインターフェースを公開した方が管理しすい という考え方がシックリくる方に関しては、パフォーマンスとの兼ね合いにはなりますが上記のような書き方を意識してみても良いのではないでしょうか。

対策: readonlyな配列にする

一度インスタンス化した後に配列の中身をそもそも変更する必要がなければ、articleIdsの型をstring[]ではなくreadonly string[]にしておけば破壊的な操作を防げます:

class User {
  // _articleIdsの型がstring[]からreadonly string[]に変わっている
  constructor(private readonly _articleIds: readonly string[], private readonly _name: string) {
    // 何らかの不変条件
  }

  public get name() {
    return this._name
  }
  public get articleIds() {
    return this._articleIds
  }
}
const u = new User([], 'hoge')
u.articleIds.push('fuga', 'piyo') // read-onlyなので不可

昔はReadonlyArray<string>みたいな書き方が必要でしたが、最近はreadonlyを付け加えるだけなので書き換えも楽ちんですね。

ちなみに配列に変更を加え得る関数にreadonlyな配列を渡すことはできません:

const hoge: readonly string[] = ['a']
const dosomething = (_: string[]) => {} // string[]型なので、引数の配列に変更を加えうる事が型から推察される
dosomething(hoge) // なので、readonlyな配列は渡せない

引数をreadonlyな配列として定義しておけばpopなどの破壊的なメソッドがそもそも呼び出せないので、より安心して配列を使う事ができます:

const dosomething = (a: readonly string[]) => { // readonly string[]にしてみたら
  a.pop() // popが定義されていないので呼び出せない
}

mutableな配列とimmutableな配列がそもそも別の型として定義されている言語なら意識しやすいのですが、TSはreadonlyを付与しなければデフォルトでmutableになってしまうので無防備な配列が使われているのを見かける機会が言語的にちょっと多めかもしれません。「ここreadonly付けられないかな...?」と常に意識しておくと後々になって楽ができるのでオススメです

まとめ

インスタンスのプロパティをそのまま返す前に「コピーで済むのではないか?」「immutable（不変）に出来ないか?」など考えてみると良い事があるかもしれない

VSCodeでもmagrittrパイプをいい感じに入力するためにVSCode拡張機能の作成にチャレンジしてみる
Node18系の変更点について