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

refactor : 애그리거트 재 설계로 인한 패키지 조정 #83

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

waterricecake
Copy link
Contributor

@waterricecake waterricecake commented Jul 18, 2024

close #82
작업단위가 너무 커져서 우선적으로 의존성 분리전 패키지 분리 pr부터 올려봅니다.
의존성 분리의 경우 레디스 도입과 같이 진행을 해야할 것 같아 우선적으로 분리된 패키지부터 같이 보자는 의미에서 올려봅니다.

변경 사항
어그리거트 별 패키지 분리
패키지 구조

  • domain
  • presentation
  • service
  • repository
  • dto
    이렇게 구현되어있습니다.

@thdwoqor
Copy link
Contributor

thdwoqor commented Jul 18, 2024

레포지토리 위치 변경

리포지터리 인터페이스는 애그리거트와 같이 도메인 영역에 속하고, 리포지터리를 구현한 클래스는 인프라스트럭터 영역에 속한다.

└── room
    ├── domain
        ├── Room.class
        ├── RoomRepository.class

위와 같은 형태는 어떨까요?

https://incheol-jung.gitbook.io/docs/study/ddd-start/4-jpa
https://velog.io/@hgo641/Repository는-도메인-계층일까-영속성-계층일까

패키지 네이밍 변경

image

└── room
    ├── ui(controller)
    ├── application(service)
    ├── domain
    ├── infra

ddd에서 레이어를 위와 같이 나누는거같은데 패키지 네이밍을 위와 같이 하는것은 어떨까요?

https://github.com/woowacourse/service-apply/tree/master/src/main/kotlin/apply
https://wangmin.tistory.com/52

DTO 패키지

저는 개인적으로 DTO 패키지 위치는 사용하는 Layer에 있어야한다고 생각해요
각 Layer별로 나오는 DTO를 한 패키지에 모아버리면 패키지가 너무커지고 유지보수가 어렵다고생각
그래서 보통 Controller-Service 사이에 Request-Response DTO는 service에 두는편인데 어떻게 생각하시나요?

https://github.com/woowacourse/service-apply/tree/master/src/main/kotlin/apply/application

Copy link
Contributor

@thdwoqor thdwoqor left a comment

Choose a reason for hiding this comment

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

고생 많았어 👍

@@ -1,4 +1,4 @@
package mafia.mafiatogether.domain;
package mafia.mafiatogether.room.util;
Copy link
Contributor

Choose a reason for hiding this comment

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

개인적인 생각으로는 CodeGenerator는 우리 비즈니스 로직이 들어가있다고생각해서 domain 패키지에 넣는것이 어떻까요?

    private final static int TARGET_STRING_LENGTH = 10;

클래스명이나 해당 부분에서도 저희가 정한 코드 10자가 비즈니스 규칙이라고 생각하는데
해당 클래스가 유틸클래스가되려면 숫자를 입력받고 거기에 해당하는 랜덤 문자열을 뽑아내야하지않을까요? (추가로 이런 느낌의 클래스명)

Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 CodeGenerator는 도메인에 포함되어야 한다고 생각해요!

Copy link
Collaborator

@kpeel5839 kpeel5839 left a comment

Choose a reason for hiding this comment

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

image

PR 타이틀에 조금 더 해당 PR의 의도가 드러나면 좋을 것 같아요! 가령 애그리거트 재 설계로 인한 패키지 조정 이런 식으로요?! (그렇게 중요한 것은 아니라고 생각하지만, 나중에 해당 PR을 볼 때 이게 뭐였지?! 라고 생각할 수도 있을 것 같아서 말씀드려요!)

그리고 항상 앞장서서 솔선수범 해주시는 것 항상 감사하게 생각합니다. 🥰

제 의견도 파워랑 비슷한 것 같아요! ui, application, 이런 식으로 나누는 것은 거기서 거기인 것 같긴하지만, 현재 Repository가 Domain에 위치해야 한다는 점이요.

DDD 책을 읽어보면, Service쪽에서 infra쪽을 의존하지 않기 위해, 의존 역전을 활용하여 domain쪽에 repository interface를 두고, infra 딴에서 이를 구현을 많이 하고는 하는데, 이를 저희의 상황에 대입해서 생각해보면, 사실 구현체는 JPA가 구현해주고 있으니, 따로 infra 패키지를 둘 필요도 service와 repository 패키지를 구분할 필요도 없다고 봅니다.

Dto도 비슷하게 생각해요, Presentation 영역에 두게 되면, Service에서 Presentation 영역에 의존하게 되니, Service 딴에 두어야 한다고 생각하고, 위에서 말씀드렸던 repository와 비슷한 논리로 Service와 Dto를 구분할 필요는 없다고 생각해요!

말씀드린 모든 사항은 개인적인 생각이니 한번 생각해보시고 다시 의견주시면 감사할 것 같아요!
다시 한번 고생했습니다. 달달달리~~~


public record RoomStatusResponse(
StatusType statusType

Copy link
Collaborator

Choose a reason for hiding this comment

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

공백!
물론 중요한 건 아니지만용 호호호

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -1,4 +1,4 @@
package mafia.mafiatogether.domain;
package mafia.mafiatogether.room.util;
Copy link
Collaborator

Choose a reason for hiding this comment

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

저도 CodeGenerator는 도메인에 포함되어야 한다고 생각해요!

@waterricecake waterricecake changed the title Refactor/82 애그리거트 별 패키지 분리 Jul 18, 2024
@waterricecake waterricecake changed the title 애그리거트 별 패키지 분리 refator/82 애그리거트 별 패키지 분리 Jul 18, 2024
@waterricecake waterricecake changed the title refator/82 애그리거트 별 패키지 분리 refactor/82 애그리거트 별 패키지 분리 Jul 18, 2024
@waterricecake
Copy link
Contributor Author

@thdwoqor @kpeel5839 이렇게 하니까 더 깔끔해 보이네요! 계층별로 나뉘어서 한눈에 보기 쉬운것 같습니다! 역시 뭘 더하기 전에 패키지 정리를 해본것이 좋았던거 같네요!

Copy link
Contributor

@thdwoqor thdwoqor left a comment

Choose a reason for hiding this comment

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

👍

pr제목 근데 통일성있게 하면 좋을듯?
image
refactor: 애그리거트 별 패키지 분리
이런 식이나 다른 방식이더라도 통일성있게?

@waterricecake waterricecake changed the title refactor/82 애그리거트 별 패키지 분리 refactor : 애그리거트 재 설계로 인한 패키지 조정 Jul 19, 2024
@waterricecake waterricecake merged commit 1504f3f into dev Jul 19, 2024
1 check passed
@waterricecake waterricecake deleted the refactor/82 branch October 14, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

에그리거트 단위로 패키지 리팩토링
3 participants