fix: SchedulePlace 중간 테이블 생성 #167
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
fix
|
출격준비 |
|
✅ 빌드 성공 ✅ |
KimDoubleB
reviewed
Aug 22, 2024
There was a problem hiding this comment.
아직 다 보지 못했지만, 중간 점검 차...!
- Schedule과 Place 매핑 테이블이여서
SchedulePlace로 정한 것 같은데... 이게 코드 상으로 쉽게 이해가 되지 않네요. 더 적합한 이름은 없을까요? 특정 스케줄의 장소 느낌...으로? - Place list가 필요한 곳에 SchedulePlace list가 포함된 곳들이 좀 보이는데요. 서비스 로직을 다 이해한 것은 아니지만, Place 정보만을 필요한 곳이라면 SchedulePlace 정보를 가지지 않고, Place list로 바꿔 이용하면 더 코드가 이해하기 좋을 것 같아요 (단순 생각이여서 틀린 말일수도...) 특히
SchedulePlaceId는 쉽사리 이해가 되지 않는 것 같아요 (매핑 테이블의 id이다보니 어떻게 활용하는지 쉽게 잘 이해가 안된다서 적어봅니다 )
새벽 리뷰라... 정확하지 않을 수 있는데... 아무튼... 한번 봐주시면 감사 감사 왕감사
Comment on lines
113
to
123
| private fun mapPlacesBySchedule( | ||
| schedules: List<Schedule>, | ||
| places: List<Place>, | ||
| ): Map<Schedule, List<Place>> { | ||
| val placesByScheduleId = places.groupBy { it.scheduleId } | ||
| schedulePlaces: List<SchedulePlace>, | ||
| ): Map<Schedule, List<SchedulePlace>> { | ||
| val schedulePlaceByScheduleId = schedulePlaces.groupBy { it.scheduleId } | ||
|
|
||
| return schedules.associateWith { schedule -> | ||
| placesByScheduleId[schedule.id] | ||
| schedulePlaceByScheduleId[schedule.id] | ||
| ?: throw PiikiiException( | ||
| exceptionCode = ExceptionCode.ILLEGAL_ARGUMENT_EXCEPTION, | ||
| detailMessage = "$EMPTY_CONFIRMED_PLACE Schedule ID: ${schedule.id}", |
There was a problem hiding this comment.
이 메서드는 schedule 당 place를 가지는거니깐
SchedulePlace가 아닌 List를 가지는게 더 말이 되지 않나요?
|
현상유지로 논의되어 해당 PR close |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
이슈
closed #166
변경 사항
스크린샷
부연 설명
체크리스트