🔀 :: (#71) 알림 카테고리 활성화 / 비활성화 변경 #225
Conversation
lyutvs
requested review from
leeseojune53,
jeongho1209,
jhhong0509,
eogus2513,
oliver37632,
jaemin05,
alsco39 and
jyk1029
as code owners
| fun saveSetting(categories: List<Category>, userId: UUID, isActivated: Boolean): List<Setting> | ||
| fun updateSetting(categories: List<Category>, userId: UUID, isActivated: Boolean): List<Setting> | ||
| fun settingExist(categories: List<Category>, userId: UUID): Boolean |
| } | ||
|
|
||
| override fun updateSetting(category: Category, userId: UUID, isActivated: Boolean): Setting { | ||
| private fun saveSettingId(settingId: SettingId, isActivated: Boolean): Setting { |
There was a problem hiding this comment.
saveSettingId가 아니라 updateIsActiveById에 맞지 않나?
There was a problem hiding this comment.
아 그러네요 isActivated를 받는다는걸 생각 못했네요
| @@ -33,31 +34,21 @@ class SettingController( | |||
| } | |||
|
|
|||
| @Operation(summary = "알림 카테고리 활성화") | |||
| override fun saveAllSetting(categories: List<Category>, userId: UUID, isActivated: Boolean): List<Setting> { | ||
| return getSettingIdList(categories, userId).map { | ||
| updateIsActiveById(it, isActivated) | ||
| } | ||
| } | ||
|
|
||
| override fun updateAllSetting(categories: List<Category>, userId: UUID, isActivated: Boolean): List<Setting> { | ||
| return getSettingIdList(categories, userId).map { | ||
| updateIsActiveById(it, isActivated) | ||
| } |
There was a problem hiding this comment.
Q) 여기는 구현되어있는 부분이 동일한 것 같은데 분리해서 사용하는 이유가있나요?
There was a problem hiding this comment.
override fun saveSetting(category: Category, userId: UUID, isActivated: Boolean): Setting {
return settingMapper.settingEntityToDomain(
settingRepository.save(
SettingEntity(
settingId = getSettingId(category, userId),
isActivated = isActivated
)
)
)
}
override fun updateSetting(category: Category, userId: UUID, isActivated: Boolean): Setting {
return settingMapper.settingEntityToDomain(
settingRepository.save(
SettingEntity(
settingId = getSettingId(category, userId),
isActivated = isActivated
)
)
)
}변경하기 전 코드도 동일한 로직이어서 구현은 분리해서 해놨습니다 이거 사용하는 UseCase가 save또는 update 두가지를 가져서 구분해둔것 같습니다
There was a problem hiding this comment.
음.. 사용하는 UseCase에서 Save, Update를 나눈게 http status(201, 204)때문인걸로 기억하는데, save/update하는거는 위에서 호출하고 save/update여부에 따라 status만 반환하는게 있으면 되지 않을까 라는 생각이 듭니다.
굳이 동일한 method 2개를 유지할 필요는 없어보여서요!
| override fun saveAllSetting(categories: List<Category>, userId: UUID, isActivated: Boolean): List<Setting> { | ||
| return getSettingIdList(categories, userId).map { | ||
| updateIsActiveById(it, isActivated) | ||
| } | ||
| } |
There was a problem hiding this comment.
p4) 개별적으로 save하는게 아니라 Entity를 만들어준걸 바탕으로 saveAll을 호출하는게 좋을 것 같아요
There was a problem hiding this comment.
또는 .map을 사용하지않고, getSettingIdList: List을 updateIsActiveById에 넘겨줘서 한 번에 하는게 좋을지도..?
그냥 의견임니당
| isActivated = isActivated | ||
| ) | ||
|
|
||
| override fun updateAllSetting(categories: List<Category>, userId: UUID, isActivated: Boolean): List<Setting> { |
There was a problem hiding this comment.
@leeseojune53 전부터 궁금했는데 update인데 save 형태로 동작한 이유가 궁금합니다
There was a problem hiding this comment.
Update 동작을 하는데 save method를 사용한걸 이야기하는건가유?
There was a problem hiding this comment.
네네 맞아요 updateSetting이라는 메소드 안에 save method를 사용한 이유가 궁금해요
There was a problem hiding this comment.
Jpa에서 제일 간편하게 update 수행하는게 기존 Entity에서 값 변경 후 save라서 사용했습니다 :)
| override fun activateCategory(categoryId: UUID, userId: UUID): Int { | ||
| override fun activateCategory(isActivate: Boolean, topic: String, userId: UUID): Int { | ||
| return saveOrUpdateSetting( | ||
| categoryId = categoryId, | ||
| isActivate = isActivate, | ||
| topic = topic, | ||
| userId = userId, | ||
| isActivate = true | ||
| ) |
There was a problem hiding this comment.
isActivate에 따라 활성화 또는 비활성화 시킬 수 있는 메소드니까 activateOrDeActivate 이런식으로 메소드명을 나타내주면 좋을 거 같습니다!
|
Kudos, SonarCloud Quality Gate passed!
|
No description provided.