技術的負債としてのコードアンチパターン

はじめに

Hello World！akippaバックエンドエンジニアの杉山です。

3月に入り花粉症が猛威を振るっていますね。基本は自宅で勤務しているので平日はマシですが、休日に外出すると目は痒くなるわ、喉もイガイガするわで嫌な季節です…暖かくなってきて春を少しずつ感じられるのは嬉しいのですが。

さて、現在の私はYiiフレームワークで作成されているAPIに手を入れるタスクに取り組んでいます。
今やPHPのデファクトスタンダードといえばLaravelと言われる世界で、Yiiという聞き慣れない(自分が不勉強なだけ)フレームワークかつ、そこで動く技術的負債となっているコードを目の当たりにしました。さながらジャングルである 

今すぐこの負債を返却できるかというと、そうはできないのでしょうが可能な限りやれることをやってみようという意識で進めています。
今回の記事ではアンチパターンの事例とそれに対する対応方法を備忘の意味も込めて書かせて頂きます。

そもそもどういう状況なのか

akippaのバックエンドでのメインフレームワークはLaravelなのですが、実は他にもフレームワークが稼働しています。
以前リードエンジニアの村上がこの記事のはじめにちょろっと書いてくれていますね。

古いフレームワークもまだ現役でバリバリ残ってます

akippaで採用しているLaravelのデザインパターンのお話：前編

この古いフレームワークに該当するのがYiiというやつです。 実はZend Frameworkってやつもいるよ！  

akippaのアプリではこのYiiで作成されたAPIがいくつか稼働しています。
今回の自分のタスクでは予約時の動作に手をいれることになりまして、その部分でのコードアンチパターンが散見されました。
Git Blameでコードがいつ作成されたかを確認すると、7〜8年前のものもあったり…

それでは次の項目で具体的にどのようなものがあったかをお見せできればと思います。

アンチパターン

ネストが深い

try {
    // 予約情報を更新する
    if ($hoge->hogeStatus == Hoge::STATUS_NO_STARTED) {
    // 開始時間変更可能
    if(!hoge->save()) {
        throw new HogeException();
    }
    // シェアゲート
    if ($hoge->isShareGate()) {
    // 予約から外れた日程処理
    foreach ($hoge->shareGatePasswords() as $shareGatePassword) {
        if (!in_array($shareGatePassword->date, $newDates)) {
            $date = $shareGatePassword->date
        }
    if ($deleteDate) {
        // 何らかの処理
    }
    foreach ($fugas as $fuga) {
        if(empty($fuga)) {
            if() {
                // 何らかの処理
            elseif ($fuga === 'fuga') {
                // 何らかの処理
            }
            else {
                // 何らかの処理
            }
        if (!$password) {
            // 何らかの処理
        }
    unset($hoge->hogeMethod());
    else {
        // 何らかの処理
    }
    // 以下続く

とある処理の部分を頑張って途中までいくつかの変数名等をぼかしつつ書いてみましたが…最後で力尽きました…

とはいえ、大体の流れはこんな感じです。
こう見てみると、とにかくネストが深く処理を追うことが大変難しいです。
( ネストというのは入れ子構造のことです。マトリョーシカ人形みたいなイメージ。)

簡単に上から追うと、try catch文の中でif文がいくつかあり、さらにその中でforeachでループ処理が入り、更にその中でif文があり、それが終わったらまたforeach文が入りその中でif elseif elseが入り、最後にunset()して、もし最初のifで条件に該当しなければelseに入り…あ！途中で例外があったらここには書いてませんが、catchの中に入りますね…
うーん、簡単か？

実際に文に起こしても入り組んでいることがおわかり頂けるかと。
ネストはあまり深くしないほうが良いですね。
可読性も下がり処理内容の確認もかなり大変になってしまいます。

メソッド分離がなされていない

上で書いたネストが深いとも関連しますが、各種処理がメソッドに切り分けられていないので、これもまた処理が追いにくくなっている要因です。

上記のコード例で考えますと予約更新処理とシェアゲートの処理がベタ書きされているようですね。
これらの処理はお互いに違う処理なのでメソッドを切り分けて、そのメソッドを呼出してあげる方が後々楽になります。

処理を分離してあげれば後々に変更が入った際もそのメソッドの内容を変えるだけで済みますし、場合によってはネスト自体を深くせず実装することができるかもしれません。

検索と設定が同時に行われている

if (!$parkingDetail->setParkingId($reservation->parking->parking_id)->findParking()) {
    $this->addError('result', '指定の駐車場が見つかりません');
    return false;
}

上記のコード自体から読み取れるのは、どうやら駐車場詳細から駐車場を検索しているのか？といったところですね。
その駐車場が存在しなければ結果とエラー文を返却すると。ここは問題ないかなと思いますね。
そこからしばらく進むと下記のコードが現れます。

$parkingDetail->startDate

初見の時の私は「$parkingDetailのプロパティにアクセスしてるけど…このstartDateはどこで設定されているんだ？」となりました。

ちなみにこのプロパティは先程のfindParking()内で設定されています。
まさかここで設定されているとは自分も思っていなかったのですが、丁寧にコードを追っていくと判明します。
つまり、「検索」と「設定」という別々の処理が一つのメソッドに閉じ込められている状態です。

if文内で判定を実行しているのでメソッド名からは駐車場を検索するだけと判断してしまうと、見落としてしまいます。
そのため、判定と設定を分けておいた方が安心かと。
それが無理そうならコメントを残すなどしておいた方が良さそうですね！

おわりに

内容としてはありふれたものでしたが、アンチパターンと対応方法についてここまで書かせて頂きました。
技術的負債は基本的にどうしても発生しうるものだと思います。
その当時には一番良いと思って実装しても、あとから見返したらちょっと良くないというのに気付いたり…

また技術的負債は実装者やフレームワークそのものに責任があると考えないほうが良いかなと個人的には思います。
もちろん実装時には責任感を持って開発しますし、コードレビューを行ってより良い実装にしようとしますが、どうしても完璧にするのは難しいと思います。
チームや組織として「どこかのタイミングで技術的負債を返却していこう！」という認識を持っておくことが対策になるかと。
(そもそもソフトウェアの世界はどんどん進んでいくので、勝手に負債化する可能性もあります)

それでも、なるべく技術的負債を減らしておこうという意識は開発者として持っておくのは必須のように思います。
これからも引き続き技術的負債を返却しつつ、akippaをより良いプロダクトにできるように頑張って参ります！

おまけ

上記のアンチパターンについて学習するにあたって、リーダブルコードという書籍が大変参考になりました。
エンジニアの間ではバイブルとして大変有名ですが、改めて読んでもまだまだ発見があります。(さすがは名著と言われるだけありますね！)
未読の方はぜひ読んでみて下さい！