-
Notifications
You must be signed in to change notification settings - Fork 8
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] 로그 중복 작성 제거, 비즈니스 로직 내 로그 추가(#608) #610
Changes from 4 commits
20f2324
96eda7b
81d6168
70aab4c
c9f508d
7d00764
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,12 +8,14 @@ | |||||
import corea.member.domain.Member; | ||||||
import corea.member.repository.MemberRepository; | ||||||
import lombok.RequiredArgsConstructor; | ||||||
import lombok.extern.slf4j.Slf4j; | ||||||
import org.springframework.stereotype.Service; | ||||||
import org.springframework.transaction.annotation.Transactional; | ||||||
|
||||||
import static corea.exception.ExceptionType.INVALID_TOKEN; | ||||||
import static corea.exception.ExceptionType.TOKEN_EXPIRED; | ||||||
|
||||||
@Slf4j | ||||||
@Service | ||||||
@RequiredArgsConstructor | ||||||
@Transactional(readOnly = true) | ||||||
|
@@ -35,7 +37,12 @@ public TokenInfo login(GithubUserInfo userInfo) { | |||||
} | ||||||
|
||||||
private Member register(GithubUserInfo userInfo) { | ||||||
return memberRepository.save(new Member(userInfo.login(), userInfo.avatarUrl(), userInfo.name(), userInfo.email(), true, userInfo.id())); | ||||||
Member member = memberRepository.save(new Member(userInfo.login(), userInfo.avatarUrl(), userInfo.name(), userInfo.email(), true, userInfo.id())); | ||||||
createLog(member); | ||||||
return member; | ||||||
} | ||||||
private static void createLog(Member member){ | ||||||
log.info("멤버를 생성했습니다. 멤버 id={}, 멤버 이름={},깃허브 id={}, 닉네임={}",member.getId(),member.getName(),member.getGithubUserId(),member.getUsername()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
private String extendAuthorization(Member member) { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ public class DevelopFeedbackService { | |
@Transactional | ||
public DevelopFeedbackResponse create(long roomId, long deliverId, DevelopFeedbackRequest request) { | ||
validateAlreadyExist(roomId, deliverId, request.receiverId()); | ||
log.debug("개발 피드백 작성[작성자({}), 요청값({})", deliverId, request); | ||
log.info("개발 피드백 작성[작성자({}), 요청값({})", deliverId, request); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
MatchResult matchResult = matchResultRepository.findByRoomIdAndReviewerIdAndRevieweeId(roomId, deliverId, request.receiverId()) | ||
.orElseThrow(() -> new CoreaException(ExceptionType.NOT_MATCHED_MEMBER)); | ||
|
@@ -50,7 +50,7 @@ private DevelopFeedback saveDevelopFeedback(long roomId, DevelopFeedbackRequest | |
|
||
@Transactional | ||
public DevelopFeedbackResponse update(long feedbackId, long deliverId, DevelopFeedbackRequest request) { | ||
log.debug("개발 피드백 업데이트[작성자({}), 피드백 ID({}), 요청값({})", deliverId, feedbackId, request); | ||
log.info("개발 피드백 업데이트[작성자({}), 피드백 ID({}), 요청값({})", deliverId, feedbackId, request); | ||
|
||
DevelopFeedback feedback = developFeedbackRepository.findById(feedbackId) | ||
.orElseThrow(() -> new CoreaException(ExceptionType.FEEDBACK_NOT_FOUND)); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,10 +3,12 @@ | |||||
import corea.exception.CoreaException; | ||||||
import corea.exception.ExceptionType; | ||||||
import corea.participation.domain.Participation; | ||||||
import lombok.extern.slf4j.Slf4j; | ||||||
|
||||||
import java.util.ArrayList; | ||||||
import java.util.List; | ||||||
|
||||||
@Slf4j | ||||||
public class ParticipationFilter { | ||||||
|
||||||
private final List<Participation> participations; | ||||||
|
@@ -51,6 +53,7 @@ private List<Participation> findPRSubmittedParticipation(PullRequestInfo pullReq | |||||
|
||||||
private void invalidateIfNotSubmitPR(PullRequestInfo pullRequestInfo, Participation participation) { | ||||||
if (!hasSubmittedPR(pullRequestInfo, participation)) { | ||||||
log.warn("매칭에 실패 했습니다. 방 id={},사용자 id={},사용자 깃허브 닉네임={}",participation.getRoomsId(),participation.getMembersId(),participation.getMember().getUsername()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
participation.invalidate(); | ||||||
} | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ public List<MatchResult> match(long roomId, PullRequestInfo pullRequestInfo) { | |
room.updateStatusToProgress(); | ||
|
||
List<MatchResult> matchResults = matchPairs(room, pullRequestInfo); | ||
logMatchResults(roomId,matchResults); | ||
return matchResultRepository.saveAll(matchResults); | ||
} | ||
|
||
|
@@ -54,6 +55,19 @@ private List<MatchResult> matchPairs(Room room, PullRequestInfo pullRequestInfo) | |
.toList(); | ||
} | ||
|
||
private void logMatchResults(long roomId, List<MatchResult> matchResults) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 위에 createLog() 와 동일한 리뷰입니다! |
||
StringBuilder logMessage = new StringBuilder(); | ||
logMessage.append("매칭 결과 [방 번호: ").append(roomId).append("]\n"); | ||
|
||
matchResults.forEach(result -> | ||
logMessage.append("리뷰어: ").append(result.getReviewer().getUsername()) | ||
.append(", 리뷰이: ").append(result.getReviewee().getUsername()) | ||
.append(", PR 링크: ").append(result.getPrLink()).append("\n") | ||
); | ||
|
||
log.info(logMessage.toString()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 현재 모든 MatchResult에 대해 리뷰어 리뷰이 pr링크를 순회하며 로그를 찍고 있는데, 매번 조회 쿼리가 나갈 것이라 예상이 됩니다. 그리고 저런 정보들이 유의미할까도 고민이 되는데 roomId에 대한 매칭 성공 로그 정도만 남겨보는건 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. public static MatchResult of(long roomId, Pair pair, String prLink) {
return new MatchResult(roomId, pair.getDeliver(), pair.getReceiver(), prLink);
} 여기서 찍는게 맞는지에 대한 고민은 들었는데 빼도 크게 상관없을듯요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
로그 위치는 저도 크게 상관없다 느껴지네요~! 저 부분도 괜찮은것같아요! |
||
} | ||
|
||
private List<Participation> findPRSubmittedParticipation(Room room, PullRequestInfo pullRequestInfo) { | ||
List<Participation> participations = participationRepository.findAllByRoom(room) | ||
.stream() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
import corea.exception.CoreaException; | ||
import corea.exception.ExceptionType; | ||
import corea.participation.domain.ParticipationStatus; | ||
|
||
public enum MemberRole { | ||
|
||
|
@@ -22,4 +23,11 @@ public static MemberRole from(String role) { | |
public boolean isReviewer() { | ||
return this == REVIEWER; | ||
} | ||
|
||
public ParticipationStatus getParticipationStatus() { | ||
return switch (this) { | ||
case REVIEWER,REVIEWEE,BOTH -> ParticipationStatus.PARTICIPATED; | ||
case NONE -> ParticipationStatus.NOT_PARTICIPATED; | ||
}; | ||
} | ||
Comment on lines
+27
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. break 없이 잘 동작하는가 싶었는데, 람다라 괜찮군요. 구두로 물어봤으니 해결했습니당~ |
||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -40,14 +40,16 @@ public class Participation extends BaseTimeEntity { | |||||
|
||||||
private int matchingSize; | ||||||
|
||||||
public Participation(Room room, Member member, MemberRole memberRole, ParticipationStatus status, int matchingSize) { | ||||||
this(null,room,member,memberRole,status,matchingSize); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 근데 이건 좀 심했다 |
||||||
} | ||||||
|
||||||
public Participation(Room room, Member member, MemberRole role, int matchingSize) { | ||||||
this(null, room, member, role, ParticipationStatus.PARTICIPATED, matchingSize); | ||||||
debug(room.getId(), member.getId()); | ||||||
} | ||||||
|
||||||
public Participation(Room room, Member member) { | ||||||
this(null, room, member, MemberRole.REVIEWER, ParticipationStatus.MANAGER, room.getMatchingSize()); | ||||||
debug(room.getId(), member.getId()); | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 전 이래서 편의성을 사용하는 ( REVIEWER , MANAGER 등을 미리 지정하는 ) |
||||||
|
||||||
public boolean isNotMatchingMemberId(long memberId) { | ||||||
|
@@ -90,7 +92,4 @@ public String getMemberGithubId() { | |||||
return member.getGithubUserId(); | ||||||
} | ||||||
|
||||||
private static void debug(long roomId, long memberId) { | ||||||
log.debug("참가자 생성[방 ID={}, 멤버 ID={}", roomId, memberId); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -21,6 +21,7 @@ | |||||
public class ParticipationService { | ||||||
|
||||||
private final ParticipationRepository participationRepository; | ||||||
private final ParticipationWriter participationWriter; | ||||||
private final RoomRepository roomRepository; | ||||||
private final MemberRepository memberRepository; | ||||||
|
||||||
|
@@ -34,23 +35,16 @@ private Participation saveParticipation(ParticipationRequest request) { | |||||
Member member = memberRepository.findById(request.memberId()) | ||||||
.orElseThrow(() -> new CoreaException(ExceptionType.MEMBER_NOT_FOUND)); | ||||||
MemberRole memberRole = MemberRole.from(request.role()); | ||||||
Room room = getRoom(request.roomId()); | ||||||
|
||||||
Participation participation = new Participation(getRoom(request.roomId()), member, memberRole, request.matchingSize()); | ||||||
participation.participate(); | ||||||
Participation participation = participationWriter.create(room,member, memberRole,request.matchingSize()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return participationRepository.save(participation); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. participationRepo.save() 두번 일어나요~! |
||||||
} | ||||||
|
||||||
@Transactional | ||||||
public void cancel(long roomId, long memberId) { | ||||||
validateMemberExist(memberId); | ||||||
deleteParticipation(roomId, memberId); | ||||||
} | ||||||
|
||||||
private void deleteParticipation(long roomId, long memberId) { | ||||||
Participation participation = participationRepository.findByRoomIdAndMemberId(roomId, memberId) | ||||||
.orElseThrow(() -> new CoreaException(ExceptionType.NOT_ALREADY_APPLY)); | ||||||
participation.cancel(); | ||||||
participationRepository.delete(participation); | ||||||
participationWriter.delete(roomId, memberId); | ||||||
} | ||||||
|
||||||
private void validateIdExist(long roomId, long memberId) { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,57 @@ | ||||||
package corea.participation.service; | ||||||
|
||||||
import corea.exception.CoreaException; | ||||||
import corea.exception.ExceptionType; | ||||||
import corea.member.domain.Member; | ||||||
import corea.member.domain.MemberRole; | ||||||
import corea.participation.domain.Participation; | ||||||
import corea.participation.domain.ParticipationStatus; | ||||||
import corea.participation.repository.ParticipationRepository; | ||||||
import corea.room.domain.Room; | ||||||
import lombok.RequiredArgsConstructor; | ||||||
import lombok.extern.slf4j.Slf4j; | ||||||
import org.springframework.stereotype.Component; | ||||||
import org.springframework.transaction.annotation.Transactional; | ||||||
|
||||||
@Slf4j | ||||||
@Component | ||||||
@RequiredArgsConstructor | ||||||
@Transactional | ||||||
public class ParticipationWriter { | ||||||
|
||||||
private final ParticipationRepository participationRepository; | ||||||
|
||||||
public Participation create(Room room, Member member, MemberRole memberRole,ParticipationStatus participationStatus) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
return create(room, member, memberRole,participationStatus, room.getMatchingSize()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
public Participation create(Room room, Member member, MemberRole memberRole, int matchingSize) { | ||||||
return create(room, member, memberRole,memberRole.getParticipationStatus(),matchingSize); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
private Participation create(Room room, Member member, MemberRole memberRole, ParticipationStatus participationStatus, int matchingSize) { | ||||||
Participation participation = participationRepository.save(new Participation(room, member, memberRole, participationStatus, matchingSize)); | ||||||
participation.participate(); | ||||||
createLog(participation); | ||||||
return participation; | ||||||
} | ||||||
|
||||||
// TODO 객체 두개를 넣어서 삭제하는 방향으로 변경 해야합니다. | ||||||
// 현재, 로직상 cancel 호출 시, 의도하지 않는 room 조회문 발생 | ||||||
public void delete(long roomId, long memberId) { | ||||||
Participation participation = participationRepository.findByRoomIdAndMemberId(roomId, memberId) | ||||||
.orElseThrow(() -> new CoreaException(ExceptionType.NOT_ALREADY_APPLY)); | ||||||
participation.cancel(); | ||||||
deleteLog(participation); | ||||||
participationRepository.delete(participation); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. ROOM 조회문을 밖에서 해서 넘겨주는 식이 될 거 같아용. 현재 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
이해 완👍 좋습니당~ |
||||||
} | ||||||
|
||||||
private static void createLog(Participation participation) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기도 마찬가지입니당! 네이밍 + static |
||||||
log.info("방에 참가했습니다. id={}, 방 id={}, 참가한 사용자 id={}, 역할={}, 원하는 매칭 인원={}", participation.getId(), participation.getRoomsId(), participation.getMembersId(), participation.getMemberRole(), participation.getMatchingSize()); | ||||||
} | ||||||
|
||||||
private static void deleteLog(Participation participation) { | ||||||
log.info("참여를 취소했습니다. 방 id={}, 참가한 사용자 id={}", participation.getRoomsId(), participation.getMembersId()); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,9 +10,8 @@ | |||||
import corea.participation.domain.Participation; | ||||||
import corea.participation.domain.ParticipationStatus; | ||||||
import corea.participation.repository.ParticipationRepository; | ||||||
import corea.participation.service.ParticipationWriter; | ||||||
import corea.room.domain.Room; | ||||||
import corea.room.domain.RoomClassification; | ||||||
import corea.room.domain.RoomStatus; | ||||||
import corea.room.dto.*; | ||||||
import corea.room.repository.RoomRepository; | ||||||
import corea.scheduler.domain.AutomaticMatching; | ||||||
|
@@ -21,8 +20,6 @@ | |||||
import corea.scheduler.repository.AutomaticUpdateRepository; | ||||||
import lombok.RequiredArgsConstructor; | ||||||
import lombok.extern.slf4j.Slf4j; | ||||||
import org.springframework.data.domain.Page; | ||||||
import org.springframework.data.domain.PageRequest; | ||||||
import org.springframework.stereotype.Service; | ||||||
import org.springframework.transaction.annotation.Transactional; | ||||||
|
||||||
|
@@ -48,6 +45,7 @@ public class RoomService { | |||||
private final FailedMatchingRepository failedMatchingRepository; | ||||||
private final AutomaticMatchingRepository automaticMatchingRepository; | ||||||
private final AutomaticUpdateRepository automaticUpdateRepository; | ||||||
private final ParticipationWriter participationWriter; | ||||||
|
||||||
@Transactional | ||||||
public RoomResponse create(long memberId, RoomCreateRequest request) { | ||||||
|
@@ -56,9 +54,10 @@ public RoomResponse create(long memberId, RoomCreateRequest request) { | |||||
Member manager = memberRepository.findById(memberId) | ||||||
.orElseThrow(() -> new CoreaException(ExceptionType.MEMBER_NOT_FOUND)); | ||||||
Room room = roomRepository.save(request.toEntity(manager)); | ||||||
Participation participation = new Participation(room, manager); | ||||||
log.info("방을 생성했습니다. 방 생성자 id={}, 요청한 사용자 id={}", room.getManagerId(), memberId); | ||||||
|
||||||
Participation participation = participationWriter.create(room,manager,MemberRole.REVIEWER,ParticipationStatus.MANAGER); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
participationRepository.save(participation); | ||||||
automaticMatchingRepository.save(new AutomaticMatching(room.getId(), request.recruitmentDeadline())); | ||||||
automaticUpdateRepository.save(new AutomaticUpdate(room.getId(), request.reviewDeadline())); | ||||||
|
||||||
|
@@ -112,6 +111,7 @@ public void delete(long roomId, long memberId) { | |||||
Room room = getRoom(roomId); | ||||||
validateDeletionAuthority(room, memberId); | ||||||
|
||||||
log.info("방을 삭제했습니다. 방 id={}, 사용자 iD={}", roomId, memberId); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 변경된 부분이 아니여서 이곳에 커멘트 남길게요. RoomService에서 ParticipationWriter를 잘 사용하고 있는가에 대한 커멘트입니다. 개인적으로 리팩터링으로 수고하고 있다는거 잘 알지만 👍, 약간 우려된 부분이 ParticipationWriter에서 나온 것 같아 글 남겨봐요! 현재 Room과 Participation이 저희 서비스상 많은 연관관계를 가지지만 방을 삭제할 때, 막상 RoomService에서는 ParticipationWriter의 delete 메서드를 활용하기 어려운 구조라 생각해요. 제가 느낄 수 있는 한가지 ParticipationWriter의 장점으로는 방을 참여할 때, participation.participate();를 같이 호출해주어 개발자로 하여금 휴먼 에러를 방지한다?? 정도가 있겠네요. 근데 이 장점도 저희의 처음 코드였던 Room을 생성할 때 초기 참여자 수를 1로 하고, 저장한다면 똑같이 해결되고 오히려 지금 코드보다 깔끔해진다 느껴집니다. 전에 조이썬한테 제가 먼저 이러한 구조로 바꿔보는거 좋을 것 같다고 동영상을 추천해줬던 것 같아요. 근데 나중에 다시 이 방식을 도입한 사람들에게 의견을 들었을 때, 큰 장점을 못 느꼈다는 말도 들었고, 솔직히 말하자면 ParitipationWriter는 필요해서 나온 느낌이 아니라 그냥 써보고 싶어서 만든 느낌이랄까요?? 다른 영역에서 자주 사용되는 Room이나 Member 같은 경우는 다음과 같이 사용하면 의미가 있다 느껴지네요~ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
사실, 해당 부분이 Writer 가 분리되는 이유라고 생각해요.
해당 내용도, 엔티티 라면 비즈니스 변화에 의존되지 않게 코드를 사용해야 한다고 생각해요 ( 저는 이런 관점에서 Reader / Writer 의 필요성을 명확하게 느낀거 같아요. 추가로, 지금 당장은 그냥 로직을 넣은게 다지만, ( 사실 이거 하나로도 만족스럽긴 해요 전 - 에러 일괄적 관리 + 로그 관리 )
해당 부분 역시도 DB 의 원자성과 비슷하다고 생각해요. 이들은 물론 다 제 의견이니 더 생각 달아줘도 좋습니당 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
ParticipationWriter에 대한 조이썬 의견 궁금했는데, 좋습니다~! 팀 안에서도 로그는 엔티티 내부가 아닌 Writer, Service로 한정하도록 컨벤션 정해봐도 좋을듯 하네요!
저도 이 부분 동의합니다! 저도 앞으로 리팩토링 할때는 최대한 Service가 Repo를 의존하지 않는 방향으로 같이 리팩터링 해보겠습니다~ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
가볍게 추가만 하자면 저도 이 말에 동의하는 것 같습니다~ 이번에 기본 참여자를 1로 설정한다는 건 물론 지금 단계에서는 깔끔한 코드처럼 보이고 실제로 깔끔한 코드를 작성할 수 있게 해주지만, 그런 관점에서 전체적으로 이런 방향의 리팩터링이 이루어지고 전체 코드의 구조가 일관적이 된다고 하면 더할 나위 없겠네요. 그런데 조금만 덧붙이자면... 이런 리팩터링이 이루어졌을 때 이전에 테스트하던 메서드가 주석 처리 되는 경우가 종종 있는 것 같아요. 그 때는 가능하다면 이 테스트가 주석 처리된 이유도 함께 적어주셨으면 좋겠습니다. 이번에 베타 테스트로 QA하면서도 느꼈지만 테스트가 잘 작성되었다면 사전에 확인할 수 있었을 것 같은 오류가 꽤 많더라고요. 그런 관점에서 보았을 때 리팩터링 이후 이전 테스트 로직이 비활성화되는 건 조금 의문스럽게 느껴지는 것 같아요. (그렇다면 이 로직이 정확히 이전과 똑같이 동작하는지는 뭘 보고 확신할 수 있지?) 리팩터링은 기본적으로 '이전의 로직과 동일한 동작을 하지만 로직만 교체되는 일'인 만큼 "이전에 테스트하던 걸 왜 이제는 테스트하지 않아도 되는 지"에 대해 코드를 보는 사람도 한 눈에 알 수 있으면 바뀐 이후에도 오류가 나지 않을 수 있을 거라는 확신이 더 강해질 것 같기 때문입니다~~! 이번에 생성자 관련해서 남긴 리뷰도 같은 결이었던 것 같아요! 리팩터링은 정말 너무너무 귀찮고 힘든 일인 걸 알고 있기 때문에 다들 고생하고 계신 거 너무 잘 알고 있습니다~~ 만! 요약: 리팩터링할 때 테스트도 신경 쓰자 (작성자도 리뷰어도 다같이) (뭐라하는 거 아님!!) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 말 안했던 건 제 잘못인거 같네용.🥲🥲 주석한 사유는 그전 PR 에 남겼으므로 생략하겠습니다. 🫡 |
||||||
roomRepository.delete(room); | ||||||
participationRepository.deleteAllByRoomId(roomId); | ||||||
automaticMatchingRepository.deleteByRoomId(roomId); | ||||||
|
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.
static 인 이유가 있나요? 아니면 아니여도 괜찮을 것 같아요!
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.
아래 logMatchResult() 메서드와 컨벤션이 맞으면 좋겠어요!
...log 혹은 log... 로 통일 가능한가요?
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.
클린 코드 적으로는 여러 인스턴스 가 각각 가질 메소드 가 아니라서 static 으로 분리 했는데
성능적으로 생각하면 어차피 싱글톤 + static 메소드 메모리를 찾아야 해서 ( 객체 내 같이 메소드가 위치하는게 아니므로 )
더 비효율적 일거 같네요 반영 할게요 🙂