Refactor/#154: 채팅방 서비스 리팩토링 #164

hyunn522 · 2024-10-09T08:32:20Z

📝 요약

resolved Refactor: 채팅방 서비스 리팩토링 #154

🔖 변경 사항

ChatRoom 객체의 행동은 해당 객체 내로 책임을 위임하였습니다
ChatRoomService 단에서 SRP를 지키기 위해 메소드를 분리하였습니다
+) 리뷰에 따라 TimeUtils을 추가하였습니다 프로젝트 전체에서 사용되는 클래스들은 아래 구조처럼 global 디렉토리 내에 관리하는 게 좋을 것 같습니다

├── domain
│   ├── authorization
│   ├── user
│   ├── chat
│   ├── ...
├── global
│   ├── common
│   ├── config
│   ├── error
│   └── util

✅ 리뷰 요구사항

메소드를 임의로 분리해봤는데 더 필요한 부분이 있으면 의견 남겨주세요
디렉토리 구조 변경 및 util 함수 미사용 처리는 ChattingService 리팩토링 시에 진행할 예정입니다

📸 확인 방법 (선택)

📌 PR 진행 시 이러한 점들을 참고해 주세요

* P1 : 꼭 반영해 주세요 (Request Changes) - 이슈가 발생하거나 취약점이 발견되는 케이스 등
* P2 : 반영을 적극적으로 고려해 주시면 좋을 것 같아요 (Comment)
* P3 : 이런 방법도 있을 것 같아요~ 등의 사소한 의견입니다 (Chore)

drbug2000 · 2024-10-10T00:14:06Z

src/main/java/space/space_spring/dao/chat/UserChatRoomDao.java

+    UserChatRoom findByUserAndChatRoomAndStatus(User userByUserId, ChatRoom chatRoomByChatRoomId, String status);

-    List<UserChatRoom> findByChatRoom(ChatRoom chatRoom);
+    List<UserChatRoom> findByChatRoomAndStatus(ChatRoom chatRoom, String status);


soft delete 구현하는 방식들 중에서 이렇게 status 조건을 parameter로 넘겨주는 방식, Jpa를 잘 활용하고 헷갈릴 일이 없어서 좋은 방법 중에 하나라고 생각합니다.
다만 Active를 계속 적어줘야돼서 조금 귀찮을 순 있겠네요

네 따로 custom한 repository 메소드의 경우에는 Active 필터링 조건을 추가하는 게 변경이 많이 없는데
기본 Jpa 메소드를 활용하는 경우에는 이 방식이 제일 간단한 것 같습니다
음 파라미터로 status를 노출하는 게 조금 부자연스러운 것 같긴 한데.. 나중에 같이 얘기해보면 좋을 것 같아요

drbug2000 · 2024-10-10T00:16:21Z

src/main/java/space/space_spring/dao/chat/custom/ChatRoomDaoImpl.java

+                .join(userChatRoom).on(
+                        userChatRoom.chatRoom.eq(chatRoom)
+                        .and(userChatRoom.user.eq(who))
+                        .and(userChatRoom.status.eq("ACTIVE")))


"ACTIVE"랑 "INACTIVE"를 프로젝트에서 공통으로 사용할 enum으로 만들면 좋을 것 같아요

좋아요 반영하겠습니다

drbug2000 · 2024-10-10T00:17:58Z

src/main/java/space/space_spring/entity/ChatRoom.java

+        return this.getCreatedAt()
+                .atZone(ZoneId.of("UTC")) // UTC로 해석
+                .withZoneSameInstant(ZoneId.of("Asia/Seoul")) // 서울 시간대로 변환
+                .toLocalDateTime(); // LocalDateTime으로 변환


UserChatRoom에도 같은 함수가 있던데 TimeUtil 같은 객체로 뻬서 로직을 묶어주는 것도 좋아보입니다

