LoginSignup
11
9
お題は不問！Qiita Engineer Festa 2024で記事投稿！
Qiita Engineer Festa20242024年7月17日まで開催中！
@che-ryu(naga shima)in株式会社スカラパートナーズ

新卒研修で書いたクソコードをリファクタリングする

Posted at

はじめに

新卒研修で成果物を作る機会があったのですが、最初に書いていたコードがかなり冗長で、リファクタリングを繰り返してコードを短くできたので、その一部を切り取って再現していきます。

実装したい関数 (JavaScript)

とある関数をJavaScriptで作っていたのですが、その部分が特に冗長で、かつリファクタリングでかなり改善できたので、その関数とリファクタリングの流れを再現しながら紹介していきます。

関数に渡すもの

hogeMain, hogeSub1, hogeSub2, fugaMain, fugaSub1, fugaSub2 のプロパティからなるオブジェクト。
バリューには'foo', 'bar', 'baz', 'qux', 'quux' のいずれかが与えられている。

const structure = {
    hogeMain: 'foo',    // 10点
    hogeSub1: 'baz',    // 3点
    hogeSub2: 'quux',   // 3点
    fugaMain: 'bar',    // 10点
    fugaSub1: 'baz',    // 3点
    fugaSub2: 'bar',    // 3点 
};

関数が返すもの

fooPoint, barPoint, bazPoint, quxPoint, quuxPoint のプロパティからなるオブジェクト。
hogeMain または fugaMain に入っていたものについては10点、それ以外については3点にポイント換算して、項目ごとの合計ポイントをバリューに格納する。

const points = {
    fooPoint: 10,   // 10点(hogeMain)
    barPoint: 13,   // 10点(fugaMain) + 3点(fugaSub2)
    bazPoint: 6,    // 3点(hogeSub1) + 3点(fugaSbu1) 
    quxPoint: 0,    // 'qux'はどこにもないので0点
    quuxPoint: 3,   // 3点(hogeSub2)
}

最初に書いたコード

とりあえずポイントを格納しておく変数を作って〜、
与えられたオブジェクトのキーが「hogeMain または fugaMain」の時と「それ以外」の時で場合分けをして〜、
与えられたオブジェクトのバリューが'foo', 'bar', ... の時でそれぞれポイントを加算して〜、
最後にオブジェクトとして返せばいけそう！

const structure = {
    hogeMain: 'foo',
    hogeSub1: 'baz',
    hogeSub2: 'quux',
    fugaMain: 'bar',
    fugaSub1: 'baz',
    fugaSub2: 'bar', 
};

function calculatePoints(structure) {
    let fooPoint = 0;
    let barPoint = 0;
    let bazPoint = 0;
    let quxPoint = 0;
    let quuxPoint = 0;

    for(const [key, value] of Object.entries(structure)) {
        if(key === 'hogeMain' || key === 'fugaMain') {
            switch(value) {
                case 'foo':
                    fooPoint += 10;
                    break;
                case 'bar':
                    barPoint += 10;
                    break;
                case 'baz':
                    bazPoint += 10;
                    break;
                case 'qux':
                    quxPoint += 10;
                    break;
                case 'quux':
                    quuxPoint += 10;
                    break;
            }
        } else {
            switch(value) {
                case 'foo':
                    fooPoint += 3;
                    break;
                case 'bar':
                    barPoint += 3;
                    break;
                case 'baz':
                    bazPoint += 3;
                    break;
                case 'qux':
                    quxPoint += 3;
                    break;
                case 'quux':
                    quuxPoint += 3;
            }
        }
    }

    const points = {
        fooPoint: fooPoint,
        barPoint: barPoint,
        bazPoint: bazPoint,
        quxPoint: quxPoint,
        quuxPoint: quuxPoint,
    };
    return points;  
}

リファクタリング その１

先輩「同じ数字を使いまわすときは変数にしようね😊」

const structure = {
    hogeMain: 'foo',
    hogeSub1: 'baz',
    hogeSub2: 'quux',
    fugaMain: 'bar',
    fugaSub1: 'baz',
    fugaSub2: 'bar', 
};

function calculatePoints(structure) {
    let fooPoint = 0;
    let barPoint = 0;
    let bazPoint = 0;
    let quxPoint = 0;
    let quuxPoint = 0;

    let addPoint;
    for(const [key, value] of Object.entries(structure)) {
        if(key === 'hogeMain' || key === 'fugaMain') {
            addPoint = 10; // 加算するポイントを変数に格納
            switch(value) {
                case 'foo':
                    fooPoint += addPoint;
                    break;
                case 'bar':
                    barPoint += addPoint;
                    break;
                case 'baz':
                    bazPoint += addPoint;
                    break;
                case 'qux':
                    quxPoint += addPoint;
                    break;
                case 'quux':
                    quuxPoint += addPoint;
                    break;
            }
        } else {
            addPoint = 3; // 加算するポイントを変数に格納
            switch(value) {
                case 'foo':
                    fooPoint += addPoint;
                    break;
                case 'bar':
                    barPoint += addPoint;
                    break;
                case 'baz':
                    bazPoint += addPoint;
                    break;
                case 'qux':
                    quxPoint += addPoint;
                    break;
                case 'quux':
                    quuxPoint += addPoint;
                    break;
            }
        }
    }

    const points = {
        fooPoint: fooPoint,
        barPoint: barPoint,
        bazPoint: bazPoint,
        quxPoint: quxPoint,
        quuxPoint: quuxPoint,
    };
    return points;  
}

作るシステムの都合上、加算するポイントを変更することはほぼないのですが、仮に変更したくなったときは
addPoint = 10, addPoint = 3のところを変えるだけで済むようになりました。

リファクタリング その２

