コード保守する時間があったので、引き継ぎ等も見据えつつ自分のコーディング設計の確認も兼ねて、自分がVue.js始めたてに書いた箇所をリファクタリングしてみた。
今回は状態管理ライブラリのVuexに焦点を当ててみる。

まずは既存コード（を掲載できる用にアレンジしたコード）

パッと思いついた「おもちゃ」を題材にして不完全に再現しました。
お部屋がいくつかあり、その部屋にはおもちゃ箱がいくつかあり、おもちゃ箱にはおもちゃがいくつかある状態を管理するとします。
おもちゃは種類によってIDが振られているので複数個存在する（ピカ○ュウは何匹存在しようがNoは025であるのと同じ）。1つのおもちゃ箱の中身はユニークであるとします。

内容としては、ある配列に対して、条件が揃えばレコードを追加したり削除したりするようなミューテーションやアクションがあると思えば大丈夫です。

mutations.js


  addUniqueToy (state, {toyBoxId, roomId, toyId}) {
    const toyBox = state.toyboxes[toyBoxId];
    const index = toyBox.toys.findIndex(toy => (
      toy.toyId === toyId && toy.roomId === roomId
    ));
    if (index === -1) {
      toyBox.toys.push({roomId, toyId});
    }
  },
  removeToyById (state, { toyBoxId, toyId }) {
    state.toyboxes[toyBoxId].toys.some(function (toy, key) {
      if (toy.id === toyId) {
        state.toyboxes[toyBoxId].toys.splice(key, 1);
      }
    });
  }

actions.js

  putToyInToyBox ({ commit, getters }, { toyBoxId, roomId, toyId }) {
    commit('addUniqueToy', {toyBoxId, roomId, toyId});
  },
  takeOutToyInToyBox ({ commit }, { toyBoxId, toyId }) {
    commit('removeToyById', {toyBoxId, toyId});
  }

Vuexの設計

問題点を挙げる前に、Vuexのアクション、ミューテーション、ゲッターについて確認します。

アクション(actions)

ドキュメント記載の通り。

ミューテーションをコミットする
任意の非同期処理を含むことができる

条件が合わなかったり、もしくは例外処理なんかで必ずコミットしなければならない訳ではないかなと思います。
まあその時はエラーメッセージ等をコミットするかもしれませんが。

ミューテーション(mutations)

Vuexストアの状態を変更できる唯一の方法

ということなので、値を与えて更新するだけの処理だけを書いた方がいいと思います。

ゲッター(getters)

ストアの状態を算出する

ドキュメントでは、ステートの配列をIDでフィルタしたり長さを取得する際の例を挙げています。
ステートを使って何かしら計算したい処理を書く場合はこちらに書くといいと思います。

Javaのprivateなメンバへのアクセサのように、ステートだけを返却するようなコードを時々見かけます。

getters.js

  getToyBoxId: state => {
    return state.toyBoxId;
  },

これはやめましょう...ステートの数だけゲッターを定義することになるのでパンクします。

既存コードの問題点

以上を踏まえるとの問題点がわかってくると思います

`mutations.js`

ミューテーションがストアを更新しないことがある
- AからAに遷移するのはわかるとして、Aからどこにも遷移しないのはどうなのか
ステートの更新をするまでの前処理をしているので、他のアクションでコミットする際の再利用性が低い
removeToyByIdという命名がまずあり得ない
- 内容としてはIDが見つかれば追加したり、IDを見つけて削除しているが、この辺のIDでフィルタ処理はゲッターがすべき処理
- ついでにaddUniqueToyも同様

`actions.js`

アクションがアクションしていない
- 特に何事もなくコミットするだけの処理なら、アクション噛まさずに直でコミットした方がいいと思います

他には、チーム保守的な面として、探索系関数が統一されていないのも見受けられたかなと。

という訳で書き直した

mutations.js


  addToy (state, {toyBoxId, roomId, toyId}) {
    state.toyBoxes[toyBoxId].toys.push({roomId, toyId});
  },
  removeToy (state, { toyBoxId, index }) {
    state.toyboxes[toyBoxId].toys.splice(index, 1);
  });

actions.js

  putToyInToyBox ({ commit, getters }, { toyBoxId, roomId, toyId }) {
    if (!getters.toyExists(toyBoxId, roomId, toyId)) {
      commit('addToy', {toyBoxId, roomId, toyId});
    }
  },
  takeOutToyInToyBox ({ commit, getters }, { toyBoxId, toyId }) {
    const index = getters.getToysIndexById(toyBoxId, toyId);
    if (index !== -1) {
      commit('removeToy', {toyBoxId, index});
    }
  }

getters.js

  toyExists: state => (toyBoxId, roomId, toyId) => {
    const index = state.toyBoxes[boxId].toys.findIndex(toy => (
      toy.toyId === toyId && toy.roomId === roomId
    ));
    return index !== -1
  },
  getToysIndexById: state => (toyBoxId, toyId) => {
    return state.toyBoxes[boxId].toys.findIndex(toy => (
      toy.id === toyId
    ));
  }

IDでフィルタ系はゲッターに委譲した。おもちゃ箱の中のおもちゃを探したい時なんかに再利用できそう。

コミットするかどうかの分岐等の前処理はアクションに委譲した（分岐が外れた時のアラート等は省略しています）。どういう時におもちゃ箱に置けるか/取り出せるか一貫性が持たせれたような気がします。

ミューテーションはステートの更新だけに集中してもらった。また何かしらの機能追加でおもちゃ箱の中身を変化させる際も特にここを弄る必要はなくなりそうです。

いい感じにすんなりまとまったような気がします。

おわり

Vue.js手探りの時に書いたコードをリファクタリングしてみたら、当時は中々...だなぁということがわかるようにはなった程度には成長したような気がします。
触りたてほやほやの頃のコードを眺めるとなにか発見があるかもしれません。チーム開発となると誰かが消したり改修しているかもしれませんが...。

Vuexはミューテーションくんには優しくしましょう。ステートを更新するための条件ではなく、ミューテーションをコミットするための条件で記述するといいと思います。
その判断にストアの状態を算出する場合は、ゲッターくんに任せましょう。