저도 생각해봤는데 해당 TimeUtil 객체가 어느 계층에 들어가는 게 적합할지 잘 모르겠어서 남겨뒀던 부분입니다
어느 계층이 적합하다고 생각하시나요?

외부 의존성이 없고(Service나 repository를 참조하지 않음) 그냥 함수 모음? 로직 모음? 이니까 계층과 상관 없이 만들어도 되지 않을까 싶습니다. (KUIT 미션에서 JwtUtil처럼)

seongjunnoh

LGTM
기존 ChatRoomService 에서 수행하는 많은 로직들을 특정 객체들에게 책임을 위임 & SRP에 따라 private 메서드 여러개로 분리하여 이전보다 훨씬 가독성이 좋아진 것 같습니다!
다만, private 메서드로 분기처리하신 로직들도 테스트가 필요한 중요한 로직이라고 생각하고, 테스트가 해당 객체의 내부 구현이 아니라 외부로 보여지는 인터페이스 부분을 대상으로 작성되는 것임을 생각하면, private 메서드들로 쪼개놓은 로직들에 대한 책임을 담당하는 다른 객체를 생성하는 방법을 생각해보시는 방법은 어떤가요??
물론 이렇게 되면 가독성 측면에서는 조금 손해를 보는 것 같지만(서비스단 내에서 관련 로직들을 모아서 처리하는게 가독성 측면에서는 더 좋은 것 같다는 개인적인 생각입니다) , 새로 생성한 객체들에 대한 유닛 테스트를 작성함으로써 서비스단 코드의 테스트를 더 수월하게 할 수 있다는 장점이 있다고 생각합니다!!

seongjunnoh · 2024-10-28T13:56:02Z

src/main/java/space/space_spring/global/util/TimeUtils.java

+            .atZone(ZoneId.of("UTC"))
+            .withZoneSameInstant(ZoneId.of("Asia/Seoul"))
+            .toLocalDateTime();
+    }


LGTM
이 코드는 따로 테스트가 필요한 코드는 아닌거 같으니, 중복되는 코드를 막기위해 utils 로 분리하신 것이 구조적으로 훨씬 좋아보입니다

seongjunnoh · 2024-10-28T13:59:38Z

src/main/java/space/space_spring/entity/UserChatRoom.java

+    public void setUserRejoin() {
+        this.setLastReadTime(LocalDateTime.now());
+        this.updateActive();
+    }


LGTM
UserSpaceRoom 객체 내부로 userRejoin에 대한 책임을 위임하니 훨씬 객체지향스러워 진 것 같습니다

hyunn522 · 2024-10-29T03:43:28Z

LGTM 기존 ChatRoomService 에서 수행하는 많은 로직들을 특정 객체들에게 책임을 위임 & SRP에 따라 private 메서드 여러개로 분리하여 이전보다 훨씬 가독성이 좋아진 것 같습니다! 다만, private 메서드로 분기처리하신 로직들도 테스트가 필요한 중요한 로직이라고 생각하고, 테스트가 해당 객체의 내부 구현이 아니라 외부로 보여지는 인터페이스 부분을 대상으로 작성되는 것임을 생각하면, private 메서드들로 쪼개놓은 로직들에 대한 책임을 담당하는 다른 객체를 생성하는 방법을 생각해보시는 방법은 어떤가요?? 물론 이렇게 되면 가독성 측면에서는 조금 손해를 보는 것 같지만(서비스단 내에서 관련 로직들을 모아서 처리하는게 가독성 측면에서는 더 좋은 것 같다는 개인적인 생각입니다) , 새로 생성한 객체들에 대한 유닛 테스트를 작성함으로써 서비스단 코드의 테스트를 더 수월하게 할 수 있다는 장점이 있다고 생각합니다!!

'다른 객체'로 빼는 것은 결국에 전에 의논했던 service 레이어 내에서의 계층 구분으로 이어지는 것 같습니다!
순환 참조 이슈를 주의하여 단방향으로만 의존관계를 맺으면 service 레이어 내에서 계층을 구분하는 것도 좋은 방법인 것 같아요.
레퍼런스 : https://jaeseo0519.tistory.com/314

