はじめに
ecforce ma というサービスの開発に参画してから半年程度が経ちました。
当初、スピード重視であまりコードの品質にこだわらずにローンチを目指したサービスのようで、途中から参加した私が読めない（まったく何をしているのかわからない、あるいは理解するまでに時間がかかる）コードが多く、機能追加や不具合修正のたびに時間がかかっていました。
そのたびに、関連するコードの改善をしながら、今ではある程度読み易いコードになったと思いますので、この半年程度で遭遇し、そのたびに改善してきたことをまとめようと思います。
この記事のターゲット
- TypeScriptの経験が多くないが、業務でTypeScriptを書かないといけない人
- どんな書き方のコードをアンチパターンを捉えたらよいのか判断する材料がほしい人
フロントエンド、特にJavaScriptやTypeScriptにある程度精通している人にとっては、特に新しい情報は無いかもしれません。
具体例と解決策
1. 新しい配列の作成をforEach/ifで実装している
const newArray = [];
oldArray.forEach((a) => {
if (a === xxx) {
newArray.push(a);
}
});
new Array = [] で空配列を定義して、必要な中身を後から入れるという手法だと、コードを行ごとに取り出したときに「空配列の瞬間」と「一部の中身が入った瞬間」と「すべての中身が入った状態」に分かれてしまいます。このようなコードの場合、以下のように書くことで行単位での状態がひとつに定まり、シンプルにもなります。
const newArray = oldArray.filter((a) => a === xxx);
2. スコープの中で let による再宣言をしている
let isMatched = false;
numbers.forEach((n) => {
if (n === xxx) {
isMatched = true;
}
});
これも #1 と同様です。加えて、この場合、後続の処理でisMatchedがさらに書き換えられてしまう危険性を排除できません。以下のように書き換えられます。
const isMatched = numbers.some((n) => n === xxx);
同じパターンで、以下のように「分岐した結果をもとに代入しているケース」もあります。
let url = '';
if (xxx === yyy) {
url = 'yyy';
} else {
url = 'zzz';
}
このような場合も、後からurlを書き換えられることや、どのような条件でurlが書き換えられるのかをコード全体を追わないと正しく把握できなくなってしまいます。即時関数を用いてconstで定義しましょう。
const url = (() => {
if (xxx === yyy) {
return 'yyy';
} else {
return 'zzz';
}
})();
3. 過度に繰り返されているif文のネスト
if (xxx === aaa) {
if (yyy === bbb) {
if (zzz === ccc) {
// ...do something
}
}
}
ロジックを設計せずに書きながらコードを考えているとこのような書き方になりがちです。このような場合、複数の方法で解決できます。
const shouldDoSomething = xxx === aaa && yyy === bbb && zzz === ccc;
if (shouldDoSomething) {
// ...do something
}
if (xxx !== aaa) {
return;
}
if (yyy !== bbb) {
return;
}
if (zzz !== ccc) {
return;
}
// ...do something
また、以下のようなパターンもあります。これは冗長に見えますが、 文芸的プログラミング の観点から、ひとつひとつの処理の意味を伝わりやすくしたい場合に有用です。
function doSomething() {
// ...do something
}
function doSomethingWhenCcc() {
if (zzz === ccc) {
doSomething();
}
}
function doSomethingWhenBbb() {
if (yyy === bbb) {
doSomethingWhenCcc();
}
}
if (xxx === aaa) {
doSomethingWhenBbb();
}
4. undefinedなオブジェクトへのプロパティアクセス防止の記述
if (obj && obj.key) {
const value = obj.key;
doSomething(value);
}
型定義を正しくしている場合、undefined or null になりうるオブジェクトへのプロパティアクセスがエラーとなるため、先にオブジェクトの存在判定を行うのが定石となります。この場合、ES2020で追加されたオプショナルチェーンを使うことで、エラーを防ぐことができます。
const value = obj?.key;
if (value) {
doSomething(value);
}
5. 条件によって返り値が異なる場合の冗長な記述（.tsxの例）
if (xxx === aaa) {
return (
<div>
<h1>見出しaaa</h1>
<p>本文aaa</p>
</div>
);
} else {
return (
<div>
<h1>見出しbbb</h1>
<p>本文bbb</p>
</div>
);
}
xxx === aaa の場合と yyy === bbb の場合の返り値が似ています。共通項を変数を使って返すようにすると、パターンが増えた場合などの応用もすぐにできるようになります。
const headline = xxx === aaa ? '見出しaaa' : '見出しbbb';
const paragraph = xxx === aaa ? '本文aaa' : '本文bbb';
return (
<div>
<h1>{headline}</h1>
<p>{paragpraph}</p>
</div>
);
これでパターンがさらに増えた場合は、以下のように headline と paragraph を即時関数として切り出すことで、いくつのパターンでも対応できるようになります。
const headline = (() => {
if (xxx === aaa) {
return '見出しaaa';
} else if (yyy === bbb) {
return '見出しbbb';
} else {
return '見出しccc';
}
})();
const paragraph = (() => {
if (xxx === aaa) {
return '本文aaa';
} else if (yyy === bbb) {
return '本文bbb';
} else {
return '本文ccc';
}
})();
return (
<div>
<h1>{headline}</h1>
<p>{paragpraph}</p>
</div>
);
さらに要素が増えてくるとこれでも冗長なので、パターンをオブジェクトの配列として持ち、簡易的なDBのように処理するとさらに柔軟なコードになります。
const contents = [
{
type: 'aaa',
headline: '見出しaaa',
paragraph: '本文aaa',
},
{
type: 'bbb',
headline: '見出しbbb',
paragraph: '本文bbb',
},
{
type: 'ccc',
headline: '見出しccc',
paragraph: '本文ccc',
}
];
const type = (() => {
if (xxx === aaa) {
return 'aaa';
} else if (yyy === bbb) {
return 'bbb';
} else {
return 'ccc';
}
})();
const content = contents.find((c) => c.type === type);
return (
<div>
<h1>{content?.headline}</h1>
<p>{content?.paragpraph}</p>
</div>
);
6. 参照代入を使わないことによる冗長な記述
const userInfo = {
id: '1',
name: 'name',
};
const newUserInfo = {
email: 'email@example.com',
phone: '09011112222',
};
const newUser = {
id: userInfo.id,
name: userInfo.name,
email: newUserInfo.email,
phone: newUserInfo.phone,
};
上記のように「既存のユーザー情報」と「新しく追加したいユーザー情報」をまとめたひとつのオブジェクトを作成したい場合に、 userInfo. と newUserInfo. を何度も記載するのは冗長です。このような場合、参照代入を使ってオブジェクトの情報をクローンした変数インスタンスを用意することができます。
const userInfo = {
id: '1',
name: 'name',
};
const newUserInfo = {
email: 'email@example.com',
phone: '09011112222',
};
const { id, name } = userInfo;
const { email, phone } = newUserInfo;
const newUser = {
id: id,
name: name,
email: email,
phone: phone,
};
また、ES2015以降では、JavaScriptのキーの名前とプロパティの変数名が同じであれば、省略して記述できるので、さらに以下のように書くことができます。
const userInfo = {
id: '1',
name: 'name',
};
const newUserInfo = {
email: 'email@example.com',
phone: '09011112222',
};
const { id, name } = userInfo;
const { email, phone } = newUserInfo;
const newUser = {
id,
name,
email,
phone,
};
7. スプレッド構文を使わないことによる冗長な記述
上記の #6 のケースでは、実はそれ以前に スプレッド構文 を使うことにより、オブジェクトを簡単に展開することができます。
const userInfo = {
id: '1',
name: 'name',
};
const newUserInfo = {
email: 'email@example.com',
phone: '09011112222',
};
const newUser = {
...userInfo,
...newUserInfo
};
配列の場合も同様です。
const userIds = ['1', '2', '3'];
const newUserIds = ['4', '5', '6'];
const allUserIds = [
...userIds,
...newUserIds
];
8. 単純な条件一致に Array.prototype.some を使っている
const userId = '1';
const userIds = ['1', '2', '3'];
if (userIds.some((i) => i === userId)) {
// ...do something
}
Array.prototype.some は、条件に当てはまるかどうか比較を行う（計算をするなど）に対し、ES2016で追加された Array.prototype.includes() は、シンプルな「一致」を判定します。
const userId = '1';
const userIds = ['1', '2', '3'];
if (userIds.includes(userId)) {
// ...do something
}
Array.prototype.some は Array.prototype.includes よりも格段に処理に時間がかかることが明らかになっています。たとえば JavaScript の Array.some と Array.includes と Array.everyの実行速度比較 などの記事でその結果が紹介されています。
9. 意味のないコメント
// AをBにセットする
function setAtoB() {
// ...do something
}
言語を問わず、ソースコードではこのような「意味のないコメント」が見られることが少なからずあります。 setAtoB というメソッドに対して AをBにセットする というコメントをする意図は何でしょうか？ コメントに魂を込めよ（意訳）ということが、 リーダブルコード でも述べられています。このようなコードでは、「読み手に何を伝えたいのか」という気持ちが一切感じられません。（英語が読めない人がいるかもしれないというなら、思い切って関数名や変数名を日本語で定義すればよいでしょう。）
私のおすすめは、「コードから読み取れないこと（そのコードを実装するに至った背景や、修正・削除するタイミングなど）」をコメントすることです。
// この処理のロジックはチケット https://... に記載されています
// XXX機能でしか使わないので、XXX機能が不要になったらこのコードも不要になります
function setAtoB() {
// ...do something
}
10. メソッドの処理内容や目的をコメントで記載している
// Aが...に存在していたら...を実行する
function checkA() {
// ...do something
}
上で記載した #9 と同様、 リーダブルコード には「メソッドの処理内容をコメントで説明しなければならないなら、それはメソッド名がイケてないからだ（意訳）」ということが記載されています。上記の例では、 checkA というあまりにも抽象的なメソッド名のせいで、「Aの何をどうチェックするのか」がコード（またはコメント）を読まないと伝わらなくなっています。
冗長でも、下記のように「メソッド名を見たら処理内容（と、さらに言えばその目的）がわかる」ような記載を心がけましょう。
function do...WhenAexistsIn...() {
// ...do something
}
この例の場合、たとえばこのようなメソッド名を書いたときに「そもそも WhenAexistsIn... をさらに分割できないか？」ということに気が利くようになり、以下のようにさらに改善できるかもしれません。
function isAEsistsIn...() {
// ...check A existance in...
}
function do...() {
// ...do something
}
if (isAEsistsIn...()) {
do...();
}
いったん、思いついたことを羅列してみただけですので、今後追加するかもしれません。
本文中であげた リーダブルコード 以外にも、最近では以下のような書籍も出版されており、参考になるでしょう。