さとうです！
今日、SNSでこんなものを見かけました。
このコードをレビューしてみようと思います！
元のコード
const alertPrice = (price: number) => {
const tax = 0.1;
const priceWithTax = price + price * tax;
const message = `税込金額は${priceWithTax}円です。`;
alert(message);
}
税込金額を計算して画面に表示する、ということをやっていますね！
では、いくつか指摘してみましょう！
基本データ型ではなくValueObjectを使う
まず僕が気になったのは、number型を使っているところです。
const alertPrice = (price: number) => {
TypeScriptのnumber型は大きい値を受け取ることができますが、最大値を受け取ってしまった場合、計算する部分でエラーが発生しそうです。
// number型で定義されている最大値(9007199254740991)を超えてしまう可能性がある。
const priceWithTax = price + price * tax;
このようなことがあるため、基本データ型をそのまま使うのではなく、ValueObjectを受け取るようにしましょう。
// ValueObjectを受け取る
const alertPrice = (price: Price) => {
このValueObjectのコンストラクターで最大値を設定するようにしましょう！
関数の中から値を抽出する
次に気になったのは、taxを宣言している部分です。
const tax = 0.1;
関数の中でtaxを宣言するのではなく、関数でtaxを受け取るようにした方が、一つの関数で複数のtaxに対応することができます。
// taxを受け取るようにする
const alertPrice = (price: Price,tax: Tax) => {
ここでももちろんValueObjectを使うようにしましょう。
これができると、税率を計算している部分をValueObjectに切り出すことができそうです。
// 計算ロジックをTaxに委譲する
const priceWithTax = tax.calculat(price);
ロジックと見た目は分離する
もう一度元のコードを見てみましょう。
const alertPrice = (price: number) => {
const tax = 0.1;
const priceWithTax = price + price * tax;
const message = `税込金額は${priceWithTax}円です。`;
alert(message);
}
この関数では、「税率計算」と「画面への表示」の2つの責務を負ってしまっています。
この場合、業務ロジックである「税率計算」は分離した方が良いです。
こちらを参考
見た目を分離した場合、このようなコードが考えられます。(今までの指摘を取り込んでいます)
function createPriceWithTax(price: Price,tax: Tax): PriceWithTax {
const priceWithTax = tax.calculat(price);
return new PriceWithTax(priceWithTax);
}
このようにすることで、業務ロジックだけを抽出することができました。
(元のコードと比べると、完全に別物になってますね！)
createPriceWithTaxはPriceWithTaxのファクトリーメソッドみたいになってますね！
createPriceWithTaxでPriceとTaxを受け取ることにより、依存関係を明示することもできています。
今の依存関係はこんな感じになっています。TaxがPriceに依存している部分は微妙な感じがしますが、コードを少しいじれば改善できそうですね。
おわりに
全ての行に指摘が入るという悲しい結果になってしまいました…。
ただ、ここらへんはすごいミスりやすいところなので、注意していきたいですね！
※今回出てきたサンプルコードは空で書いたので、コンパイルが通らない可能性があります。その場合、雰囲気だけ感じとってもらえると！ｗ
また、おかしな部分があったら↓から連絡ください！