Refactoring voice room 객체 정리 및 미사용 코드 삭제 #162

drbug2000 · 2024-10-04T15:57:56Z

📝 요약

이슈 번호 : #158

🔖 변경 사항

VoiceRoomDtoList를 갖고 있는 VoiveRoomDtoList 일급 컬렉션 객체를 만들어 리팩토링했습니다.
성능비교용으로 남겨둔 비동기 적용 전 코드들을 삭제 했습니다.

✅ 리뷰 요구사항

VoiceRoomList 객체를 중심으로 리팩토링된 구조에 대해서 피드백 해주시면 감사하겠습니다.

📸 확인 방법 (선택)

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

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

apply on VoiceRoomService only set RoomDto not participant

…voice-room-domain/dto,repo' of https://github.com/KUIT-Space/KUIT_Space_BackEnd into refactoring/#158/voice-room-domain/dto,repo

hyunn522

LGTM!
status가 active인 것까지 검색하려니 다 쿼리문을 쏴줘야 하는 게 아쉽네요.. 대안을 좀 찾아봐야겠어요

hyunn522 · 2024-10-07T07:09:30Z

src/main/java/space/space_spring/dto/VoiceRoom/VoiceRoomDtoList.java

+import java.util.stream.Collectors;
+@Service
+@RequiredArgsConstructor
+public class VoiceRoomDtoList {


p3: 사소한 거긴 한데 클래스명을 VoiceRoomListDto로 하는 건 어떨까요? dto의 list를 나타내기 위해서 네이밍을 이렇게 하신 것 같은데 결국 이 클래스도 DTO에 속하니까 접미사를 통일하면 좋을 것 같아서요

오 좋은 의견 감사합니다. 그런 식의 이름이 더 알맞은 것 같네요

뭔가 해당 객체는 Data Transfer라는 DTO의 본질적인 존재 이유? 에 비해 많은 프로시저를 수행하기 때문에
아예 DTO 라는 네이밍을 제거하는게 어떤가요??
이 객체가 보이스룸 리스트를 만들고, 관리하는 역할을 수행하는거 같은데 VoiceRoomListProvider 추천 드립니닷

hyunn522 · 2024-10-07T07:22:34Z

src/main/java/space/space_spring/dto/VoiceRoom/VoiceRoomDtoList.java

+
+    }
+
+    private List<ParticipantDto> getParticipantDtoListById(long voiceRoomId){


p2: 해당 메소드는 repository에 의존해서 VoiceRoomDtoList 객체 내에서의 행동으로 보고 객체 내로 위임하기에는 애매한 것 같은데 어떻게 생각하시나요?

네 동의 합니다.
이 함수와 관련 함수/기능들을 ParticipantService와 ParticipantListDto로 책임을 분리하는 설계를 현재 고려 중입니다.
Repository관련 의존성은 최대한 service단으로 빼고, 최대한 객체 자체(혹은 객체List자체)에 대한 로직만 남길 수 있도록 짜보겠습니다.

음 개인적인 생각이긴 한데, 스프링 빈 객체(컴포넌트) 로 분리한 객체가 Repository 에 의존성을 가지는 것이 필수적이라면 이 방식도 괜찮지 않을까요??
서비스단의 하나의 메서드에서 너무 많은 책임을 가져서 객체를 분리하고, 해당 로직을 분리한 객체로 위임 & 책임을 분리한 로직이 repository 의존성을 가질 경우에는 이 방식도 괜찮지 않나 라는 생각입니다

혹시 레포지토리에 의존성을 가지는 객체를 하나 더 생성할 바에는 서비스단의 코드를 좀 더 무겁게 하고, 레포지토리 의존성을 서비스단만 가지도록 하는게 더 낫다고 생각하시나요??

아 제가 애매하게 적어놓았네요
저는 1. repository에 의존성을 가지는 것 2. 객체의 행동이라고 보기에 애매한 것 -> 이 두 가지 때문에 객체 내로 위임하지 않는 게 맞다고 생각했습니다
해당 로직이 객체 내부의 행동이라기보다는, db에서 데이터를 찾아오는 것이 주된 기능이라고 생각해서요

repository 의존성 관련해서는 좀 더 생각해보겠습니다 전 객체의 행동인데 repository가 필요한 게 있을까.. 하는 생각이었습니당

우선 제 최근 커밋에서는 VoiceRoomDtoList에서 모든 의존성을 제거했습니다. 위 코드는 이전 커밋 버전입니다.
제가 repository 의존성을 제거한 가장 중요한 이유는 VoiceRoomDtoList를 일급 컬렉션으로 만들고 싶었기 때문입니다.
일급 컬렉션 규칙 중에 "컬렉션 하나 만을 멤버 변수로 가져야한다"가 있었기 때문에 최대한 의존성을 제거하는 방향으로 만들 었습니다. (final 변수들이 포함되는지는 잘 모르겠지만)

"외부와 데이터를 주고 받는 (repository 혹은 외부 서버 API호출) 로직을 데이터 객체에 책임을 맡겨야 하는가, 아니면 service에서 맡아야 하는가" 에 대해서 아직도 고민이 많습니다.
지금 작업 중인 Participant 또한 LiveKit서버와의 통신(LiveKitUtil), UserSpace에서 Profile Image를 가져오는 책임(UserDao, SpaceDao, UserSpaceDao 의존성) 이 전부 필요합니다.
현재는 VoiceRoomList와 ParticipantList를 최대한 일급 컬렉션으로 만들어 의존성을 없애는 방향으로 개발 중입니다.

개발 중간에 문제점이 발견되어 방향을 수정하거나, 개발이 끝나고 또 한번 피드백 진행하면서 더 나은 구조를 생각해봐야할 것 같습니다.

drbug2000 · 2024-10-07T08:02:20Z

LGTM! status가 active인 것까지 검색하려니 다 쿼리문을 쏴줘야 하는 게 아쉽네요.. 대안을 좀 찾아봐야겠어요

네 저도 찾는 중입니다... soft delete를 어떻게 일괄적을 예쁘게 적용할 수 있는지 고민중입니다.

seongjunnoh

서비스단의 로직들을 객체 분리를 통해 여러 객체들에게 위임하니 서비스단의 코드가 훨씬 간결해진 것 같습니다!!

작성해주신 테스트 코드에 대한 저의 개인적인 의견은 모킹을 하여 테스트를 진행하는 것이 아니라, 실제 테스트용 인메모리 db에 보이스룸 엔티티를 생성하고, 생성한 보이스룸 엔티티의 아이디 값을 비교하는 것이 좀 더 낫지 않을까 싶습니다.

실제 스프링 빈으로 주입된 객체들이 스프링 프레임 워크 상에서 협력을 통해 나오는 결과가 어떨지에 대한 테스트가 더 의미가 있다고 생각합니다.
그런데 스프링 통합테스트를 수행하기 위해서는 저희 프로젝트에서 주입해줘야하는 의존성이 너무 많아서 저도 고민중입니다,,,
이 부분은 좀 더 찾아보고 고민해보겠습니닷!

seongjunnoh · 2024-10-07T08:04:18Z

src/main/java/space/space_spring/controller/VoiceRoomController.java

-            roomInfoList= voiceRoomService.getVoiceRoomInfoListConcurrency(spaceId,voiceRoomList);
-        }
+
+        roomInfoList= voiceRoomService.getVoiceRoomInfoListConcurrency(spaceId,voiceRoomList);



