refactor: portfolio 도메인 마이그레이션 #76
Conversation
yechan-kim
left a comment
yechan-kim
left a comment
There was a problem hiding this comment.
고생하셨습니다. 근표님!
코멘트 확인 부탁드릴게요~
| @OneToMany(mappedBy = "portfolio", fetch = FetchType.EAGER, orphanRemoval = true) | ||
| @JsonIgnore | ||
| val contributors: MutableList<Contributor> = mutableListOf() |
There was a problem hiding this comment.
List와 MutableList의 차이를 알 수 있을까요?
There was a problem hiding this comment.
가변/불변 차이로 알고있습니다
Contributors는 그 list 원소가 변경될 수 있으므로 MutableList를 사용했습니다
| enum class ProjectScope(val projectScope: String) { | ||
| TOY("Toy"), | ||
| FINAL("Final") | ||
| } No newline at end of file |
| return if (generation == null) { | ||
| listOf( | ||
| getPortfoliosByScope(ProjectScope.FINAL), | ||
| getPortfoliosByScope(ProjectScope.TOY) | ||
| ) | ||
| } else { | ||
| listOf( | ||
| getPortfoliosByGenerationAndScope(generation.toLong(), ProjectScope.FINAL), | ||
| getPortfoliosByGenerationAndScope(generation.toLong(), ProjectScope.TOY) | ||
| ) | ||
| } |
There was a problem hiding this comment.
early return 패턴을 사용하면, 코드의 가독성이 더 좋아질 것 같아요!
There was a problem hiding this comment.
기존 코드 흐름을 준수하기 위해 별도 수정을 진행하지 않았습니다.
변경하는게 더 나아보이시나요?
| private fun getPortfoliosByScope(scope: ProjectScope): List<PortfoliosResponse> { | ||
| return portfolioRepository.findAllByScopeOrderByGenerationDesc(scope) | ||
| .map { PortfoliosResponse.from(it) } | ||
| .toList() | ||
| } | ||
|
|
||
| private fun getPortfoliosByGenerationAndScope( | ||
| generation: Long, | ||
| scope: ProjectScope | ||
| ): List<PortfoliosResponse> { | ||
| return portfolioRepository.findAllByGenerationAndScope(generation, scope) | ||
| .map { PortfoliosResponse.from(it) } | ||
| .toList() | ||
| } |
There was a problem hiding this comment.
위의 코드를 보아하니, all method에서만 사용하는 코드인 것 같네요.
두 method의 차이는 generation의 유무에 따라 사용되는 쿼리 차이인 것 같은데,
두 method를 통합하고, 분기를 통합된 메서드 내부에서 진행하는 건 어떤가요?
아래는 제가 제안한 내용에 대한 대략적인 코드입니다!
private fun getPortfolios(
generation: Long?,
scope: ProjectScope
): List<PortfoliosResponse> {
val list = if (generation == null) {
portfolioRepository.findAllByScope(scope)
} else {
portfolioRepository.findAllByGenerationAndScope(generation, scope)
}
return list
.map { PortfoliosResponse.from(it) }
.toList();
}There was a problem hiding this comment.
여기도 마찬가지로 기존 비즈니스 로직을 그대로 변경하자는 요구사항이 있어 별도 흐름 수정을 진행하지 않았습니다
변경하는게 더 좋아보이시나요? 의견 부탁드립니다
There was a problem hiding this comment.
개인적으로는 기존의 흐름을 많이 해치지 않는 개선은 괜찮다고 생각합니다!
@jwnnoh 님의 의견이 궁금해요!
There was a problem hiding this comment.
두 분 다 충분히 고민해주신 것 같아서 고민을 하고 있었는데 제가 좀 늦게 왔네요 ㅎㅎ..
기존 흐름이 크게 변경되지는 않는 변경점인 것 같아서 변경하는 것도 좋아보입니다!
고생하셨어요 ❤️
1. 무슨 이유로 코드를 변경했나요?
portfolio 도메인 마이그레이션을 진행했습니다
close #75
2. 어떤 위험이나 장애를 발견했나요?
양방향 매핑이 있긴한데 일단 기존 로직 그대로 마이그레이션 진행했습니다
3. 관련 스크린샷을 첨부해주세요.
4. 완료 사항
5. 추가 사항