-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FEAT] 뱃지 작업 #143
base: develop
Are you sure you want to change the base?
[FEAT] 뱃지 작업 #143
Conversation
void save(Badge badge); | ||
Optional<Badge> findByName(String name); | ||
void register(Badge badge); |
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.
save 와 register 같은 기능을 하는 것 같은데 왜 두개를 만드셨나요?
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.
void save(Badge badge)
- 뱃지 그 자체를 저장합니다.
- 생성한 Badge를 BadgeEntity에 저장한다고 보면 좋을 것 같습니다.
void regeister(Badge badge)
- 뱃지를 등록(사용자에게 부여)해주는 기능입니다.
- BadgeMemberEntity에 Badge와 Member간의 관계를 저장합니다.
차이점은
save는 뱃지 생성 시점에 저장되므로 도메인모델에 뱃지에 관한 정보밖에 없을 것이고
register는 뱃지 도메인이 가지고 있는 owners 멤버필드를 활용합니다.
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.
혹은 도메인
float hue = random.nextFloat(); | ||
float saturation = 0.5f + (random.nextFloat() * 0.3f); | ||
float brightness = 0.8f + (random.nextFloat() * 0.2f); | ||
|
||
Color color = Color.getHSBColor(hue, saturation, brightness); |
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.
랜덤으로 만드는건 임시적인 요구사항이고, 추후에 사용자가 색상을 선택하여 만들 수 있도록 로직을 수정할텐데 그러면 create가 색상 코드를 받게 되겠죠?
그러므로 밸류 객체가 아닌 응용에 둬야 하지 않나 싶지만 굳이 고칠 필요는 없을듯
@Override | ||
public void register(Badge badge) { | ||
badgeMemberRepo.save(new BadgeMemberEntity(badge.getOwners().get(0).id(), converter.from(badge))); | ||
} |
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.
음 구현을 보내 Badge 자체를 추가하는 게 아니라, 사용자한테 Badge 를 달아주는 거네요
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.
고생하셨어요 코멘트 확인 부탁드립니다
float hue = random.nextFloat(); | ||
float saturation = 0.5f + (random.nextFloat() * 0.3f); | ||
float brightness = 0.8f + (random.nextFloat() * 0.2f); | ||
|
||
Color color = Color.getHSBColor(hue, saturation, brightness); |
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.
랜덤으로 만드는건 임시적인 요구사항이고, 추후에 사용자가 색상을 선택하여 만들 수 있도록 로직을 수정할텐데 그러면 create가 색상 코드를 받게 되겠죠?
그러므로 밸류 객체가 아닌 응용에 둬야 하지 않나 싶지만 굳이 고칠 필요는 없을듯
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.
Member와 Badge는 M:N입니다
마치 Product와 Category 같은 느낌이죠
연관을 맺기 위해서 Product가 Set를 가질 수도 있고 Category가 Set를 가질 수도 있습니다.
(혹은 서로 가질수도 있지만 한 쪽이 추가/삭제되면 다른 한 쪽도 이에 맞게 업데이트해줘야 하기 때문에 복잡도가 올라가고, 성능이 떨어지며, 트랜잭션 경계가 흐려질겁니다)
이 상황에서 Product를 가져올 때 Category들을 가져오는게 나을까요, Category를 가져올 때 Product들을 가져오는게 나을까요?
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.
정말 thanks
이거 하겠음
현재 후즈인 프로젝트 관점에 보았을 때 뱃지입장에서는 자신을 가진 사람들이 별로 중요하지 않고 누가 어떤 뱃지들을 가지고 있는지가 중요할 것 같음
(Product를 가져올 때 Category들을 가져오는 게 나을 것 같다는 말)
그리고 애초에 Category마다 Product를 저장하는 효율적이지 않은 것 같음
그래서 Badge도메인 모델에서 List owner를 없애고 멤버도메인 모델에 List badges를 추가해보고자 함
private final EventBus eventBus; | ||
|
||
@Transactional | ||
@EventListener |
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.
음 멤버엔티티가 생성되었다는 MemberCreated event가 발생하지 않아서 저 로직은 실행되지 않을 것 같습니다..
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.
@Transactional
public void handle(Command cmd) {
repository.save(obj);
eventBus.publish(obj.pullDomainEvents());
}
트랜잭션 커밋과 이벤트 발행 중 뭐가 먼저 일어나나요?
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 org.springframework.context.event.EventListener; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Handler |
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.
수정하겠습니다.
@Getter | ||
@NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
public class BadgeMemberEntity extends BaseEntity { | ||
@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.
memberId와 badgeId로 복합키를 구성할 수는 없을까요
다른 관계에서 BadgeMemberEntity 자체를 참조할 일은 거의 없을거 같아서 대리키를 만들 필요가 없을거 같습니다
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.
반영하겠습니다.
- 회원가입시 발생하는 멤버 엔티티가 생성되었다는 이벤트를 받음 - 이벤트로부터 회원의 정보를 얻고 회원의 분야에 대한 뱃지를 찾아 등록해줌
- Badge도메인모델에서 List<MemberId> owners를 없애고 - Member도메인모델에 List<BadgeId> badges를 추가
- 뱃지의 상태와 함께 뱃지아이디 저장 - 뱃지 상태 변경 - 뱃지 상태 조회
- 하나의 애그리거트에 대해서 도메인레포지토리를 가짐 - 도메인을 다시 저장할 때 1차 캐시에 의해 save&update됨 - 그래서 Member의 save기능을 사용 - addBadge와 관련된 기능(회원가입시분야뱃지등록, 뱃지달기) 수정
- 그에 따른 컨버팅 수정
📌 관련 이슈
close #116, #117, #121, #172
✨ PR 내용
🤓 리뷰어에게
다 함!!
(트랜잭션이벤트리스너예외는 디스코드로 발송되는 거 보고 우리가 처리해주기로 함)