QUOカードPay オンラインストアをリライトしました

こんにちは、日本のテトリスプレイヤーの上位 0.5% に入るデジタルイノベーションラボの飯島です。 今回は Java のプログラムをリライトした話を書きます。

背景

2020年6月にQUOカードPayオンラインストアの内製化を行いました。 その時は EC2 で動いていた Java プログラムを ECS に乗せるために必要な修正を加えましたが、ソースコードの大半には手を加えていませんでした。 内製化から1年と少し経ちますが、バグの修正や新規機能追加の際に複雑なソースコードの解読に時間を取られることが多く、これはまずいということで2ヶ月間他の機能追加無しでリライトをすることにしました。

方針

プロダクトの全体的な設計から見直すのが理想でしたが、そこまでやると果てしない時間がかかってしまうため、少ない時間で効果が得られそうな箇所や、今後よく触りそうな箇所を重点的にリライトするようにしました。

実施した内容

Kotlin 化

QUOカードではサーバーサイド実装の言語としてKotlinを採用しています。 今回リライト対象となるコードはJavaで書かれていたため、まずKotlin化をすることにしました。 Kotlin化により nullとmutableの扱いが限定されるので、可読性の向上とバグの減少が見込まれます。

null まわりの修正

元々のソースコードでは @Nullable や @NotNull が使われていなかったため、引数に null が入ってくるのか、戻り値として null が考えられるのかがぱっと見でわからないようになっていました。 そのため不要な null チェックが蔓延していたり、また必要な箇所に null チェックが足りなかったりという状態でした。

Kotlin では基本的に null は許容されず、型の後に ? をつけることで nullable になります。

val a: String? = null
val b: String = null // コンパイルエラー

Java から Kotlin に変換する際には型に ? をつけておき、可能なら ? を削除するという方針でリライトしました。 ただ、この修正は当初考えていたよりもずっと難しく、まだかなりの量の ? が残ってしまっています。 理由としては、元々のソースコードを読んでも使われている値が null を許容するのかしないのかの判断が難しかった為です。 例えばメソッドの引数で言うと、そのメソッドを呼んでいるメソッド、そのメソッドの引数がそのまま使われているなら更にその呼び出し元、というように追いかけて虱潰しに調べないといけません。 クラスのフィールドで言えば、それが使われている全ての箇所を調べないと判断ができません。

変数の不変化

Kotlin では、val で宣言した変数は不変 (immutable) になり、後から変数に値を代入しようとすることができません。これにより、一度初期化した変数がソースコード中のどこかで変更されないことがコンパイラレベルで保証されるので、変数に何が入っているのかを把握するためにソースコードを深く潜っていかなくてもよくなります。（詳しくは「副作用」の箇所に書きます） リライトにあたり、ローカル変数でもクラスのフィールドでもとにかく可能な限り val で宣言することを意識しました。

綴りミス（タイポ）を修正

当たり前ですが綴りミスは無い方が良いです。 コードリーディングの妨げになりますし、正しい綴りで検索をかけた時に引っかからなくなります。 例えば、Downloadで検索したのに、Donloadと綴りミスしていたら一生引っかかりません。 （DonloadはもしかしたらDonald なのでは？と社内で議論になりました） 今回は一つのクラス内に2, 3個綴りミスがあるというレベルだったので、他の項目のリライト中に直せる箇所だけ直す、という対応をとりました。

修正時の注意点

thymeleafなど実行時に名前を解決する処理がある場合、Java側の綴りミスを直してコンパイルが通っても実行時にエラーが発生します。 この部分のリライトは影響範囲の調査に時間がかかるため、今回は後回しにしています。

String の配列をクラスに変更

例えば名前と年齢を持つ犬を表現したい時、一般的にはクラスを作ると思います。

data class Dog(val name: String, val age: Int)

これをStringの配列で表している箇所がありました。

// 0番目の要素に名前、1番目の要素に年齢を入れる
String[] dog;

年齢を取り出す場合は Integer.parseInt(dog[1]) となっています。 さらに、犬をたくさん扱う場合はこうなっていました。

String[][] dogs;

実際にはdogsのような体を表す変数名はつけられておらず、result のような名前だったため、変数宣言から何を意味するのか読み取れず苦戦しました。 こちらは Dog クラスを用意することで対応しました。

フラグ保持用の Map<String, String> を List に修正

飼い主に対して複数の猫が紐づくモデルを考えます。 単純に DB から猫を取ってくると、このようなクラスにマッピングできます。

class Cat {
    private Person person; // 飼い主
    private String name;
}

DB から取ってきた List に対してループを回す中で、既に登場した飼い主を保持しておく Map を作っている箇所がありました。

