🗂

読みやすいコードを書く_メソッドの副作用を無くせないか考える

2021/05/15に公開

Java

設計

tech

あらまし

レビューしたコードに以下のようなものがありました。
（クラス名等は公開できるように改変）
その中の setTestSettingData というメソッドについての話です。

@Test
public void test1(){
  SettingData testSettingData = new SettingData();
  setTestSettingData(testSettingData,SettingMode.INSERT);
  ・・・略・・・
}

@Test
public void test2(){
  SettingData testSettingData = new SettingData();
  setTestSettingData(testSettingData,SettingMode.UPDATE);
  ・・・略・・・
}

private void setTestSettingData(SettingData testSettingData,SettingMode mode){
  testSettingData.setMode(mode);
  testSettingData.setSettingCode("testCode");
  testSettingData.setSettingName("testName");
  testSettingData.setSettingComment("testComment");
}

setTestSettingData はテスト用の SettingData に値をセットする処理を共通化したメソッドのようでした。
テストコードのprivateメソッドの話なのでそんなに重要な話では無かったのですが、

レビュイーの方がプログラミング初学者の方で丁寧にフィードバックしたい
なぜ修正するべきなのか言語化しようとすると意外と難しい
真面目にフィードバックするとMergeRequestのコメントに記載するには長い
読みやすいコードを書く際の重要な観点が含まれているかもしれない
同じような話を他の方にする際にも使えるかもしれない

と思いフィードバックしたい内容を記事にしてみました。

修正例

感覚的にsetメソッドではなく以下のようにmakeメソッドにするべきだなと感じました。

@Test
public void test1(){
  SettingData testSettingData = makeTestSettingData(SettingMode.INSERT);
  ・・・略・・・
}

@Test
public void test2(){
  SettingData testSettingData = makeTestSettingData(SettingMode.UPDATE);
  ・・・略・・・
}

private SettingData makeTestSettingData(SettingMode mode){
  SettingData testSettingData = new SettingData();
  testSettingData.setMode(mode);
  testSettingData.setSettingCode("testCode");
  testSettingData.setSettingName("testName");
  testSettingData.setSettingComment("testComment");
  return testSettingData;
}

何故そうした方が良いと感じたのかを言語化してみます。

なぜmakeメソッドにするべきなのか

端的に言うと本質的にやっていることが SettingData の生成だから
なのですがsetメソッドよりもmakeメソッドにした方が良い具体的な理由について列挙してみます。

インスタンスを生成するコードも共通化できる
引数を一つ減らせる
makeメソッドにすると副作用を無くせる

1または2のフィードバックでも修正例のように修正することに納得してくれると思います。
しかし今回一番強調したいのは3の重要性についてです。
副作用のあるコードを見た時に副作用を無くす書き方ができないか考える、という観点を持ってほしいです。

副作用とは

今回の例では 参照渡しされた引数に対するいわゆる“破壊的操作” にあたります。
一般的に不必要な副作用を避ける、副作用のあるコードを集約する、ことが読みやすいコードを書く上で重要であると言われています。

なぜ不必要な副作用を避けるべきなのか

一般論

副作用があるメソッドは外部の状態を変更します。
外部の状態を変更するメソッドは利用する側が、どのように状態が変更されるのかを把握しなければ適切に扱えません。
メソッドを利用する側が把握しなければいけないことが増えると扱いづらいメソッドになってしまいます。

今回のコードではどういう問題が起きるのか

例えば以下のように setTestSettingData を呼び出す前にsettingCodeをセットするようなロジックを書いた場合、無意味なコードになります。

@Test
public void test1(){
  SettingData testSettingData = new SettingData();
  testSettingData.setSettingCode("testSettingCodeX");
  setTestSettingData(testSettingData,SettingMode.UPDATE);
  ・・・略・・・
}

setTestSettingData は引数のSettingDataに副作用を及ぼすメソッドであり、暗黙的に空のSettingDataを引数に求めています。
これは setTestSettingData を利用するメソッドが空のSettingDataを渡すという使い方を把握していなければ適切に利用できないということです。

事前のインスタンスの生成箇所も含めて共通化した makeTestSettingData であれば空ではないSettingDataを渡すという想定外の利用はできなくなります。

まとめ

今回例で挙げた setTestSettingData は小さなメソッドなので内容を読んで利用すれば問題ありませんが、こういったメソッドが積み重なるとアプリケーション全体のロジックがじわじわと読みづらくなっていきます。

どのようにメソッドを設計するのかを考える際に「不要な副作用が無いか？」という観点を持って考えてみてください。