…ckEnd into refactor/#154/채팅방-서비스-리팩토링

arkchive

LGTM!

drbug2000

GOOD

hyunn522 added 5 commits October 8, 2024 17:14

feat: ChatRoom 관련 객체의 BaseResponse 수정 로직 분리

469fc0e

feat: ChatRoomDao에서 DB 조회 시 active 조건 추가

0e8fd3b

feat: ChatRoomService에서 Optional 객체 도입

64599fe

feat: ChatRoomService 메소드 분리

a74b62f

feat: 변경 사항에 따라 test 임의 주석 처리

d9e6f8f

hyunn522 self-assigned this Oct 9, 2024

hyunn522 linked an issue Oct 9, 2024 that may be closed by this pull request

Refactor: 채팅방 서비스 리팩토링 #154

Closed

feat: 불필요한 클래스 제거

ecd329d

drbug2000 approved these changes Oct 10, 2024

View reviewed changes

hyunn522 added 2 commits October 10, 2024 11:35

refactor: TimeUtils 추가

dbe4be5

refactor: TimeUtils 위치 변경

a9f2ec6

seongjunnoh approved these changes Oct 28, 2024

View reviewed changes

hyunn522 added 4 commits October 30, 2024 21:09

refactor: BaseStatus의 status 상수화

9268500

fix: TimeUtil의 getEncodedTime 메소드 적용

e96cd58

refactor: 글로벌 디렉토리 생성

b6f2fe5

Merge branch 'develop' of https://github.com/KUIT-Space/KUIT_Space_Ba…

962f3df

…ckEnd into refactor/#154/채팅방-서비스-리팩토링

arkchive approved these changes Oct 30, 2024

View reviewed changes

drbug2000 approved these changes Oct 30, 2024

View reviewed changes

hyunn522 added 3 commits October 31, 2024 14:57

refactor: ChatRoomDao -> ChatRoomRepository로 클래스명 변경

8023f1a

refactor: ChatRoomRepositoryTest 시 status 조건 추가

bcde30a

refactor: chat 도메인 내의 dao -> repository 네이밍 변경

750a00f

hyunn522 merged commit 595e7ad into develop Nov 5, 2024
3 checks passed

hyunn522 deleted the refactor/#154/채팅방-서비스-리팩토링 branch November 5, 2024 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/#154: 채팅방 서비스 리팩토링 #164

Refactor/#154: 채팅방 서비스 리팩토링 #164

hyunn522 commented Oct 9, 2024 •

edited

Loading

drbug2000 Oct 10, 2024

hyunn522 Oct 10, 2024 •

edited

Loading

drbug2000 Oct 10, 2024

hyunn522 Oct 10, 2024

drbug2000 Oct 10, 2024

hyunn522 Oct 10, 2024

drbug2000 Oct 10, 2024

seongjunnoh left a comment

seongjunnoh Oct 28, 2024

seongjunnoh Oct 28, 2024

hyunn522 commented Oct 29, 2024

arkchive left a comment

drbug2000 left a comment

Refactor/#154: 채팅방 서비스 리팩토링 #164

Refactor/#154: 채팅방 서비스 리팩토링 #164

Conversation

hyunn522 commented Oct 9, 2024 • edited Loading

📝 요약

🔖 변경 사항

✅ 리뷰 요구사항

📸 확인 방법 (선택)

📌 PR 진행 시 이러한 점들을 참고해 주세요

Choose a reason for hiding this comment

hyunn522 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongjunnoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyunn522 commented Oct 29, 2024

arkchive left a comment

Choose a reason for hiding this comment

drbug2000 left a comment

Choose a reason for hiding this comment

hyunn522 commented Oct 9, 2024 •

edited

Loading

hyunn522 Oct 10, 2024 •

edited

Loading