비즈니스 로직을 전부 서비스단으로 넘겨버리니 컨트롤러의 코드가 훨씬 깔끔해진것 같습니다!!

seongjunnoh · 2024-10-07T08:05:05Z

src/main/java/space/space_spring/dao/UserSpaceRepository.java

+public interface UserSpaceRepository extends JpaRepository<UserSpace, Long> {
+    @Query("SELECT r FROM UserSpace r WHERE r.user.userId = :userId AND r.space.spaceId = :spaceId AND r.status = 'ACTIVE'")
+    Optional<UserSpace> findUserSpaceByUserAndSpace(@Param("userId") Long userId, @Param("spaceId") Long spaceId);
+


저도 UserSpaceDao의 코드를 작성하신 UserSpaceRepository 로 넘기도록 하겠습니다

네 다만, 꼭 id 값으로 Entity를 가져오는 것이 정답은 아닌 것 같아서 고민중입니다.
객체 간의 상관관계를 명확히 하는 용도로 Entity를 따로 가져오는 경우도 있다고 합니다.
추가적으로 함수이름에 Id를 포함시켜 헷갈리지 않게 수정해야겠네요

seongjunnoh · 2024-10-07T08:10:01Z

src/main/java/space/space_spring/dto/VoiceRoom/VoiceRoomDtoList.java

+import java.util.stream.Collectors;
+@Service
+@RequiredArgsConstructor
+public class VoiceRoomDtoList {


뭔가 해당 객체는 Data Transfer라는 DTO의 본질적인 존재 이유? 에 비해 많은 프로시저를 수행하기 때문에
아예 DTO 라는 네이밍을 제거하는게 어떤가요??
이 객체가 보이스룸 리스트를 만들고, 관리하는 역할을 수행하는거 같은데 VoiceRoomListProvider 추천 드립니닷

seongjunnoh · 2024-10-07T08:15:52Z

src/test/java/space/space_spring/domain/VoiceRoom/VoiceRoomServiceTest.java

+        //then
+        assertThat(voiceRoomId).isEqualTo(1L);
+
+    }


해당 테스트 코드가 VoiceRoom 엔티티가 어떤 역할을 수행하는 것을 테스트하기 위한 코드인지를 DIsplayName 에 명시적으로 작성해 주시면 보이스룸 도메인에 대한 지식 공유가 훨씬 쉬워질꺼 같습니다!

ex. ",,, 정보를 받아 VoiceRoom 객체를 생성할 수 있다" 이런 식으로 DisplayName 을 작성하는 건 어떤가요??

저번에 말씀해주신 것처럼 " 테스트 코드만 봐도 이 도메인의 기능과 흐름이 보여야한다" 라는 목적을 가지고 테스크 코드도 다시 한번 짜보겠습니다.
남은 participantService 코드와 도메인 리팩토링이 전체적으로 완료가 되면 본격적으로 남은 테스트 코드 작성할 계획입니다.

seongjunnoh

이 부분은 같이 얘기해보면 좋을거 같습니다!

seongjunnoh · 2024-10-07T08:28:16Z

src/main/java/space/space_spring/dto/VoiceRoom/VoiceRoomDtoList.java

+
+    }
+
+    private List<ParticipantDto> getParticipantDtoListById(long voiceRoomId){


음 개인적인 생각이긴 한데, 스프링 빈 객체(컴포넌트) 로 분리한 객체가 Repository 에 의존성을 가지는 것이 필수적이라면 이 방식도 괜찮지 않을까요??
서비스단의 하나의 메서드에서 너무 많은 책임을 가져서 객체를 분리하고, 해당 로직을 분리한 객체로 위임 & 책임을 분리한 로직이 repository 의존성을 가질 경우에는 이 방식도 괜찮지 않나 라는 생각입니다

혹시 레포지토리에 의존성을 가지는 객체를 하나 더 생성할 바에는 서비스단의 코드를 좀 더 무겁게 하고, 레포지토리 의존성을 서비스단만 가지도록 하는게 더 낫다고 생각하시나요??

seongjunnoh

LGTM

…omain/dto,repo Refactoring voice room 객체 정리 및 미사용 코드 삭제

drbug2000 added 10 commits September 23, 2024 16:27

[test] voice room service test : create voice room

3f509e1

[refactoring] add Todo comment

8567dec

[refactoring] change setActive func

c1cd862

[feat] ask roomList By roomNameList

1ab4d97

[refactoring] add convert func in RoomDto

55f682c

[refactoring] add set RoomDtoList code

0d866e1

apply on VoiceRoomService only set RoomDto not participant

[refactoring] delete not used import & func

875bc1a

[refactoring] Remove unused function in voiceRoom module

c47b3fd

Merge branches 'test/#155/voiceroom-domain-test' and 'refactoring/#158/…

22aa779

…voice-room-domain/dto,repo' of https://github.com/KUIT-Space/KUIT_Space_BackEnd into refactoring/#158/voice-room-domain/dto,repo

Merge branch 'develop' into refactoring/#158/voice-room-domain/dto,repo

23b3ad7

drbug2000 linked an issue Oct 4, 2024 that may be closed by this pull request

Refactoring : Voice Room Domain 리팩토링 #158

Closed

4 tasks

drbug2000 added 2 commits October 7, 2024 03:32

[fix] PostVoiceRoom.Request refactoring

08a841b

[refactoring] add id base dao in voiceRoom and UserSpace

dfb2a27

hyunn522 reviewed Oct 7, 2024

View reviewed changes

seongjunnoh reviewed Oct 7, 2024

View reviewed changes

[refactoring] rename VoicRoomDtoList->VoiceRoomListDto

6b9348c

seongjunnoh approved these changes Oct 9, 2024

View reviewed changes

drbug2000 merged commit e90ee3e into develop Oct 30, 2024
3 checks passed

seongjunnoh pushed a commit that referenced this pull request Oct 30, 2024

Merge pull request #162 from KUIT-Space/refactoring/#158/voice-room-d…

325cdc8

…omain/dto,repo Refactoring voice room 객체 정리 및 미사용 코드 삭제

hyunn522 deleted the refactoring/#158/voice-room-domain/dto,repo branch November 5, 2024 05:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring voice room 객체 정리 및 미사용 코드 삭제 #162

Refactoring voice room 객체 정리 및 미사용 코드 삭제 #162

drbug2000 commented Oct 4, 2024

hyunn522 left a comment

hyunn522 Oct 7, 2024

drbug2000 Oct 7, 2024

seongjunnoh Oct 7, 2024

hyunn522 Oct 7, 2024

drbug2000 Oct 7, 2024

seongjunnoh Oct 7, 2024

hyunn522 Oct 7, 2024 •

edited

Loading

drbug2000 Oct 7, 2024 •

edited

Loading

drbug2000 commented Oct 7, 2024

seongjunnoh left a comment

seongjunnoh Oct 7, 2024

seongjunnoh Oct 7, 2024

drbug2000 Oct 7, 2024

seongjunnoh Oct 7, 2024

seongjunnoh Oct 7, 2024

drbug2000 Oct 7, 2024

seongjunnoh left a comment

seongjunnoh Oct 7, 2024

seongjunnoh left a comment


		}

		private List<ParticipantDto> getParticipantDtoListById(long voiceRoomId){

Refactoring voice room 객체 정리 및 미사용 코드 삭제 #162

Refactoring voice room 객체 정리 및 미사용 코드 삭제 #162

Conversation

drbug2000 commented Oct 4, 2024

📝 요약

🔖 변경 사항

✅ 리뷰 요구사항

📸 확인 방법 (선택)

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

hyunn522 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyunn522 Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

drbug2000 Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

drbug2000 commented Oct 7, 2024

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

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

seongjunnoh left a comment

Choose a reason for hiding this comment

hyunn522 Oct 7, 2024 •

edited

Loading

drbug2000 Oct 7, 2024 •

edited

Loading