副作用満載のコードをリファクタリングした

はじめに

の参加記事になります。

少し昔の話ですが、Javaのシステムのコードを大きなリファクタリングをした経験があり、
その内容のうち3つのリファクタリングについて書いてみました。

既存コードの状況

自分は新しくチームに参画したばかりで、既存コードには詳しくありませんでした。

新規の機能追加のタスクを担当していましたが既存のコードを見た感覚として３つの問題があると思いました。

• １つ目はメソッドの呼び出しが副作用で溢れており、どこで何が行われているかが追いづらくなっていました

• ２つ目はDBのレコードデータを実行時に処理するときに様々なテーブルの情報を組み合わせながら処理していましたが全てのレコードidがintで扱われており、処理に必要なテーブルが5-10種類の様々なidの存在チェックの認知が難しくなっていました

• 3つ目はcookieのデータを読み取りと保存処理がありましたが、cookieから読み取りオブジェクト化するロジックとcookieに書き込む文字列を組み立てる処理が自前のifやfor文で行われていて処理の追加や修正で問題が発生しやすくなっていました

ゴール

それぞれの課題に対して、主に以下の対処法でコードを再設計し実装し直しました。

1. 不変の活用
2. 値の間違いを型で防止する
3. 修正漏れや重複コードの排除

実際に行ったこと 1.不変の活用

リファクタリング前

副作用で溢れておりresultDataの内容がどのように更新されているか分からなくなっていました。
メソッドA，B，Cの中では他のメソッドも呼ばれており、resultDataのsetterメソッドを使って様々な場所で更新されるため、このデータがどこで生成され、どこで更新されるかが追いにくくなっており、ソースコードを概要として掴んでも、詳細まで全てコードを読まなければどこでdtoの値が更新されているかわからない状態になっていました。

副作用にあふれるコードをimmutable(不変)なコードに書き換えることにしました。

下記はイメージのコードです。

class ServiceLogic {

    public Data execute(DTO dto) {

        Data resultData = new Data()

        //dataが何か更新される
        メソッドA(resultData, dto);
        //dataが何か更新される
        メソッドB(resultData, dto.param3);
        //dataが何か更新される
        メソッドC(resultData);

        return resultData;
    }
}

リファクタリング後

それぞれのメソッドは副作用をなくし、新しく得たデータについては全て戻り値で受け取るようにしました。
また、メソッド内部で利用する値として使わない情報も多く渡されているケースがあったため、必要な値のみを渡すことで、どの変数が必要であり、参照されているかを一目瞭然な形にしました。

戻り値となるオブジェクトはコンストラクタでのみオブジェクト内部のデータを更新するようにして、setterでの更新をなくしたため、生成時以外での変更がなくなり、副作用が減ったため、ソースコードを追いやすくなりました。

class ServiceLogic {

    public Data execute(DTO dto) {

        //dtoで必要な値のみを引数に与える。dataは更新されないようにする
        ResultA resultA = メソッドA(dto.param1, dto.param2);
        //メソッドAの結果が必要なものとdtoの値で必要なものを明確に渡す
        ResultB resultB = メソッドB(resultA.valueList, dto.param3);
        //メソッドAとメソッドBの結果の組み合わせで必要な引数を明確に渡す
        ResultC resultC = メソッドC(resultA.paramX, resultB.data);

        // 戻り値として必要なデータをコンストラクタで生成し、余計なDataのsetterはメソッドは削除
        Data resultData = new Data(resultA.paramX, resultC.map, resultC.list)

        return resultData;
    }
}

実際に行ったこと 2.値の間違いを型で防止する

リファクタリング前

テーブルのレコードはマスタのIDが存在するかしないかどうかの判定を行うことで、そのステータスを持つかどうかを確認し、カテゴリごとにさらに違うテーブルを参照することを行っていました。
初見で入った自分は似たテーブル名がたくさんあり、idも全てintで扱いながら、様々なメソッドを渡り歩いたり、intのidの比較を行うことがされていたりしましたが、似ているテーブル名なので、idが何なのかがすぐわからなくなっていました。

List<Integer> tableAIds;

for(Integer id : tableAIds) {
   List<Integer> tableBIds = tableAtoB（id）
   methodX(tableBIds)
}

// use other... tableBIds and tableAIds

リファクタリング後

idについて全てに型をつけることで、間違ったIDを間違ったメソッドの引数として渡すことが出来なくなりました。
毎回インスタンスの生成が必要なため処理のパフォーマンスは多少下がったのですが、リファクタリングを安心して進めることが出来るようになりました。

(ちなみにScalaであればパフォーマンスが下がることなく別の型をつけることが出来るので、少しおすすめです。最近Scalaは流行っていませんが・・・)

