-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
♻️ 역 즐겨찾기 우선순위 적용 (#339) #340
Conversation
@discphy |
companion object { | ||
const val MAX_STATION_COUNT = 4 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
최대 가능 개수를 4로 확장하였습니다.
ALTER TABLE tb_member_station | ||
ADD COLUMN label VARCHAR(100) NULL AFTER member_station_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
즐겨찾기 역 라벨 컬럼을 추가하였습니다.
import backend.team.ahachul_backend.api.member.application.command.BookmarkStationCommand | ||
import backend.team.ahachul_backend.api.member.application.command.BookmarkStationCommands | ||
|
||
class BookmarkStationDto { | ||
|
||
data class Request( | ||
val stationNames: List<String> | ||
val stations: List<BookmarkStation> | ||
) { | ||
fun toCommand(): BookmarkStationCommand { | ||
return BookmarkStationCommand( | ||
stationNames = stationNames.toMutableList() | ||
fun toCommand(): BookmarkStationCommands { | ||
return BookmarkStationCommands( | ||
stations.map { BookmarkStationCommand(it.stationName, it.label) } | ||
) | ||
} | ||
} | ||
|
||
data class Response( | ||
val memberStationIds: List<Long> | ||
) | ||
|
||
data class BookmarkStation( | ||
val stationName: String, | ||
val label: String?, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존의 BookmarkStationCommand
를 BookmarkStationCommands
로 변경하고,
BookmarkStationCommand
를 새롭게 작성하였습니다.
이에 따른 API 요청 스펙을 변경하였습니다. stationNames
👉 stations
data class StationInfo( | ||
val stationId: Long, | ||
val stationName: String, | ||
val label: String?, | ||
val subwayLineInfoList: List<SubwayLineInfo> | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조회 API 응답 스펙에도 label
을 추가하였습니다. 😂
리뷰 부탁드립니다! 🙇♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
memberStationWriter.delete(it.id) | ||
} | ||
if (originMemberStations.isNotEmpty()) { | ||
memberStationWriter.deleteAllByMember(member) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 이러면 즐겨찾기 역 순서가 바뀐게 없을 경우에도 무조건 다 지우고 다시 생성하게 되는건가요? 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇,, 그러네요.. 기존 데이터와 동일하다면 예외 처리 해주는게 좋을까요? 🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네넵 그게 좋아보여요 현재 방법은 조금 비효율적인 것 같아서요..!
사실상 순서를 바꾸는 경우는 그렇게 많을 것 같지 않아서 예외처리만 해놔도 DB에 부담을 덜 줄 것 같습니당.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@semi-cloud 수정완료했습니다..!!
memberStationWriter.delete(it.id) | ||
} | ||
if (isAlreadyRegisteredStation(originMemberStations, bookmarkStations)) { | ||
throw CommonException(ResponseCode.ALREADY_REGISTERED_STATION) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@discphy
음 사실 유저 입장에서 비정상적인 동작은 아닌거라, 예외를 던지기보다는 그냥 같지 않을 경우에만 삭제하고 새로 넣는 작업을 수행하는 방식이 더 나아보이는데 어떻게 생각하시나요? 👀
if (!isAlreadyRegisteredStation(originMemberStations, bookmarkStations)) {
// 삭제하고 새로 넣는 작업 수행
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기존에는 등록 후, 즐겨찾기 역 정보의 식별자 리스트를 반환하고 있습니다. 예외를 던지지 않으면 빈 컬렉션 객체를 응답할 것 같은데 이 부분도 수정되어야 하지 않을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네네 예외 던지지 않는 대략적인 플로우만 말씀드린거라 디테일한 부분들은 처리해주시면 될 것 같아요!
꼭 저렇게 말고 다르게 처리하셔도 됩니다.
아예 같은 순서로 저장이 오는 경우는 많지 않을테니 그냥 전체 삭제하고 넣어도 괜찮지 않을까 싶습니다. |
override fun bookmarkStation(command: BookmarkStationCommands): BookmarkStationDto.Response { | ||
override fun bookmarkStation(command: BookmarkStationCommands): GetBookmarkStationDto.Response { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조회 API와 동일한 응답 객체 반환 적용
if (isAlreadyRegisteredStation(originMemberStations, bookmarkStations)) { | ||
throw CommonException(ResponseCode.ALREADY_REGISTERED_STATION) | ||
if (isEqualsAlreadyRegisteredStation(bookmarkStations, command.stations)) { | ||
return createBookmarkStationResponse(bookmarkStations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 네이밍을 명확하게 변경하고 예외처리 대신 기존 데이터를 반환합니다. ⭐️
@hwgyun 아이고.. 규연님 일단!! 세미님 의견을 수용해서 수정을 했습니다. 수정한 코드 기준으로 다시 한번 리뷰 부탁드립니다!! |
별도 쿼리 없이 식별자 리스트를 가져올 수 있군요! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아예 같은 순서로 저장이 오는 경우는 많지 않을테니 그냥 전체 삭제하고 넣어도 괜찮지 않을까 싶습니다.
아? 제가 잠깐 기획을 헷갈렸군요 별도 수정 버튼을 눌러야 순서가 변경이 가능한거면 리팩토링 이전 방식도 좋습니다!
죄송합니다 한영님.. 🥲 되돌리셔도 괜찮고, 지금 코드 유지하셔도 좋아요. 어프로브 하겠습니다!
📚 개요
✏️ 작업 내용
💡 주의 사항