さとうです！
今日、SNSでこんなものを見かけました。
このコードをレビューしてみようと思います！
目次
責務を分離する
まず気になったのは、メソッドのシグネチャーです
function manageReport(
report: Report,
operation: 'create' | 'edit' | 'publish',
newTitle?: string,
newContent?: string,
newAuther?: string,
newDate?: Date,
): void
operationでどんな処理をしたいのか、を受け取り、内部で分岐させています。
これは、ひとつのメソッドに3つの責務が詰められていることになるので、3つのメソッドに分割しましょう！
分割すると、このようなメソッドになるはずです。
function createReport(
report: Report,
title: string,
content: string,
author: string,
date: Date,
): void
function editReport(
report: Report,
title: string,
content: string,
author: string,
date: Date,
): void
function publishReport(
report: Report,
): void
とりあえず、これで責務を分離することができました！
switch文を使う
元のコードでは、このようなことをしていました。
if(operation === 'create'){
// レポート作成
} else if(operation === 'edit'){
// レポート編集
} else if(operation === 'publish'){
// レポート公開
}
operationのような列挙型の場合、switch文と相性が良いです。
このようになります。
switch(operation){
case 'create':
// レポート作成
break;
case 'edit':
// レポート編集
break;
case 'create':
// レポート公開
break;
default:
throw new Error();
}
プログラミング言語によっては、ENUMに追加があった場合、switch文がコンパイルエラーになってくれたりします。
早期リターンを使う
先ほどはswitch文を使いましたが、他のパターンも見てみましょう。
改めて、元のコードはこちらです。
if(operation === 'create'){
// レポート作成
} else if(operation === 'edit'){
// レポート編集
} else if(operation === 'publish'){
// レポート公開
}
else ifを何回も使うのではなく、早期リターンをするようにしましょう！
このようになります。
if(operation === 'create'){
// レポート作成
return
}
if(operation === 'edit'){
// レポート編集
return
}
if(operation === 'publish'){
// レポート公開
return
}
throw new Error()
シグネチャーをシンプルにする
責務を分離する で、メソッドを分割しました。
function createReport(
report: Report,
title: string,
content: string,
author: string,
date: Date,
): void
function editReport(
report: Report,
title: string,
content: string,
author: string,
date: Date,
): void
function publishReport(
report: Report,
): void
ただし、まだ問題があります。
Reportを受け取っているにも関わらず、引数が多いですね！
修正すると、このようになります。
function createReport(report: Report): void
function editReport(report: Report): void
function publishReport(report: Report): void
かなりよくなりましたね！
テスト容易性を上げる
シグネチャーをシンプルにする で良い感じにシグネチャーをシンプルにできましたが、実はまだ問題があります…。
function createReport(report: Report): void
function editReport(report: Report): void
これ、めっちゃテストしづらいです！
テスト容易性を上げるために、戻り値を変更しましょう！
このようになります。
function createReport(report: Report): ReportId // ReportIdを内部で生成するイメージ
function editReport(report: Report): Report // immutableな処理をするイメージ
ここまできたら、Report Entityを作る、という話にできそうですね！
コードのイメージはこんな感じです。
class Report{
constructor(
public readonly reportId: ReportId,
public readonly title: Title,
public readonly content: Content,
public readonly author: Author,
public readonly createdAt: CreatedAt,
){}
function edit(title: Title): Report {
return new Report(
this.reportId,
title,
this.content,
this.author,
this.createdAt,
)
}
function edit(content:Content): Report{
// edit(title)とほとんど同じ処理
}
function edit(author: Author): Report{
// edit(title)とほとんど同じ処理
}
}
(空で書いているので、コンパイルできないかも…)
注目ポイントは、editメソッドがすべてimmutableな処理になっている部分ですね！
また、デフォルト引数を使えば、editメソッドはひとつにまとめられそうです。
さらに、editメソッドがあるのであれば、updatedAtフィールドもあったほうがよさそうですね！
Reportオブジェクトで外部サービスの呼び出しを行わない
元のコードを見てみると、RepositoryやServiceを直接呼び出してしまっています。
下記のような感じで呼び出したいですね！
class CreateReport{
@Inject // DI
const reportRepository: ReportRepository
function handle(input:CreateReportOptions):UUID {
const reportId = new ReportId(UUID.randomUUID)
const title = new Title(input.title)
const content = new Content(input.content)
const author = new Author(input.author)
const createdAt = new CreatedAt(Date())
const report = new Report(reportId, title, content, author, createdAt) // 名前付き引数を使ったほうが良いですね！
reportRepository.add(report)
return reportId.value
}
}
class PublishReport{
@Inject // DI
const reportRepository: ReportRepository
@Inject // DI
const reportNoticeService: ReportNoticeService
function handle(input:UUID): void{
const reportId = new ReportId(input)
const report = reportRepository.resolveBy(reportId)
reportNoticeService.publish(report)
}
}
おわりに
全ての行に指摘が入るという悲しい結果になってしまいました…。
ただ、ここらへんはすごいミスりやすいところなので、注意していきたいですね！
※今回出てきたサンプルコードは空で書いたので、コンパイルが通らない可能性があります。その場合、雰囲気だけ感じとってもらえると！ｗ
また、おかしな部分があったら↓から連絡ください！