-
Notifications
You must be signed in to change notification settings - Fork 15
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
[BE REFACTOR: ItemHistory에 amount 컬럼 추가#1695 #1696
Conversation
userIds.forEach(userId -> { | ||
userCommandService.updateCoinAmount(userId, amount); | ||
}); |
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.
userId 개수당 N회의 쿼리일 것 같은데 in 절을 이용해서 updateMany로 처리하시는 건 어떤가요??
라고 쓰고보니 JPA여서 commit할 때 모아서 알아서 해줬던가..
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.
update는 모아서 처리해주지 않습니다. 전부 동일한 상태로의 변경이라면 @Modifying 애노테이션을 활용해서 bulk update를 수행하는게 좋을 것 같습니다~
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.
고생하셨습니다 👍
userIds.forEach(userId -> { | ||
userCommandService.updateCoinAmount(userId, amount); | ||
}); |
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.
update는 모아서 처리해주지 않습니다. 전부 동일한 상태로의 변경이라면 @Modifying 애노테이션을 활용해서 bulk update를 수행하는게 좋을 것 같습니다~
@@ -96,6 +105,15 @@ public static ItemHistory of(long userId, long itemId, LocalDateTime usedAt) { | |||
return itemHistory; | |||
} | |||
|
|||
public static ItemHistory coin(Long userId, Long itemId, LocalDateTime assignedAt, |
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.
위의 생성자 팩토리 메서드의 메서드명으로 coin만 적어둔다면, 메서드명만 보고 생성자 팩토리 메서드일 것이다라고 유추하기는 힘들어 보입니다 .createHistoryWithCoin 등 조금 더 직관적인 이름이 좋을 것 같습니다
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.
혹시 제가 작성한 이슈에 대해 어떻게 생각하시나요?
일단은 amount 필드를 추가하는 방식으로 진행했지만 더 좋은 아이디어가 있을까 하고 생성자 부분은 모두 임시적으로 만들어둔거라서요..!
amount를 추가하는걸로 확정이 된다면 생성자 관련 처리나 sku 통합(기존 관리자 지급 코인 100, 200으로 명시해둔 부분)을 진행하려 합니다!
Long amount) { | ||
ItemHistory itemHistory = new ItemHistory(userId, itemId, assignedAt, amount); | ||
if (!itemHistory.isValid()) { | ||
throw ExceptionStatus.INVALID_ARGUMENT.asControllerException(); |
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.
ControllerException이 아닌 DomainException이 적합해 보입니다.
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.
어흑 마이깟 오류 발견 감사합니다
@@ -96,6 +105,15 @@ public static ItemHistory of(long userId, long itemId, LocalDateTime usedAt) { | |||
return itemHistory; | |||
} |
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.
amount 필드가 추가 되었음에 따라 기존의 amount를 파라미터로 받지 않는 생성자의 경우 해당 값에 대한 처리를 해주는 것이 좋아보입니다. 해당 내용을 모르고 이 팩토리 메서드를 사용한다면, 참조 타입이기 때문에 amount에 null이 저장되어 런타임에 DB에 저장이 안되는 에러가 발생할 것 같습니다
…/innovationacademy-kr/Cabi into be/dev/refactor-ItemHistory#1695
…/innovationacademy-kr/Cabi into be/dev/refactor-ItemHistory#1695
해당 사항 (중복 선택)
설명
#1695
반드시 위 이슈를 읽어주십셔 ItemHistory에 amount 컬럼이 추가되는게 최선일까여?????
더 좋은 아이디어가 있다면 편하게 말씀해주세여!!!!