このIdオブジェクトも1の改修のときと同じようにimmutable(不変)です。

下記がイメージコードになります。
比較のときはequalメソッドでも出来ますが、別の自分の型と同じものしか受け付けないメソッドを用意して、int同士の比較であっても、意味が違うものの比較を出来なくしました

public class TableAId {
  private final int id;
  public TableAId(int id) {
    this.id = id;
  }

  public boolean equals(Object other) {
    if (other == this) return true;
    if (this.getClass().equals(other.getClass())){
      return false;
    }
	    
    return this.id == ((TableAId)other).id;
  }
}


public class TableAtoBId {
  private final int id;
  public TableAtoBId(int id) {
    this.id = id;
  }

  // eq略
}

public class TableBId {
  private final int id;
  public TableBId(int id) {
    this.id = id;
  }
  // eq略
}

public class TableAtoCId {
  private final int id;
  public TableAtoCId(int id) {
    this.id = id;
  }

  // eq略
}

実際に行ったこと 3.修正漏れや重複コードの排除

リファクタリング前

cookieからデータを読み込む処理と書き込む処理がそれぞれ分離しており、dataの状態に合わせてif文で構築していました。
読み込みと書き込みの整合性を人間の手で行う必要があり、書き込みは更新したが、読み込みを更新し忘れた場合にバグってしまう問題がありました。

class CookieUtil {

  public String makeCookieString(CookieData dto) {
    StringBuilder sb = new StringBuilder();
    if(dto.a != null) {
      sb.add(dto.a);
      sb.add(",");
    }

    if(dto.fList.size()>0) {
      sb.add(":f");
    }

    for(Obj f : dto.fList) {
      sb.add(f.a);
      sb.add(",");
    }

    if(dto.mapping.size()>0) {
      sb.add(":m");
    }

    for(Set<String,String> set : dto.mapping) {
      sb.add(set.key)
      sb.add("_")
      sb.add(set.value)
    }

    return sb.toString();
  }

  public CookieData makeCookieObject(String cookieString) {
     CookieData data = new CookieData();

     List<String> cookieValueStrings = cookieString.split(":");

     for(String value : cookieValueStrings) {
       String valueId = value.substring(0,1);
       if(valueId.equals("f")) {
           // list処理し、dataに値をセット
       } else if(valueId.equals("m")) {
           // map処理し、dataに値をセット
       } else {
           // 基本データ処理し、dataに値をセット
       }
     }

     return data;
  }
}

リファクタリング後

jacksonのObjectMapperをベースに、オブジェクトからアノテーションを参照しながらCookie用の文字列を生成する機能を作りました。

また、Cookieで取得した文字列からオブジェクトの生成も可能にしています。
ObjectMapparに追加で登録するSerializerとDeserializerの両方を実装しなければならないテンプレートとなる親クラスを作成しました。

これで、書き込みと読み込みがずれることはなくなりました。
基本的に１つのアノテーションと参照の型に対して、１つのObjectMapperがあるイメージになります。

Cookie特有の難しさとして工夫した点は、

1. 文字列が長すぎるとCookieに書き込めなくなってしまうため、変数名を直接出力せず、nameを指定してなるべく短い文字でオブジェクトの変数とのマッピングするキーを指定していること
2. 型によって可変になる文字列は区切りの良いところまででリミット数でカットし、文字として出力しないこと
3. Listについては先に優先順でソートしておいて、優先度が低いものはカットされても良いようにすること
4. 読み込み時に不正なクッキーデータが流れることがあるため、特定パラメータのみ個別で例外処理により、オブジェクト全体が死なないようにしたこと

などがあります。

下記はイメージのコードになります。実際は複雑なオブジェクトの階層構造になっていましたが、サンプルはシンプルな表現にしてあります。

// setterとgetterは省略して表現しています
@CookieData
class CookieDto {

  @CookieValue(name="i")
  public String dataId;

  @CookieValue(name="f",rule="abc", max="20")
  public List<String> categoryIds;

  @CookieValue(name="m")
  public Map<String,String> mappings;
}

まとめ

今回は

1. 不変の活用
2. 値の間違いを型で防止する
3. 修正漏れや重複コードの排除

の観点での今までのリファクタリング経験について書いてみました。

システム内部の設計の良さによってコーディングがしやすくなるため、これからもきれいなシステム、きれいなコードであることを目指して、良いコードを設計する力をつけることが出来ると良いかなと思います。

今回、こちらの記事でも自分が知識を得るために読んだ本の一部を紹介しているので、ご興味あったら見て頂ければと思います。