Map<Person, String> map;
for (Cat cat: cats) {
    if (!map.get(cat.person).equals(“”)) {
        // キーが存在しない場合の処理
    } else {
        // キーが存在する場合の処理
        map.put(cat.person, “”);
    }
}

最初はこの map が何をしているのかを理解するのに時間がかかりましたが、単純に処理済みの飼い主を保持しているだけということがわかりました。

val processedPersons: List<Person>;

としてリストに持たせて変数名を分かりやすくすることで解決しました。

private メソッドを別クラスの public メソッドにする

他のクラスが持つべきメソッドが private メソッドとしてコントローラーやサービスに実装されていたので、そういうものはあるべき場所に実装を移動しました。

private String getDogInfo(Dog dog) {
    return dog.getName() + “ ” + dog.getAge().toString();
}

⬇️

data class Dog (val name: String, val age: Int) {
    val info: String
        get() = “$name $age”
}

String クラスや各種ライブラリが提供するクラスなど、自分で修正することができない型に対するメソッドに関しては、拡張関数を作成して対応しました。 型をラップしたクラスを作って必要なメソッドのみを生やすという方法も考えられますが、今回は手間も考慮して拡張関数方式にしました。 拡張関数を使うと、メソッドのネストをチェーンで記述できるようになるので、単純な可読性も向上しました。

private String addExclamationMark(String s) {
    return s + “!”;
}

public void a() {
    addExclamationMark(
        addExclamationMark(
            addExclamationMark(“hello”))) 
}

⬇︎

fun String.addExclamationMark(): String = “$this!”

fun a() {
    “hello”
        .addExclamationMark()
        .addExclamationMark()
        .addExclamationMark()
}

メソッドから副作用を取り除く

副作用のあるメソッドはコードリーディングの妨げになります。

public class Fox {
    private String name;
    private int age;

    // setter & getter
}

public void a() {
    Fox fox = new Fox();

    setName(fox, “taro”);
    fox.setAge(3);

    System.out.println(fox);
}

private void setName(Fox fox, String name) {
    fox.setName(name + “-chan”);
}

このコードでは、最初に宣言された時の fox と println した時の fox では状態が異なります。println 時点での fox の状態を知るには、fox が宣言された場所から println される場所までの全ての行を読まないといけません。もし fox が宣言された時点から変更されないことが保障されるなら、println がどれだけ離れていても fox の状態はすぐ把握できます（そもそも変数の宣言と使用箇所が離れていること自体がよくないですが）。

data class Fox (val name: String, val age: String)

fun a() {
    val fox = Fox(getName(“taro”), 3)
    println(fox)
}

private fun getName(name: String): String = “$name-chan”

Kotlin だとこのように書けば、宣言時から変更されることはありません。

副作用を持つメソッドの見分け方として、まず戻り値のない(voidの)メソッドは怪しいです。setXXX という名前のメソッドもほぼ副作用があります（中には BigDecimal のように新しいオブジェクトを返すものもありますが）。あとはリライト中のメソッドから呼び出しているメソッドの中を逐一確認していきます。副作用を持つメソッドを呼び出しているメソッドも副作用があることになるので、呼び出し階層を全て辿る必要があります。

修正方法ですが、副作用としてセットする値を返すようにします。上の例でいうと、setName の中で fox に値をセットしていたところを、getName として値を返し、呼び出し元の方で使用するようにしています。 何重にもネストしたプライベートメソッドの中に副作用がある場合は、こうやって少しずつ副作用を上のメソッドに押し出していき、最後に呼び出し元で使用するようにします。 副作用の無いメソッドが完成したら、副作用のある元のメソッドは Deprecated でマークし、「副作用があるため、極力 XXX を使用すること」とコメントしておきます。 こうすると Intellij などの IDE では呼び出し箇所に取り消し線が引かれるため、今後リライトするときの目印となります。

終わりに

リライトをすればするほど「これは一から書き直した方が早いのでは。。」という気持ちに見舞われましたが、実際はソースコードの奥深くに隠された仕様を発見するための時間が必要になるので、やっぱり時間が取れたタイミングでリライトしていくのが現実的かな、とも思いました。

また、技術的負債は放っておくと返済にかかるコストが上がっていくと言うのを身を以て体験しました。クリーンアーキテクチャの最初の方に、「時間がある時に綺麗にすればいいと思うかもしれないが、そんな時間は一生来ない」のようなことが書いてありましたが、自分も実装するときはそれを意識しようと思いました。今回は何をするにもソースコードの解読に多大なコストがかかってしまう状態になっていたのでリライトの時間を設けざるを得ませんでしたが、最初から綺麗なコードだったらもっと生産的なことに時間を割けたのに、、と考えると、常にその時点で最も読みやすいコードを書く努力は大事だと思います。（ただ、リライトしてコードを綺麗にしていく作業自体は楽しかったです。）