レガシーコードをぶっ壊す！〜基本のキ編〜

この投稿はアイスタイル Advent Calender 2019 の7日目の記事です。

はじめに

皆さんこんにちは！
今年で新卒2年目となったnakazawayです！
今年も今年で、古くからある弊社サービスの会員基盤を担っており、レガシーな技術＆実装と日々奮闘しております！！

今年はそんなレガシーなコードに対して少しだけ規模の大きい改修を任せてもらいました！！
今回は、そんな激闘の中自分が学んだ「基本の基本」を皆さんにまとめてご紹介しようと思います！！

少しだけ規模の大きい改修（具体的に何したの？）

今まで@cosmeではメールアドレスでの会員登録しかできませんでしたが、2019/12/14より電話番号での会員登録ができるようになりました！??? ※1

その導入を任せていただいたのですが、レガシーコードへの改修は骨が折れました。。。
拡張性の無い実装、謎めいた実装などなど、どこに何が隠れているのかわからない状態での導入だったので、いつどこで爆弾が爆発してもおかしくなかったのです。。。

※1 電話番号での会員登録は2019/12/7現在@cosmeアプリからのみできます。

レガシーコードとは

先ほどからレガシーコードと散々連呼していますが、本記事が指すレガシーコードの定義を示します。

• COBOLで書いたコード？
  ?違います
• Ruby1.8系で書いたコード？
  ?違います

本記事でのレガシーコードとは、
理解しにくく、拡張性の無いコード
のことです。

レガシーと言えど、これから自分自身が作り出してしまうものかもしれないのです。
ということで、先人たちが犯した過ちを再度犯さないように、アンチパターンとその改善案をまとめていきます。

目次

1. 不要なelseやswitch文は積極的に潰す
2. マジックナンバーは消し去る
3. 言語、フレームワークの規約に準拠しよう

1.不要なelseやswitch文は積極的に潰す

どんなソースコードにも必ずと言っていいほど分岐処理は存在すると思います。
簡単に分岐処理を実装するには、当然 if文 を使用します。
その際、分岐処理が多いと、 else や swicth文 を使いたくなると思います。しかし、多用は厳禁です！
場合によってはどうしても使用しなくてはならない時がありますが、大体の場合はそれらを使わずに分岐処理が可能です。

なぜelseやswitch文の多用はダメか

単純に複雑な実装になり、他人が見てとても理解しにくいソースコードになるからです。拡張性にも乏しいです。
理由を以下に列挙します。

• 〜だった場合ではなく〜だった場合ではなく…を考えていると頭が爆発する
• ネストが多いと単純に見にくい（醜い）
• テストコードが書きにくくなる
• 改修を加える時にどの部分に手を加えればいいのかが全くわからなくなる

どうすれば良いか（改善案）

改善案はいくつかあるのですが、まずは一番手っ取り早い早期リターンの紹介です。

早期リターン

例えば以下のような、単純にbooleanを返すような関数があるとします。（ソースコードはPHPです）

public function existsUser(userId){
    if (this->getUser(userId)) {this->User->setUserId(); //セッションにuser_idをセット
     } else {
        return false;
    }
    return true;
}

指定したユーザーIDのユーザーが存在するかどうか返すものですが、早期リターンするとこうなります。

public function existsUser(userId){
    if (!this->getUser(userId)) {
        return false;
     }this->User->setUserId(); //セッションにuser_idをセット
    return true
}

こんな感じです。上記例は簡単なものですが、弊社のソースコードにもこんなのがゴロゴロいました。

デザインパターンを駆使する

早期リターンを使えない場合も出てきます。例えば、以下の例。

public function registerUserAction() {
    identifierIdType =this->request->params['identifier_id_type'];

    switch ($identifierIdType) {
        case: 'email'
            // emailによるユーザー作成
            break;
        case: 'phone'
            // phoneによるユーザー作成
            break;
        default:
            // emailとphone以外はexceptionを返す
    }
    switch () {} // type別による通知送信処理(略)
    switch () {} // type別によるログイン処理(略)
    return;
}

この場合は、どうしてもswich文かelseを使いたくなりますよね。。。
そんな時は、デザインパターンを駆使します。
この場合、StrategyパターンやStateパターンを使います。
今回はStrategyパターンのご紹介。

// interfaceを定義
interface RegisterInterface {
    public function createUser();
    public function sendNotification();
    public function login();
}

// コンテキストクラスの作成
class Register {
    private register;

    public function __construct(RegisterInterfaceregister)
    {
        this->register =register;
    }

    public function execute()
    {
        this->register->createUser();this->register->sendNotification();
        $this->register->login();
        return;
    }
}

