-
Notifications
You must be signed in to change notification settings - Fork 3
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] feat: Club 생성 참여 탈퇴 API #207
Conversation
- 가장 빨리 들어온 사람이 방장이 된다. - 만약 방탈퇴 후 방에 남은 인원이 없으면 모임 삭제
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.
복잡한 도메인 맡느라 고생 많으셨어요!!! 코멘트 몇 가지 남겨 보았습니다.
.stream() | ||
.map(fieldError -> fieldError.getDefaultMessage()) | ||
.map(DefaultMessageSourceResolvable::getDefaultMessage) | ||
.toList(); |
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.
👍
@PostMapping | ||
public ResponseEntity<ApiResponse<SaveClubResponse>> save( | ||
@Auth Long memberId, | ||
@Valid @RequestBody SaveClubRequest request) { | ||
SaveClubResponse response = clubCommandService.save(memberId, request); | ||
return ResponseEntity.created(URI.create("/clubs" + response.id())).body(ApiResponse.ofSuccess(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.
@PostMapping | |
public ResponseEntity<ApiResponse<SaveClubResponse>> save( | |
@Auth Long memberId, | |
@Valid @RequestBody SaveClubRequest request) { | |
SaveClubResponse response = clubCommandService.save(memberId, request); | |
return ResponseEntity.created(URI.create("/clubs" + response.id())).body(ApiResponse.ofSuccess(response)); | |
} | |
@PostMapping | |
public ResponseEntity<ApiResponse<SaveClubResponse>> save( | |
@Auth Long memberId, | |
@Valid @RequestBody SaveClubRequest request | |
) { | |
SaveClubResponse response = clubCommandService.save(memberId, request); | |
return ResponseEntity.created(URI.create("/clubs" + response.id())).body(ApiResponse.ofSuccess(response)); | |
} |
개행~!~!
@Enumerated(EnumType.STRING) | ||
@Column(name = "status", nullable = false) | ||
private Status status; | ||
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | ||
List<ClubMember> clubMembers = new ArrayList<>(); | ||
|
||
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | ||
List<ClubPet> clubPets = new ArrayList<>(); |
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.
@Enumerated(EnumType.STRING) | |
@Column(name = "status", nullable = false) | |
private Status status; | |
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | |
List<ClubMember> clubMembers = new ArrayList<>(); | |
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | |
List<ClubPet> clubPets = new ArrayList<>(); | |
@Enumerated(EnumType.STRING) | |
@Column(name = "status", nullable = false) | |
private Status status; | |
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | |
List<ClubMember> clubMembers = new ArrayList<>(); | |
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | |
List<ClubPet> clubPets = new ArrayList<>(); |
여기도 개행이요
} | ||
|
||
|
||
private void validateAlreadyExists(Member newMember) { |
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.
} | |
private void validateAlreadyExists(Member newMember) { | |
} | |
private void validateAlreadyExists(Member newMember) { |
public static ClubMember create(Club club, Member member) { | ||
return ClubMember.builder() | ||
.club(club) | ||
.member(member) | ||
.createdAt(LocalDateTime.now()) | ||
.build(); | ||
} |
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.
정적 팩토리를 사용할 거라면 생성자를 private으로 막으면 어떨까요?
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.
전 정적 팩토리 메서드 제거해도 괜찮을 것 같아요.
assertThatCode(() -> clubCommandService.saveClubMember(savedClub.getId(), savedNewMember.getId(), request)) | ||
.doesNotThrowAnyException(); |
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.
좋은 생각인 것 같아요.
Member savedMember = createSavedMember(); | ||
Pet savedPet = createSavedPet(savedMember); | ||
Club savedClub = createSavedClub(savedMember, savedPet, Set.of(Gender.FEMALE, Gender.FEMALE_NEUTERED), | ||
Set.of(SizeType.SMALL)); |
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.
이 부분이 계속 반복되는 것 같은데 상위 클래스의 필드로 넣고 @BeforeEach
등으로 생성하는 것은 어떻게 생각하시나요?
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랑 pet은 같은 데이터 쓰고 할게여
private boolean canDelegate(ClubMember target) { | ||
return owner.getId().equals(target.getMember().getId()) && !isEmpty(); | ||
} |
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.
private boolean canDelegate(ClubMember target) { | |
return owner.getId().equals(target.getMember().getId()) && !isEmpty(); | |
} | |
private boolean canDelegate(Member member) { | |
return owner.getId().equals(member.getId()) && !isEmpty(); | |
} |
removeClubMember()
의 파라미터랑 직접 비교하면
(+ 나중에 equals and hashCode 구현되면)
덜 복잡할 것 같은데 어떠신가요??
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.
entity에 동등성을 부여하는 부분은 논의해야 할 부분이라고 생각해서 따로 구현하진 않았어요.(너무 귀찮긴 했습니다.ㅎㅎ)
participatingMemberPets.forEach(clubPet -> clubPet.updateClub(null)); | ||
|
||
} |
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.
participatingMemberPets.forEach(clubPet -> clubPet.updateClub(null)); | |
} | |
participatingMemberPets.forEach(clubPet -> clubPet.updateClub(null)); | |
} |
.map(Pet::getImageUrl) | ||
.toList(); | ||
|
||
newClub.addClubPet(participatingPets); |
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.
굿이에요~!
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.
연관관계 때문에 고생많이했습니다 위브~!
@@ -70,6 +76,11 @@ public class Club { | |||
@Enumerated(EnumType.STRING) | |||
@Column(name = "status", nullable = false) | |||
private Status status; | |||
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | |||
List<ClubMember> clubMembers = new ArrayList<>(); |
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.
클럽이랑 생명주기가 완전 같으니까 oneToMany적용해서 cascade할 수 있는게 장점이네요~!
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.
위브~ 고생 많으셨습니다~
전체적으로 Club에서 Member나 Pet과 직접 상호작용하지 않도록 변경해주시면 좋겠어요.
Club 코드가 훨씬 간결해질 것이라고 생각합니다.
추가된 코드 양이 많아서 일단 도메인 로직 위주로만 살펴봤어요.
리뷰 반영 후에 DM 주시면 추가로 확인하겠습니다!
public ResponseEntity<ApiResponse<SaveClubResponse>> save( | ||
@Auth Long memberId, | ||
@Valid @RequestBody SaveClubRequest request) { |
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.
public ResponseEntity<ApiResponse<SaveClubResponse>> save( | |
@Auth Long memberId, | |
@Valid @RequestBody SaveClubRequest request) { | |
public ResponseEntity<ApiResponse<SaveClubResponse>> save( | |
@Auth Long memberId, | |
@Valid @RequestBody SaveClubRequest request | |
) { |
private Status status; | ||
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) |
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.
private Status status; | |
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | |
private Status status; | |
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) |
|
||
private void validateParticipatePet(Pet pet) { | ||
if (!allowedGenders.contains(pet.getGender()) || !allowedSizes.contains(pet.getSizeType())) { | ||
throw new FriendoglyException("모임에 데려갈 수 없는 강아지가 있습니다."); |
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.
throw new FriendoglyException("모임에 데려갈 수 없는 강아지가 있습니다."); | |
throw new FriendoglyException("모임에 데려갈 수 있는 강아지가 없습니다."); |
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.
지금 보니깐 , 모임에 데려갈 수 없는 강아지가 요청에 포함되어 있을 경우라서 저게 맞는 것 같아요.
public static ClubMember create(Club club, Member member) { | ||
return ClubMember.builder() | ||
.club(club) | ||
.member(member) | ||
.createdAt(LocalDateTime.now()) | ||
.build(); | ||
} |
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.
전 정적 팩토리 메서드 제거해도 괜찮을 것 같아요.
public void addClubPet(List<Pet> pets) { | ||
pets.stream() | ||
.peek(this::validateParticipatePet) | ||
.map(pet -> new ClubPet(this, pet)) | ||
.peek(clubPet -> clubPet.updateClub(this)) | ||
.forEach(clubPets::add); | ||
} |
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.
Pet
-> ClubPet
변환 로직이 Club에 들어있는게 부자연스러운 듯 합니다.
Club은 중간 테이블인 ClubPet을 통해 Pet과 상호작용해야 하는데, 어떻게 보면 Pet과 직접적으로 상호작용 하고 있네요.
해당 변환 로직을 ClubPet으로 밀어넣고, Service 에서 변환로직 호출 후 넣어주는 편이 좋겠어요.
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.
Pet, Member 두 부분 모두 entity 동등성 부여하는걸 고려한 이후에 수정해도 될까요?
아직 고민이 더 필요한 것 같습니다.
중간 테이블 엔티티가 서비스 나와도 상관이 없을까? 변환 로직을 서비스에서 하는게 적절할까? 이런 고민이 아직 있네요.! 좀 더 얘기 나눠보고 수정해봐도 될까요?
public void addClubMember(Member newMember) { | ||
validateAlreadyExists(newMember); | ||
validateMemberCapacity(); | ||
ClubMember clubMember = ClubMember.create(this, newMember); | ||
clubMembers.add(clubMember); | ||
clubMember.updateClub(this); | ||
} |
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.
Pet -> ClubPet 변환 로직이 Club에 들어있는게 부자연스러운 듯 합니다.
Club은 중간 테이블인 ClubPet을 통해 Pet과 상호작용해야 하는데, 어떻게 보면 Pet과 직접적으로 상호작용 하고 있네요.
해당 변환 로직을 ClubPet으로 밀어넣고, Service 에서 변환로직 호출 후 넣어주는 편이 좋겠어요.
이쪽도 마찬가지 입니다. 변환 로직을 ClubMember로 넣어주면 책임을 나눌 수 있을 것 같습니다.
또 이렇게 변경하면 바로 아래에 있는 vlidateAlreadyExists()
메서드 내부에서 cotains()
를 사용할 수도 있게 되겠지요?
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.
엔티티에 동등성을 아직 부여하고 있지 않아서 contains 함수를 사용하는건 별개의 문제인 것 같아용
public SaveClubMemberResponse saveClubMember(Long clubId, Long memberId, SaveClubMemberRequest request) { | ||
Club club = clubRepository.findById(clubId) | ||
.orElseThrow(() -> new FriendoglyException("모임 정보를 찾지 못했습니다.")); |
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.
여기에 덧붙여서, 현재 saveClubMember()
메서드는 모임에 참여할 회원 뿐만 아니라, 모임에 참여할 강아지 정보도 추가하고 있습니다.
현재 네이밍이 Pet이 추가된다는 의미를 충분히 나타내지 못하는 것 같아요.
Club에 Pet이 추가되는 동작이 가려지지 않도록 네이밍을 고려해보시면 좋겠습니다.
public void removeClubMember(Member member) { | ||
ClubMember target = clubMembers.stream() | ||
.filter(e -> e.getMember().getId().equals(member.getId())) | ||
.findAny() | ||
.orElseThrow(() -> new FriendoglyException("참여 중인 모임이 아닙니다.")); |
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.
public void removeClubMember(ClubMember clubMember) {
if (!clubMembers.remove(clubMember)) {
throw new FriendoglyException("참여 중인 모임이 아닙니다.");
}
// ...
}
ClubMember를 인자로 받으면 로직을 더 간결하게 표현할 수 있겠습니다.
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.
엔티티에 동등성 부여하는게 가능하다면 저렇게 깔끔하게 해결할 수 있을 것 같아요.
저도 구현하면서 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.
나는 id에만 동등성 부여하는 것 동의합니다.
우리는 객체지향을 하고는 있지만, DB와 밀접하게 연결이 되어있기 때문에 DB와 같은 관점에서 바라봐야 인지비용이 줄어든다고 생각합니다~!
<1>
id: 1, name: 도선, gender: 남
id: 2, name: 도선, gender: 여
<2>
id: 1, name: 도선, gender: 남
id: 1, name: 도선, gender: 여
1번은 id값이 다르기에 충분히 다른 사람이지만,
2번은 id를 제외한 값은 충분히 가변적인 값이고 수정이 가능한 값이기에 다른 사람이라고 보기에는 적절하지 않다고 생각
그렇기 때문에 id에만 동등성에 부여하는 것을 동의합니다!
if (canDelegate(target)) { | ||
this.owner = clubMembers.get(0).getMember(); | ||
} |
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.
모임에서 탈퇴하려는 회원이 모임장인 경우 모임장의 역할을 위임하기 위한 메서드로 보입니다.
이 코드만 보았을 때에는 의도를 파악하기 힘든 것 같아요. 차라리 메서드를 나눠주는게 가독성이 좋겠습니다.
if (isOwner(clubMember)) {
delegateOwnerToNextMember();
}
private void delegateOwnerToNextMember() {
if (!clubMembers.isEmpty()) {
this.owner = clubMembers.get(0).getMember();
}
}
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.
너무 과한 메소드 분리라고 생각해서 저 정도로 그쳤지만, 가독성이 떨어진다고 느껴진다면 좋은 것 같습니다.!!
메소드명의 경우 저희 컨벤션에도 알맞고 이해하기도 무리가 없을 것 같아서 updateOwner로 해봤는데 어떨까요?
.filter(clubPet -> clubPet.getPet().getMember().getId().equals(member.getId())) | ||
.toList(); | ||
clubPets.removeAll(participatingMemberPets); | ||
participatingMemberPets.forEach(clubPet -> clubPet.updateClub(null)); |
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.
clubPet.updateClub(null);
이렇게 하면 해당 row가 지워지나요? 서비스에서 해당 row에 대한 delete 쿼리를 날려줘야할 것 같습니다.
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.
간단하게 코멘트 남겨봤어요 이번주도 화이팅입니다~~~~
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | ||
List<ClubMember> clubMembers = new ArrayList<>(); | ||
|
||
@OneToMany(mappedBy = "club", orphanRemoval = true, cascade = CascadeType.ALL) | ||
List<ClubPet> clubPets = new ArrayList<>(); | ||
|
||
@Builder | ||
public Club( | ||
private Club( |
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.
굿
if (isOwner(target)) { | ||
updateOwner(); |
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.
여기 가독성 훨씬 나아졌네요
.map(Pet::getImageUrl) | ||
.toList(); | ||
|
||
newClub.addClubPet(participatingPets); |
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.
굿이에요~!
private Member savedMember; | ||
private Pet savedPet; |
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.
픽스처 좋아요
@BeforeEach | ||
void setMemberAndPet() { | ||
savedMember = createSavedMember(); | ||
savedPet = createSavedPet(savedMember); | ||
} |
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.
이건 상위 클래스에 있어도 될것 같기도 하고 ...
Club savedClub = createSavedClub( | ||
savedMember, | ||
savedPet, | ||
Set.of(Gender.FEMALE, Gender.FEMALE_NEUTERED), | ||
Set.of(SizeType.SMALL) | ||
); |
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.
개행 좋네요
public void removeClubMember(Member member) { | ||
ClubMember target = clubMembers.stream() | ||
.filter(e -> e.getMember().getId().equals(member.getId())) | ||
.findAny() | ||
.orElseThrow(() -> new FriendoglyException("참여 중인 모임이 아닙니다.")); |
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.
나는 id에만 동등성 부여하는 것 동의합니다.
우리는 객체지향을 하고는 있지만, DB와 밀접하게 연결이 되어있기 때문에 DB와 같은 관점에서 바라봐야 인지비용이 줄어든다고 생각합니다~!
<1>
id: 1, name: 도선, gender: 남
id: 2, name: 도선, gender: 여
<2>
id: 1, name: 도선, gender: 남
id: 1, name: 도선, gender: 여
1번은 id값이 다르기에 충분히 다른 사람이지만,
2번은 id를 제외한 값은 충분히 가변적인 값이고 수정이 가능한 값이기에 다른 사람이라고 보기에는 적절하지 않다고 생각
그렇기 때문에 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.
Club의 메서드에서 Member나 Pet 대신 ClubMember나 ClubPet을 인자로 받도록 변경된 코드를 보고싶었는데, 이에 대한 언급이 없으셔서 다시 한 번 RC 보냅니다~
이슈
개발 사항
리뷰 요청 사항 (없으면 삭제해 주세요)
전달 사항 (없으면 삭제해 주세요)