自分「switchの部分が重複しているから、一箇所にまとめられそう！」

const structure = {
    hogeMain: 'foo',
    hogeSub1: 'baz',
    hogeSub2: 'quux',
    fugaMain: 'bar',
    fugaSub1: 'baz',
    fugaSub2: 'bar', 
};

function calculatePoints(structure) {
    let fooPoint = 0;
    let barPoint = 0;
    let bazPoint = 0;
    let quxPoint = 0;
    let quuxPoint = 0;

    let addPoint;
    for(const [key, value] of Object.entries(structure)) {
        if(key === 'hogeMain' || key === 'fugaMain') {
            addPoint = 10;
        } else {
            addPoint = 3;
        }
        // switch文をif文の外に出し、重複を解消
        switch(value) {
            case 'foo':
                fooPoint += addPoint;
                break;
            case 'bar':
                barPoint += addPoint;
                break;
            case 'baz':
                bazPoint += addPoint;
                break;
            case 'qux':
                quxPoint += addPoint;
                break;
            case 'quux':
                quuxPoint += addPoint;
                break;
        }
    }

    const points = {
        fooPoint: fooPoint,
        barPoint: barPoint,
        bazPoint: bazPoint,
        quxPoint: quxPoint,
        quuxPoint: quuxPoint,
    };
    return points;
}

加算させるポイントのif文を先に完結させることで、switch文を一回書くだけで済むようになりました。ネストも浅くなりました。

リファクタリング その３

先輩「先にポイントを格納しておくオブジェクトを定義してしまったほうがいいんじゃない？」

自分「確かに」

const structure = {
    hogeMain: 'foo',
    hogeSub1: 'baz',
    hogeSub2: 'quux',
    fugaMain: 'bar',
    fugaSub1: 'baz',
    fugaSub2: 'bar', 
};

function calculatePoints(structure) {
    // ポイントを格納する変数を定義する代わりに、オブジェクトをあらかじめ定義しておく
    const points = {
        fooPoint: 0,
        barPoint: 0,
        bazPoint: 0,
        quxPoint: 0,
        quuxPoint: 0,
    };

    let addPoint;
    for(const [key, value] of Object.entries(structure)) {
        if(key === 'hogeMain' || key === 'fugaMain') {
            addPoint = 10;
        } else {
            addPoint = 3;
        }
        switch(value) {
            case 'foo':
                points.fooPoint += addPoint;
                break;
            case 'bar':
                points.barPoint += addPoint;
                break;
            case 'baz':
                points.bazPoint += addPoint;
                break;
            case 'qux':
                points.quxPoint += addPoint;
                break;
            case 'quux':
                points.quuxPoint += addPoint;
                break;
        }
    }

    return points; 
}

「ポイントを格納しておく変数を定義→最後にオブジェクトとしてまとめる」だったのが、「ポイントを格納しておくオブジェクトを定義」するだけで良くなったので、コードの量が減りました。

リファクタリング その４

自分「もしかして、switch文なくせる？」

const structure = {
    hogeMain: 'foo',
    hogeSub1: 'baz',
    hogeSub2: 'quux',
    fugaMain: 'bar',
    fugaSub1: 'baz',
    fugaSub2: 'bar', 
};

function calculatePoints(structure) {
    const points = {
        fooPoint: 0,
        barPoint: 0,
        bazPoint: 0,
        quxPoint: 0,
        quuxPoint: 0,
    };

    let addPoint;
    for(const [key, value] of Object.entries(structure)) {
        if(key === 'hogeMain' || key === 'fugaMain'){
            addPoint = 10;
        } else {
            addPoint = 3;
        }     
        // pointsのバリューにポイントを足していく
        // 例：structureのバリューがfooであれば、points[fooPoint]に10or3を足す
        points[`${value}Point`] += addPoint;
    }
    return points;
}

ドット記法myObj.keyの場合はキーの名前を直接書かないとアクセスできないのですが、ブラケット記法myObj['key']の場合は[ ]内に変数を使うことができます。

そのため、switchで全パターンを記述する必要がなくなりました。

最初に書いたコードと比較すると、だいぶスッキリしました。

おわりに

研修を一通り終えて、この記事を作成するタイミングでさらにリファクタリングをして、極限までコードの量を減らしてみました。

const items = ['foo', 'bar', 'baz', 'qux', 'quux'];
const structure = {
    hogeMain: 'foo',
    hogeSub1: 'baz',
    hogeSub2: 'quux',
    fugaMain: 'bar',
    fugaSub1: 'baz',
    fugaSub2: 'bar', 
};

function calculatePoints(items, structure) {
    // 各項目のポイントを格納しておくpointsの作成
    const points = {};
    for(const item of items) {
        points[`${item}Point`] = 0;
    }
    // pointsにポイントを足していく
    for(const [key, value] of Object.entries(structure)) {
        key.includes('Main') ? points[`${value}Point`] += 10 : points[`${value}Point`] += 3;
    }
    return points;
}

項目が増えたとしても、関数のコード量は一定なのが良さそうですね。

おまけ

ループ処理についてはもともと、Object.keys(myObj)でキーの配列を受け取って、forEachで書いていたのですが、

    Object.keys(myObj).forEach((key) => {
        // 処理
        // バリューはmyObj[key]で取得
    });

for of文というものも配列に対して使えるらしく、

    for(const key of Object.keys(myObj)) {
        // 処理
        // バリューはmyObj[key]で取得
    }

さらにObject.entries(myObj)を使うとバリューも取得してくれるので、

    for(const [key, value] of Object.entries(myObj)) {
        // 処理
        // バリューはvalueで取得
    }

オブジェクトの中身をループさせるときにはこれが良さそう？

11
9
0

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
Sign upLogin
11
9