コードレビューしてみよう！②

[さとう](https://www.notion.so/f5514364f03a412f8af6c09b352c9d64?pvs=21)です！

今日、SNSでこんなものを見かけました。

https://x.com/yumemiinc/status/1724632711648911697?s=20

このコードをレビューしてみようと思います！

# 目次

# 責務を分離する

まず気になったのは、メソッドのシグネチャーです

```tsx
function manageReport(
	report: Report,
	operation: 'create' | 'edit' | 'publish',
	newTitle?: string,
	newContent?: string,
	newAuther?: string,
	newDate?: Date,
): void
```

`operation`でどんな処理をしたいのか、を受け取り、内部で分岐させています。

これは、ひとつのメソッドに3つの責務が詰められていることになるので、3つのメソッドに分割しましょう！

分割すると、このようなメソッドになるはずです。

```tsx
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文を使う

元のコードでは、このようなことをしていました。

```tsx
if(operation === 'create'){
	// レポート作成
} else if(operation === 'edit'){
	// レポート編集
} else if(operation === 'publish'){
	// レポート公開
}
```

`operation`のような列挙型の場合、switch文と相性が良いです。

このようになります。

```tsx
switch(operation){
	case 'create':
		// レポート作成
		break;
	case 'edit':
		// レポート編集
		break;
	case 'create':
		// レポート公開
		break;
	default:
		throw new Error();
}
```

プログラミング言語によっては、ENUMに追加があった場合、switch文がコンパイルエラーになってくれたりします。

# 早期リターンを使う

先ほどはswitch文を使いましたが、他のパターンも見てみましょう。

改めて、元のコードはこちらです。

```tsx
if(operation === 'create'){
	// レポート作成
} else if(operation === 'edit'){
	// レポート編集
} else if(operation === 'publish'){
	// レポート公開
}
```

`else if`を何回も使うのではなく、早期リターンをするようにしましょう！

このようになります。

```tsx
if(operation === 'create'){
	// レポート作成
	return
} 

if(operation === 'edit'){
	// レポート編集
	return
}

if(operation === 'publish'){
	// レポート公開
	return
}

throw new Error()
```

# シグネチャーをシンプルにする

[責務を分離する](https://www.notion.so/269f70facee64ab78290a21b7314e70d?pvs=21) で、メソッドを分割しました。

```tsx
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`を受け取っているにも関わらず、引数が多いですね！

修正すると、このようになります。

```tsx
function createReport(report: Report): void

function editReport(report: Report): void

function publishReport(report: Report): void
```

かなりよくなりましたね！

# テスト容易性を上げる

[シグネチャーをシンプルにする](https://www.notion.so/4bebc477ad7d4396b8c455c431f8673c?pvs=21) で良い感じにシグネチャーをシンプルにできましたが、実はまだ問題があります…。

```tsx
function createReport(report: Report): void

function editReport(report: Report): void
```

これ、めっちゃテストしづらいです！

テスト容易性を上げるために、戻り値を変更しましょう！

このようになります。

```tsx
function createReport(report: Report): ReportId // ReportIdを内部で生成するイメージ

function editReport(report: Report): Report // immutableな処理をするイメージ
```

ここまできたら、Report Entityを作る、という話にできそうですね！

コードのイメージはこんな感じです。

```tsx
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を直接呼び出してしまっています。

下記のような感じで呼び出したいですね！

```tsx
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
	}
}
```

```tsx
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)
	}
}
```

# おわりに

全ての行に指摘が入るという悲しい結果になってしまいました…。

ただ、ここらへんはすごいミスりやすいところなので、注意していきたいですね！

※今回出てきたサンプルコードは空で書いたので、コンパイルが通らない可能性があります。その場合、雰囲気だけ感じとってもらえると！ｗ

また、おかしな部分があったら↓から連絡ください！

[さとう (@satou_aaaaa_111) on X](https://twitter.com/satou_aaaaa_111)

[記事一覧](https://www.notion.so/0e521860936042528997c9a33f6e1a20?pvs=21)