Skip to content
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

Merged
merged 6 commits into from
Oct 16, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Oct 15, 2024

📌 관련 이슈

✨ PR 세부 내용

현재, 중요하다고 생각한 부분들에 대해서는 로그를 추가했습니다. (Post Request 만 )
중간에 refacotring 은 본능이 이끌어서 같이 했어요. 죄송합니당 😭

얘기해봐야 할거는 LoginArgumentResolver 에서 띄우는 로그가 의미있는가 싶어서 제거에 대해서만 얘기하면 될듯요.

@github-actions github-actions bot added BE 백엔드 개발 관련 작업 인프라 인프라 관련 작업 labels Oct 15, 2024
@youngsu5582 youngsu5582 added the 리팩터링 리팩터링 작업 label Oct 15, 2024
Copy link
Contributor Author

github-actions bot commented Oct 15, 2024

Test Results

 55 files   55 suites   7s ⏱️
163 tests 157 ✅ 6 💤 0 ❌
170 runs  164 ✅ 6 💤 0 ❌

Results for commit 7d00764.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@hjk0761 hjk0761 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로깅 추가 좋은 것 같습니다 ㅎㅎ
이후 리팩터링 방향성이 reader, writer 분리이니 리팩터링은 넘어갈께요!

createLog(member);
return member;
}
private static void createLog(Member member){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static 인 이유가 있나요? 아니면 아니여도 괜찮을 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아래 logMatchResult() 메서드와 컨벤션이 맞으면 좋겠어요!
...log 혹은 log... 로 통일 가능한가요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클린 코드 적으로는 여러 인스턴스 가 각각 가질 메소드 가 아니라서 static 으로 분리 했는데
성능적으로 생각하면 어차피 싱글톤 + static 메소드 메모리를 찾아야 해서 ( 객체 내 같이 메소드가 위치하는게 아니므로 )
더 비효율적 일거 같네요 반영 할게요 🙂

return member;
}
private static void createLog(Member member){
log.info("멤버를 생성했습니다. 멤버 id={}, 멤버 이름={},깃허브 id={}, 닉네임={}",member.getId(),member.getName(),member.getGithubUserId(),member.getUsername());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.info("멤버를 생성했습니다. 멤버 id={}, 멤버 이름={},깃허브 id={}, 닉네임={}",member.getId(),member.getName(),member.getGithubUserId(),member.getUsername());
log.info("멤버를 생성했습니다. 멤버 id={}, 멤버 이름={},깃허브 id={}, 닉네임={}", member.getId(), member.getName(), member.getGithubUserId(), member.getUsername());

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
log.warn("매칭에 실패 했습니다. 방 id={},사용자 id={},사용자 깃허브 닉네임={}",participation.getRoomsId(),participation.getMembersId(),participation.getMember().getUsername());
log.warn("매칭에 실패 했습니다. 방 id={},사용자 id={},사용자 깃허브 닉네임={}", participation.getRoomsId(), participation.getMembersId(), participation.getMember().getUsername());

@@ -54,6 +55,19 @@ private List<MatchResult> matchPairs(Room room, PullRequestInfo pullRequestInfo)
.toList();
}

private void logMatchResults(long roomId, List<MatchResult> matchResults) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위에 createLog() 와 동일한 리뷰입니다!

