はじめに
新卒研修で成果物を作る機会があったのですが、最初に書いていたコードがかなり冗長で、リファクタリングを繰り返してコードを短くできたので、その一部を切り取って再現していきます。
実装したい関数 (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;
}
リファクタリング その1
先輩「同じ数字を使いまわすときは変数にしようね😊」
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
のところを変えるだけで済むようになりました。
リファクタリング その2
自分「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
文を一回書くだけで済むようになりました。ネストも浅くなりました。
リファクタリング その3
先輩「先にポイントを格納しておくオブジェクトを定義してしまったほうがいいんじゃない?」
自分「確かに」
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;
}
「ポイントを格納しておく変数を定義→最後にオブジェクトとしてまとめる」
だったのが、「ポイントを格納しておくオブジェクトを定義」
するだけで良くなったので、コードの量が減りました。
リファクタリング その4
自分「もしかして、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で取得
}
オブジェクトの中身をループさせるときにはこれが良さそう?