// RegisterInterfaceを実装したそれぞれの具象クラスを作成
class emailUserRegister implements RegisterInterface {
    public function createUser () {
        // ユーザー作成処理
    }
    public function sendNotification () {
        //通知送信処理
    }
    public function login () {
        //ログイン処理
    }
}

class phoneUserRegister implements RegisterInterface {
    public function createUser () {
        // ユーザー作成処理
    }
    public function sendNotification () {
        //通知送信処理
    }
    public function login () {
        //ログイン処理
    }
}

//会員識別IDの種別によって具象クラスのインスタンスを返す
public function RegisterCreator (identifierIdType) {
    switch (identifierIdType) {
        case 'email':
            return new emailUserRegister;
        case 'phone':
            return new phoneUserRegister;
        default:
            // emailとphone以外はexceptionを返す
    }
}

interface , コンテキストクラス , 具象クラス , 種別によってインスタンスを変えるメソッド の4つを用意することで、最初のswitch地獄は解消されます。
以下が綺麗になったregisterUserActionメソッドです。

public function registerUserAction() {
    identifierIdType =this->request->params['identifier_id_type'];

    register = RegisterCreator(identifierIdType);
    registerContext = new Register(register);
    $registerContext->excute();

    return;
}

2.マジックナンバーは消し去る

マジックナンバーとは?

何らかの識別子もしくは定数として用いられる、プログラムのソースコード中に書かれた具体的な数値である。そのプログラムを書いた時点では製作者は数値の意図を把握しているが、他のプログラマーまたは製作者本人がマジックナンバーの意図を忘れたときに閲覧すると「この数字の意味はわからないが、とにかくプログラムは正しく動く。まるで魔法の数字だ」という皮肉を含む。

wikipediaより

なぜマジックナンバーはダメか

その開発者しか知り得ない数字だからです。具体的すぎて、意図がつかめないし、仕様に左右される数字だとさらに理解するのに時間がかかります。

例と改善案

例えば以下のようなコードがあったとします。

public function getGenderName (genderId) {
    if (genderId == 1) {
        return 'male'
    } else if ($genderId == 2) {
        return 'female'
    }
}

この例だと文脈でなんとなくわかりますが、パッと見た時に1 と 2 が何を指すのかわかりませんよね。
このように急に出てくる実装者しか知り得ない数字をなくすためにも、定数化したり変数化したりしてマジックナンバーに名前をつけることが重要です。
よくあるのが、タイムアウト処理の時間指定です。急に任意の数字が出てきて驚かれるので注意。

3.言語、フレームワークの規約に準拠しよう

昨今たくさんの種類の言語、それに伴うフレームワークが発明されています。
当たり前ですが、それぞれ文法が異なり、クセがあったりなかったりします。
それに加えて、規約が定められているものが多いです。
規約として守らなければならないが、エラーを吐くわけではないので、厄介です。
しかし、これらを守らずコードを書いていると、書き方が統一されず読みにくいコードにつながります
自分は最近Ruby（言語）とcakePHP（フレームワーク）を触っていたので、それらについて例を出します。

Ruby

Rubyにもいくつか規約があるのですが、弊社のソースコードが特に破っていた規約を紹介します。

• 真偽値を返すメソッドは末尾に ? をつけて、かつ is_ などの真偽値を表現する文字は使わない
• 変数名、関数名は全て小文字で単語の区切りには _ を使用する(スネークケース)
• file名は全て小文字で単語の区切りには - を使用(ケバブケース)
• 否定には ! を使わずに、Ruby特有である unless を使う

cakePHP

今度はフレームワークですが、こちらも（言語とは別に）規約があります。

• クラス名、関数名、変数名は、小文字で始まり、複数単語がある場合は、続く単語の頭文字を大文字にする(キャメルケース)
• コントローラーは全て複数形にして、末尾に Controller をつける
• 1行を120文字以上にしてはならない
• メソッドチェーンを使用する場合、複数行に改行し、空白４つでインデントする必要がある

おわりに

今回自分のまとめとしてこの記事を書くことにしました。
「基本のキ編」ということで、かなり常識的なことが書かれていることはご了承ください。。。
しかし、駆け出しのエンジニアとしてここら辺をおろそかにせず成長していくことに間違いはないと思っています。
冒頭でも言いましたが、弊社システムにはこのようなレガシーコードがたくさん潜んでおります。
自分はそれらと今後も戦って、アイスタイルのシステムを可読性抜群で拡張性があって高パフォーマンスなシステムにしたいと思っています。
アイスタイルでは、そんなシステムを一緒に作ってくれる仲間を募集しています！是非！！！