Comment on lines +27 to +32
public ParticipationStatus getParticipationStatus() {
return switch (this) {
case REVIEWER,REVIEWEE,BOTH -> ParticipationStatus.PARTICIPATED;
case NONE -> ParticipationStatus.NOT_PARTICIPATED;
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break 없이 잘 동작하는가 싶었는데, 람다라 괜찮군요. 구두로 물어봤으니 해결했습니당~


private final ParticipationRepository participationRepository;

public Participation create(Room room, Member member, MemberRole memberRole,ParticipationStatus participationStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public Participation create(Room room, Member member, MemberRole memberRole,ParticipationStatus participationStatus) {
public Participation create(Room room, Member member, MemberRole memberRole, ParticipationStatus participationStatus) {


public Participation create(Room room, Member member, MemberRole memberRole,ParticipationStatus participationStatus) {

return create(room, member, memberRole,participationStatus, room.getMatchingSize());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return create(room, member, memberRole,participationStatus, room.getMatchingSize());
return create(room, member, memberRole, participationStatus, room.getMatchingSize());

}

public Participation create(Room room, Member member, MemberRole memberRole, int matchingSize) {
return create(room, member, memberRole,memberRole.getParticipationStatus(),matchingSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return create(room, member, memberRole,memberRole.getParticipationStatus(),matchingSize);
return create(room, member, memberRole, memberRole.getParticipationStatus(), matchingSize);

participationRepository.delete(participation);
}

private static void createLog(Participation participation) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기도 마찬가지입니당! 네이밍 + static

Participation participation = new Participation(room, manager);
log.info("방을 생성했습니다. 방 생성자 id={}, 요청한 사용자 id={}", room.getManagerId(), memberId);

Participation participation = participationWriter.create(room,manager,MemberRole.REVIEWER,ParticipationStatus.MANAGER);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Participation participation = participationWriter.create(room,manager,MemberRole.REVIEWER,ParticipationStatus.MANAGER);
Participation participation = participationWriter.create(room, manager, MemberRole.REVIEWER, ParticipationStatus.MANAGER);

Copy link
Contributor

@jcoding-play jcoding-play left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoginArgumentResolver 에서 띄우는 로그가 의미있는가 싶어서 제거에 대해서만 얘기하면 될듯요.

인정입니다. 별로 의미 없다 생각했는데 제거해도 좋습니다~

log.debug("개발 피드백 작성[작성자({}), 요청값({})", deliverId, request);
log.info("개발 피드백 작성[작성자({}), 요청값({})", deliverId, request);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines 59 to 68
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 모든 MatchResult에 대해 리뷰어 리뷰이 pr링크를 순회하며 로그를 찍고 있는데, 매번 조회 쿼리가 나갈 것이라 예상이 됩니다.

그리고 저런 정보들이 유의미할까도 고민이 되는데 roomId에 대한 매칭 성공 로그 정도만 남겨보는건 어떨까요?

Copy link
Contributor

Choose a reason for hiding this comment

The 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);
    }

여기서 찍는게 맞는지에 대한 고민은 들었는데
한 트랜잭션 내에서 작동하므로 조회가 또 나갈거란 생각은 안 들었어요.

빼도 크게 상관없을듯요

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch = FetchType.LAZY 지연로딩이라 아마 계속 조회 쿼리 나갈거라 생각되긴 합니다!
저도 확실하지 않아 한번 확인해봐도 좋겠네요!

여기서 찍는게 맞는지에 대한 고민은 들었는데

로그 위치는 저도 크게 상관없다 느껴지네요~! 저 부분도 괜찮은것같아요!

Comment on lines 40 to 47
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

객체 두개를 넣어서 삭제하는 방향은 무엇인가요??
room 조회문은 결국 필연적이라 생각이 드는데, 이 부분이 궁금하네요~ 😀

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ROOM 조회문을 밖에서 해서 넘겨주는 식이 될 거 같아용.

현재 participation.cancel() 을 할 시, 내부 room 을 사용하는데 이때 의도하지 않은 room 조회문이 나갈거 같아요.
그렇기에 명시적으로 밖에서 조회해서 넣는게 더 좋다고 생각한 겁니다!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

내부 room 을 사용하는데 이때 의도하지 않은 room 조회문이 나갈거 같아요.
그렇기에 명시적으로 밖에서 조회해서 넣는게 더 좋다고 생각한 겁니다!

이해 완👍 좋습니당~

Comment on lines 40 to 41
Participation participation = participationWriter.create(room,member, memberRole,request.matchingSize());
return participationRepository.save(participation);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

participationRepo.save() 두번 일어나요~!

@@ -112,6 +111,7 @@ public void delete(long roomId, long memberId) {
Room room = getRoom(roomId);
validateDeletionAuthority(room, memberId);

log.info("방을 삭제했습니다. 방 id={}, 사용자 iD={}", roomId, memberId);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변경된 부분이 아니여서 이곳에 커멘트 남길게요. RoomService에서 ParticipationWriter를 잘 사용하고 있는가에 대한 커멘트입니다.

개인적으로 리팩터링으로 수고하고 있다는거 잘 알지만 👍, 약간 우려된 부분이 ParticipationWriter에서 나온 것 같아 글 남겨봐요!
조이썬이 생각할 때 ParticipationWriter가 잘 사용되고 있다 느껴지나요?

현재 Room과 Participation이 저희 서비스상 많은 연관관계를 가지지만 방을 삭제할 때, 막상 RoomService에서는 ParticipationWriter의 delete 메서드를 활용하기 어려운 구조라 생각해요.
그래서 조이썬도 그냥 두셨다 판단됩니다.

제가 느낄 수 있는 한가지 ParticipationWriter의 장점으로는 방을 참여할 때, participation.participate();를 같이 호출해주어 개발자로 하여금 휴먼 에러를 방지한다?? 정도가 있겠네요.

근데 이 장점도 저희의 처음 코드였던 Room을 생성할 때 초기 참여자 수를 1로 하고, 저장한다면 똑같이 해결되고 오히려 지금 코드보다 깔끔해진다 느껴집니다.

전에 조이썬한테 제가 먼저 이러한 구조로 바꿔보는거 좋을 것 같다고 동영상을 추천해줬던 것 같아요.
저도 다른 사람들을 통해 먼저 들었고, 개선할만한 부분이라 생각해서 추천했었습니다.

근데 나중에 다시 이 방식을 도입한 사람들에게 의견을 들었을 때, 큰 장점을 못 느꼈다는 말도 들었고,
저도 약간 ParticipationWriter에서 그러한 느낌을 받을 수 있네요.

솔직히 말하자면 ParitipationWriter는 필요해서 나온 느낌이 아니라 그냥 써보고 싶어서 만든 느낌이랄까요??

다른 영역에서 자주 사용되는 Room이나 Member 같은 경우는 다음과 같이 사용하면 의미가 있다 느껴지네요~
이 부분은 조이썬도 한번 고민해보면 좋겠습니다! 👍

Copy link
Contributor

@youngsu5582 youngsu5582 Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

제가 느낄 수 있는 한가지 ParticipationWriter의 장점으로는 방을 참여할 때, participation.participate();를 같이 호출해주어 개발자로 하여금 휴먼 에러를 방지한다?? 정도가 있겠네요.

사실, 해당 부분이 Writer 가 분리되는 이유라고 생각해요.
나중에, participation.participate() 를 room 의 size 를 올리지 말고, 엔티티 개수로 파악을 하자고 로직이 변경이 되어도
그때는 온전히 particiaptionWriter 에서만 로직이 수정되게 됩니다.

저희의 처음 코드였던 Room을 생성할 때 초기 참여자 수를 1로 하고, 저장한다면 똑같이 해결되고 오히려 지금 코드보다 깔끔해진다 느껴집니다.

해당 내용도, 엔티티 라면 비즈니스 변화에 의존되지 않게 코드를 사용해야 한다고 생각해요 (+ 생성자도요)
1이라고 했으나, 나중에 방장은 참여자에서 빼자라고 했을때
이 room 도메인을 1로 지정 했던거도 기억하는 거 자체가 비즈니스 로직이 도메인 까지 침범이 된거라고 생각이 들어요.

저는 이런 관점에서 Reader / Writer 의 필요성을 명확하게 느낀거 같아요.
추가로, 이렇게 객체를 넣으면서 코드를 짜며 느낀점으론 좀 더 코드를 생각할 수 있었어요 ( 물론, 제 기준 )
Reader 와 Writer 가 최대한 재사용 될 수 있게, "해당 객체와 Reader 의 의존성이 맞는가?" 등등

추가로, 지금 당장은 그냥 로직을 넣은게 다지만, ( 사실 이거 하나로도 만족스럽긴 해요 전 - 에러 일괄적 관리 + 로그 관리 )
비즈니스 로직이나 검증이 늘어나도
Reader / Writer 내 기능이 늘어나지 Service 의 길이가 길어지지 않는게 매력적으로 느껴졌어요.

솔직히 말하자면 ParitipationWriter는 필요해서 나온 느낌이 아니라 그냥 써보고 싶어서 만든 느낌이랄까요??
다른 영역에서 자주 사용되는 Room이나 Member 같은 경우는 다음과 같이 사용하면 의미가 있다 느껴지네요~

해당 부분 역시도 DB 의 원자성과 비슷하다고 생각해요.
모든 곳에 도입하거나, 도입 하지 않거나가 궁극적으로 되야 할 거 같아요.


이들은 물론 다 제 의견이니 더 생각 달아줘도 좋습니당

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에러 일괄적 관리 + 로그 관리
비즈니스 로직이나 검증이 늘어나도
Reader / Writer 내 기능이 늘어나지 Service 의 길이가 길어지지 않는게 매력적으로 느껴졌어요.

ParticipationWriter에 대한 조이썬 의견 궁금했는데, 좋습니다~!
로그 관리가 된다는 장점도 있었네요.

팀 안에서도 로그는 엔티티 내부가 아닌 Writer, Service로 한정하도록 컨벤션 정해봐도 좋을듯 하네요!

모든 곳에 도입하거나, 도입 하지 않거나가 궁극적으로 되야 할 거 같아요.

저도 이 부분 동의합니다!
저도 관리할 부분이 많은 곳은 Writer, Reader 쓰는게 좋다 느껴지긴 하니, 다 쓰는 쪽으로 해보는 게 좋겠네요!

저도 앞으로 리팩토링 할때는 최대한 Service가 Repo를 의존하지 않는 방향으로 같이 리팩터링 해보겠습니다~

Copy link
Contributor

@ashsty ashsty Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엔티티/생성자는 비즈니스 변화에 의존하지 않아야 한다고 생각해요. (+ 생성자도요) 나중에 방장의 자동 참여 기능이 빠진다고 했을때 이 room 도메인의 참여자 수를 1로 지정했던 사실을 기억해야 한다는 것 자체가 비즈니스 로직이 도메인� 영역을 침범한 경우라고 생각해요.

가볍게 추가만 하자면 저도 이 말에 동의하는 것 같습니다~

이번에 Participation 리팩토링 과정에서 기본 설정값(MemberRole, ParticipationStatus)을 가지는 생성자를 만들었었는데요. 작성자인 저는 제법 명확한 의미를 담고 있다고 생각했던 부분도 다른 사람의 눈으로 봤을 때는 "저게 뭐야?" 싶은 것 같더라고요. 😆

기본 참여자를 1로 설정한다는 건 물론 지금 단계에서는 깔끔한 코드처럼 보이고 실제로 깔끔한 코드를 작성할 수 있게 해주지만,
시간이 지나 이 코드를 누군가 리팩터링해야 한다는 입장에서 보았을 때는 다소 의문일 수도 있겠어요. (정책과 결부되어 있기 때문)

그런 관점에서 Writer를 분리하고 create를 오버로딩한 지금의 리팩터링 방향이 상당히 좋은 것 같다고 느꼈습니다. 👍

전체적으로 이런 방향의 리팩터링이 이루어지고 전체 코드의 구조가 일관적이 된다고 하면 더할 나위 없겠네요.
같은 맥락에서 로깅이 들어가는 클래스를 Writer, Reader로 한정하는 데 동의하고요.


그런데 조금만 덧붙이자면... 이런 리팩터링이 이루어졌을 때 이전에 테스트하던 메서드가 주석 처리 되는 경우가 종종 있는 것 같아요.

그 때는 가능하다면 이 테스트가 주석 처리된 이유도 함께 적어주셨으면 좋겠습니다. 이번에 베타 테스트로 QA하면서도 느꼈지만 테스트가 잘 작성되었다면 사전에 확인할 수 있었을 것 같은 오류가 꽤 많더라고요. 그런 관점에서 보았을 때 리팩터링 이후 이전 테스트 로직이 비활성화되는 건 조금 의문스럽게 느껴지는 것 같아요. (그렇다면 이 로직이 정확히 이전과 똑같이 동작하는지는 뭘 보고 확신할 수 있지?)

리팩터링은 기본적으로 '이전의 로직과 동일한 동작을 하지만 로직만 교체되는 일'인 만큼

"이전에 테스트하던 걸 왜 이제는 테스트하지 않아도 되는 지"에 대해 코드를 보는 사람도 한 눈에 알 수 있으면 바뀐 이후에도 오류가 나지 않을 수 있을 거라는 확신이 더 강해질 것 같기 때문입니다~~! ☺️

이번에 생성자 관련해서 남긴 리뷰도 같은 결이었던 것 같아요!

리팩터링은 정말 너무너무 귀찮고 힘든 일인 걸 알고 있기 때문에 다들 고생하고 계신 거 너무 잘 알고 있습니다~~ 만!
이런 점을 혹시나! 한 번만! 더 부탁드려도 될지! 에 대한 이야기였어요~!! 감사합니다!

요약: 리팩터링할 때 테스트도 신경 쓰자 (작성자도 리뷰어도 다같이) (뭐라하는 거 아님!!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말 안했던 건 제 잘못인거 같네용.
다음부터는 테스트 주석 할 때 사유 미리 남겨놓을게용

주석한 사유는 그전 PR 에 남겼으므로 생략하겠습니다.

Copy link
Contributor

@ashsty ashsty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드가 진짜 많이 깔끔해졌네요~!!

간단한 코멘트 정도만 남겨뒀습니다.
고생하셨어요!

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

근데 이건 좀 심했다

Comment on lines 47 to 53
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());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 생성자들은 확인해보니 테스트에서만 쓰이고 있는 것 같아요.

비즈니스 로직을 보면 create를 오버로딩해서 커버하고 있던데
그럼 더더욱 혼란을 막기 위해서라도 생성자들은 삭제하는 건 어떠신지용.

테스트 지옥이 되긴 할 테지만
리팩토링이 잘 되었는지 확인하기 위해서라도 변경사항이 테스트에 반영되어야 하지 않을까요~~ 😆

Copy link
Contributor

@youngsu5582 youngsu5582 Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전 이래서 편의성을 사용하는 ( REVIEWER , MANAGER 등을 미리 지정하는 )
생성자 증가시키는거 반대하자는 파 였는디...

@@ -112,6 +111,7 @@ public void delete(long roomId, long memberId) {
Room room = getRoom(roomId);
validateDeletionAuthority(room, memberId);

log.info("방을 삭제했습니다. 방 id={}, 사용자 iD={}", roomId, memberId);
Copy link
Contributor

@ashsty ashsty Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

엔티티/생성자는 비즈니스 변화에 의존하지 않아야 한다고 생각해요. (+ 생성자도요) 나중에 방장의 자동 참여 기능이 빠진다고 했을때 이 room 도메인의 참여자 수를 1로 지정했던 사실을 기억해야 한다는 것 자체가 비즈니스 로직이 도메인� 영역을 침범한 경우라고 생각해요.

가볍게 추가만 하자면 저도 이 말에 동의하는 것 같습니다~

이번에 Participation 리팩토링 과정에서 기본 설정값(MemberRole, ParticipationStatus)을 가지는 생성자를 만들었었는데요. 작성자인 저는 제법 명확한 의미를 담고 있다고 생각했던 부분도 다른 사람의 눈으로 봤을 때는 "저게 뭐야?" 싶은 것 같더라고요. 😆

기본 참여자를 1로 설정한다는 건 물론 지금 단계에서는 깔끔한 코드처럼 보이고 실제로 깔끔한 코드를 작성할 수 있게 해주지만,
시간이 지나 이 코드를 누군가 리팩터링해야 한다는 입장에서 보았을 때는 다소 의문일 수도 있겠어요. (정책과 결부되어 있기 때문)

그런 관점에서 Writer를 분리하고 create를 오버로딩한 지금의 리팩터링 방향이 상당히 좋은 것 같다고 느꼈습니다. 👍

전체적으로 이런 방향의 리팩터링이 이루어지고 전체 코드의 구조가 일관적이 된다고 하면 더할 나위 없겠네요.
같은 맥락에서 로깅이 들어가는 클래스를 Writer, Reader로 한정하는 데 동의하고요.


그런데 조금만 덧붙이자면... 이런 리팩터링이 이루어졌을 때 이전에 테스트하던 메서드가 주석 처리 되는 경우가 종종 있는 것 같아요.

그 때는 가능하다면 이 테스트가 주석 처리된 이유도 함께 적어주셨으면 좋겠습니다. 이번에 베타 테스트로 QA하면서도 느꼈지만 테스트가 잘 작성되었다면 사전에 확인할 수 있었을 것 같은 오류가 꽤 많더라고요. 그런 관점에서 보았을 때 리팩터링 이후 이전 테스트 로직이 비활성화되는 건 조금 의문스럽게 느껴지는 것 같아요. (그렇다면 이 로직이 정확히 이전과 똑같이 동작하는지는 뭘 보고 확신할 수 있지?)

리팩터링은 기본적으로 '이전의 로직과 동일한 동작을 하지만 로직만 교체되는 일'인 만큼

"이전에 테스트하던 걸 왜 이제는 테스트하지 않아도 되는 지"에 대해 코드를 보는 사람도 한 눈에 알 수 있으면 바뀐 이후에도 오류가 나지 않을 수 있을 거라는 확신이 더 강해질 것 같기 때문입니다~~! ☺️

이번에 생성자 관련해서 남긴 리뷰도 같은 결이었던 것 같아요!

리팩터링은 정말 너무너무 귀찮고 힘든 일인 걸 알고 있기 때문에 다들 고생하고 계신 거 너무 잘 알고 있습니다~~ 만!
이런 점을 혹시나! 한 번만! 더 부탁드려도 될지! 에 대한 이야기였어요~!! 감사합니다!

요약: 리팩터링할 때 테스트도 신경 쓰자 (작성자도 리뷰어도 다같이) (뭐라하는 거 아님!!)

@youngsu5582 youngsu5582 merged commit 48256cd into develop Oct 16, 2024
4 checks passed
@youngsu5582 youngsu5582 deleted the feat/#608 branch October 16, 2024 12:37
pp449 added a commit that referenced this pull request Oct 19, 2024
* [BE] 방 매칭 실패 시 매칭 실패 원인을 전달하는 기능 구현(#562) (#575)

* feat: 반환된 예외를 통해 실패 원인을 찾는 기능 구현

* feat: 실패한 매칭 정보를 저장하는 엔티티 구현

* feat: 매칭을 실패했을 경우 실패한 매칭 정보를 저장하는 기능 구현

* refactor: 클래스명 변경

* feat: 반환된 예외를 통해 매칭 실패 원인에 대한 메세지를 얻는 기능 구현

* refactor: FailedMatching 생성자 변경

* feat: 매칭 실패한 방을 조회시, 실패 원인을 같이 전달하는 기능 구현

* refactor: 맵 구현체 변경

* refactor: 피드백 반영

---------

Co-authored-by: gyungchan Jo <[email protected]>

* [BE] 방 종료 시, 코드 리뷰 완료 버튼 동작 안하도록 하는 기능 구현(#579) (#580)

* feat: 방 종료 후 코드 리뷰 완료 버튼 검증 로직 구현

* fix: 잘못된 요청값 수정 (#566)

Co-authored-by: youngsu5582 <[email protected]>

* refactor: 리뷰 완료 요청 검증 로직 변경

---------

Co-authored-by: gyungchan Jo <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: youngsu5582 <[email protected]>

* [BE] 참여했던 방이 종료되면 참여 중 탭에서 제거(#581) (#584)

* feat: 참여했던 방이 종료되면 참여 중 탭에서 제거하는 기능 구현

* [BE] 중복으로 매칭되던 상황 해결(#587) (#588)

* refactor: 도메인 수정

* feat: 리뷰어, 리뷰이 조회 API 기능 구현

* refactor: 중복된 기능 코드 제거

* docs: 메서드 시그니쳐 통일

* feat: 시연용 데이터 추가

* refactor: 패키지 이동으로 인한 오류 수정

* feat: 시연용 데이터 추가

* fix: REMOTE_ORIGIN 그냥 변수로 변경

* feat: 데이터 추가

* feat: 서브모듈 반영

* feat: response 생성 때 reviewer, reviewee 분리

* feat: application 설정 변경

* feat: 데모 데이터 함수로 분리

* fix: 누락된 saveAll 추가

* fix: 데이터 정합성 수정

* fix: roomId 상수 변경

* feat: 피드백 키워드 뒤 .제거

* refactor: 3차 데모데이 데이터 변경

* feat: room 4에 대한 케이스도 추가

* feat: room 4 매칭 추가

* fix: 응답 내 프로필 링크로 변경

* 최신 브랜치 병합

* feat: submodule 업데이트

* refactor: 서브모듈 변경

* fix: 매칭 중복 예외 처리 안되던 오류 해결

* fix: 리뷰어 매칭 안되던 오류 수정

* test: participationFilter 로직 수정에 따른 테스트 수정

---------

Co-authored-by: hjk0761 <[email protected]>
Co-authored-by: youngsu5582 <[email protected]>

* feat: pr 제출 안 했을 때 문구 추가 (#590)

Co-authored-by: jinsil <[email protected]>

* [BE] 방 생성 검증 로직 주석 처리(#593) (#594)

* refactor: 방 생성 검증 로직 주석 처리

* refactor: 운영 환경 설정 변경

* refactor: 방 생성 검증 로직 테스트 disabled 처리

---------

Co-authored-by: gyungchan Jo <[email protected]>

* [BE] 방 매칭 실패 시 매칭 실패 원인을 전달하는 기능 구현(#562) (#575)

* feat: 반환된 예외를 통해 실패 원인을 찾는 기능 구현

* feat: 실패한 매칭 정보를 저장하는 엔티티 구현

* feat: 매칭을 실패했을 경우 실패한 매칭 정보를 저장하는 기능 구현

* refactor: 클래스명 변경

* feat: 반환된 예외를 통해 매칭 실패 원인에 대한 메세지를 얻는 기능 구현

* refactor: FailedMatching 생성자 변경

* feat: 매칭 실패한 방을 조회시, 실패 원인을 같이 전달하는 기능 구현

* refactor: 맵 구현체 변경

* refactor: 피드백 반영

---------

Co-authored-by: gyungchan Jo <[email protected]>

* [BE] 방 종료 시, 코드 리뷰 완료 버튼 동작 안하도록 하는 기능 구현(#579) (#580)

* feat: 방 종료 후 코드 리뷰 완료 버튼 검증 로직 구현

* fix: 잘못된 요청값 수정 (#566)

Co-authored-by: youngsu5582 <[email protected]>

* refactor: 리뷰 완료 요청 검증 로직 변경

---------

Co-authored-by: gyungchan Jo <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: youngsu5582 <[email protected]>

---------

Co-authored-by: gyungchan Jo <[email protected]>
Co-authored-by: ashsty <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: hjk0761 <[email protected]>
Co-authored-by: youngsu5582 <[email protected]>
Co-authored-by: jinsil <[email protected]>

* refactor: 로컬 서버용 깃허브 토큰 변경 (#600)

Co-authored-by: gyungchan Jo <[email protected]>

* [BE] Room Controller 역할 분리(#595) (#596)

* refactor: Room Controller 역할 분리

* refactor: 피드백 반영

---------

Co-authored-by: youngsu5582 <[email protected]>

* [BE] 자동 예약 및 자동 업데이트 동시성 제어(#586) (#604)

* feat: Lock을 통한 자동 매칭 동시성 제어 기능 구현

* feat: Lock을 통한 자동 업데이트 동시성 제어 기능 구현

* feat: 자동 매칭 Lock 범위 최소화를 위한 클래스 구현

* feat: 자동 예약 Lock 범위 최소화를 위한 클래스 구현

* test: 자동 매칭 테스트 중복 코드 추상 클래스로 이동

* test: 자동 예약 테스트 중복 코드 추상 클래스로 이동

* refactor: Transactional 어노테이션 제거

* refactor: 피드백 반영

---------

Co-authored-by: gyungchan Jo <[email protected]>

* [BE] 방 수정 기능 & 스케줄러 부분 분리 및 리팩토링(#605) (#606)

* feat: repository 의존성 제거, 테스트 명확하게 변경

* feat: Reader/Writer 통해 조회 로직 분리

* feat: 룸 수정 기능 구현

* refactor: 피드백 반영

* fix: 충돌 해결

---------

Co-authored-by: youngsu5582 <[email protected]>

* [FE] 콜리네 서비스 제공후 QA(#597) (#602)

* fix: 모집 마감 -> 종료된 방으로 변경

* fix: 매칭 후 pr 제출 안 해서 실패했을 때 바로 문구 띄우기

* design: 모달 이름에 width 변경

* design: 프로필 드롭다운에서 이름 다 보여지게 하기

* design: 배너 medium일 때 높이 수정

* feat: timeDropdown에서 선택된 시간이 제일 위에 떠있게 하기

* feat: 피드백 모아보기에서 세부 피드백에 scroll 추가

* design: 세부 피드백 높이 지정

* fix: content 길이 길어졌을 때 ... 로 보이게 하기

* design: 피드백 모달 엔터처리, line-height 추가

* feat: 분류 드롭다운으로 선택하게 변경

* feat: 키워드 없을 때 ui 처리

* feat: 방 매칭 실패 시 서버가 준 message 띄우기

* [BE] Room Controller 역할 분리(#595) (#596)

* refactor: Room Controller 역할 분리

* refactor: 피드백 반영

---------

Co-authored-by: youngsu5582 <[email protected]>

* refactor: ALL 타입 추가

* feat: 수정된 api에 맞게 message 타입 추가

* fix: 방 생성 시 키워드 입력을 안했을 때 렌더링 조건 수정

* feat: 바뀐 RoomInfo 데이터 형식에 맞게 storybook 수정

---------

Co-authored-by: jinsil <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: youngsu5582 <[email protected]>
Co-authored-by: 00kang <[email protected]>

* [BE] 참여했던 방이 종료 된 후, 종료 탭 및 마이페이지에서 안보이는 문제 해결(#607) (#611)

* refactor: 참여한 방을 조회하는 기능 수정

* refactor: 방을 조회하는 기능 메서드 분리

* refactor: 방을 조회 메서드 클래스 분리

* test: Nested 어노테이션을 통한 RoomServiceTest 관심사 분리

* feat: 참여했던 방이 종료되면 종료 탭에서 보이도록 하는 기능 구현

* feat: 종료 탭에서 자신이 참여했던 방도 나타내는 기능 구현

* test: 테스트 수정

* refactor: 조회 쿼리 수정

* refactor: 피드백 반영

* refactor: 변수명 변경

---------

Co-authored-by: gyungchan Jo <[email protected]>

* [BE] 로그 중복 작성 제거, 비즈니스 로직 내 로그 추가(#608) (#610)

* feat: additivity false 로 변경

* refactor: 참가,취소 로직 분리

* chore: 로그 레벨 변경

* feat: 로그 추가

* refactor: 피드백 반영

---------

Co-authored-by: youngsu5582 <[email protected]>

* [FE] 24시간 이하로 남으면 디데이 말고 시간 보여주기(#598) (#609)

* fix: 디데이가 1일 남은경우에도 24시간 미만 남은경우 남은 dday로 반환하도록 변경

* refactor: areDatesEqual 함수를 dateFormatter 유틸 함수로 이동

* fix: 중복된 함수 제거

* test: 데이터 포메팅 유틸 함수 테스트코드 추가

* test: 테스트 시 이미지 모듈은 mock string 값이 반환되도록 모킹

* test: 룸 카드의 남은 기간 정보 UI 렌더링 테스트 추가

* fix: 진행중인 방의 D-Day 가 잘못 표기되는 문제 해결

* test: 테스트 명세 오타 수정

* test: 방 상세정보의 모집/리뷰 마감까지 남은 시간 렌더링 테스트 추가

* test: jest 환경에서 시간대를 한국 시간대를 사용하도록 변경

* feat: SEO 최적화를 위해 html에 메타태그 추가

---------

Co-authored-by: Lee sang Yeop <[email protected]>

* [FE] 생성페이지 달력, 시간 입력 리팩토링(#603) (#614)

* design: 방 생성 페이지 모바일, 태블릿 대응

* refactor: error props 제거

* feat: DateTimePicker 생성하여 날짜, 시간 한번에 관리

* refactor: formatCombinedDateTime 인자 하나로 받기

* refactor: handleInputChange 함수로 통합하기

* refactor: 분류 type 맞추기

* feat: 방 생성 유효성 처리

---------

Co-authored-by: jinsil <[email protected]>

* refactor: 초기 데이터 주석 처리

* refactor: 초기 참여자 사이즈 변경

* feat: 매칭 실패 원인을 전달하는 기능 구현

* [BE] 방 응답 시, CLASSIFICATION 반환 추가(#621) (#623)

* feat: 방 정보 응답 시, classification 정보 추가

* feat: 개발 서버 액세스 토큰 변경

* test: 테스트 통과하도록 주석 처리

---------

Co-authored-by: gyungchan Jo <[email protected]>

* refactor: 요청 dto 제약 조건 변경

* [FE] 방 수정 기능 추가(#616) (#620)

* feat: hostType node_env로 판별

* feat: 검색 title 변경

* fix: test에 message 추가하여 오류 해결

* feat: 방 수정 api 추가

* refactor: classification 속성 추가하기

* refactor: 방 상세 정보 가져오는 함수 훅으로 분리

* feat: 방 생성, 수정 페이지 분리

* fix: put 요청으로 변경

* refactor: getInitialFormState 함수로 분리

* design: 드롭다운 선택된 것 다르게 보여주기

* refactor: 불필요한 코드 제거

* fix: roomId로 변경

* feat: roomId로 변경

---------

Co-authored-by: jinsil <[email protected]>

* [FE] 참여 중인 방에 대해 바뀐 명세서 반영하기(#612) (#622)

* refactor: 수정된 API명세서에 맞게 includeClosed 쿼리 추가

* feat: 참여중 라벨 추가

* feat: 참여중, 모집중 라벨 같이 보여주기

* feat: 종료, 매칭실패 라벨 같이 보여주기

* feat: 참여중 라벨을 범용적으로 사용하기 위해 참여로 수정

* refactor: 참여 라벨, 매칭실패 라벨 고려하여 추가하기

* test: roomStatus, participationStatus, message 상태에 따라 다르게 보여지는 라벨의 테스트 구현

* feat: 서비스 어느 곳에서든지 참여 라벨이 있을 경우 상세페이지로 리다이렉팅

---------

Co-authored-by: 00kang <[email protected]>
Co-authored-by: Lee sang Yeop <[email protected]>

* [BE] 쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 변경 + 리팩터링(#613) (#617)

* refactor: 작성한 피드백은 바로 피드백 모아보기에서 보도록 수정

* refactor: 피드백 가져오는 역할 클래스 분리

* refactor: 개발 피드백 작성 기능 수정

* refactor: 개발 피드백 수정 기능 변경

* refactor: 개발 피드백 조회 기능 수정

* refactor: 생성, 수정 dto 분리

* refactor: 피드백을 조회하는 기능 수정

* refactor: 피드백 반영

* refactor: 피드백 반영

---------

Co-authored-by: gyungchan Jo <[email protected]>

* [BE] 매칭 로직 원복 및 수정(#624) (#627)

* refactor: 도메인 수정

* feat: 리뷰어, 리뷰이 조회 API 기능 구현

* refactor: 중복된 기능 코드 제거

* docs: 메서드 시그니쳐 통일

* feat: 시연용 데이터 추가

* refactor: 패키지 이동으로 인한 오류 수정

* feat: 시연용 데이터 추가

* fix: REMOTE_ORIGIN 그냥 변수로 변경

* feat: 데이터 추가

* feat: 서브모듈 반영

* feat: response 생성 때 reviewer, reviewee 분리

* feat: application 설정 변경

* feat: 데모 데이터 함수로 분리

* fix: 누락된 saveAll 추가

* fix: 데이터 정합성 수정

* fix: roomId 상수 변경

* feat: 피드백 키워드 뒤 .제거

* refactor: 3차 데모데이 데이터 변경

* feat: room 4에 대한 케이스도 추가

* feat: room 4 매칭 추가

* fix: 응답 내 프로필 링크로 변경

* 최신 브랜치 병합

* feat: submodule 업데이트

* refactor: 서브모듈 변경

* feat: submodule 업데이트

* refactor: 매칭 로직 원복

* fix: 매칭 로직 오류 수정

* refactor: 변수명 직관적으로 수정

* refactor: 리뷰어를 제외하고 matchingSize 만큼 추가 매칭하도록 수정

* refactor: 리뷰어가 matchingSize 만큼 리뷰이와 매칭되도록 수정

* feat: submodule 업데이트

* refactor: 피드백 반영

* refactor: 변수명 수정

---------

Co-authored-by: hjk0761 <[email protected]>
Co-authored-by: youngsu5582 <[email protected]>
Co-authored-by: HyunJoong Kim <[email protected]>

* [BE] 참여한 전체 방들 조회할 때, 실패 원인 같이 보내는 기능 구현(#625) (#629)

* refactor: ParticipationWriter 패키지 변경 및 코드 포맷팅 적용

* refactor: 작은 단위로 클래스 분리

* refactor: 예외 타입명 변경

* refactor: 참여했지만 매칭에 실패했던 모든 방들을 조회할 때 실패 메시지를 같이 보내는 기능 구현

* refactor: 방 조회 기능 수정

* test: 방 관련 테스트 추가

---------

Co-authored-by: gyungchan Jo <[email protected]>

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: gyungchan Jo <[email protected]>
Co-authored-by: youngsu5582 <[email protected]>
Co-authored-by: ashsty <[email protected]>
Co-authored-by: hjk0761 <[email protected]>
Co-authored-by: jinsil <[email protected]>
Co-authored-by: 00kang <[email protected]>
Co-authored-by: HyunJoong Kim <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BE 백엔드 개발 관련 작업 리팩터링 리팩터링 작업 인프라 인프라 관련 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BE] 로그 중복 작성 제거, 비즈니스 로직 내 로그 추가
4 participants