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

[Feat] 설정 연결된 계정 API #317

Merged
merged 4 commits into from
May 7, 2023
Merged

[Feat] 설정 연결된 계정 API #317

merged 4 commits into from
May 7, 2023

Conversation

heoboseong7
Copy link
Collaborator

관련 Issue

작업 내용

  • 연결된 계정 조회 API 추가

Figma 링크

image

@heoboseong7 heoboseong7 added enhancement New feature or request feature labels May 6, 2023
@heoboseong7 heoboseong7 self-assigned this May 6, 2023
@github-actions
Copy link

github-actions bot commented May 6, 2023

테스트통과 🤖

Copy link
Collaborator

@Youhoseong Youhoseong left a comment

Choose a reason for hiding this comment

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

연결된 계정이니 풍부하게 ConnectedAccount라고 표현해봐도 좋겠네요.

@Getter
@AllArgsConstructor(staticName = "of")
public class AccountResponse {
private String vendorType;
Copy link
Collaborator

Choose a reason for hiding this comment

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

VendorType이라는 ENUM을 그대로 활용해도 되지 않을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

말씀해주신 방법을 사용하면 명확해지긴 하지만, Presentation 레이어의 Response DTO와 Domain 레이어의 VendorType을 직접 사용하게 되는데 괜찮을까요?

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
Collaborator Author

Choose a reason for hiding this comment

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

현재 Domain 모듈과 종속성이 있는 부분이 ApiService와 Assembler인데 Response DTO에도 종속성이 생겨요

Copy link
Collaborator

Choose a reason for hiding this comment

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

아 그렇게 생각하실수도 있겠네요. 저는 아래와 같이 생각해요

이미 presentation 영역이 domain에 의존하는건 우리가 의존성 방향을 그렇게 정했어요. (build.gradle)
특정 클래스에 종속, 의존 역할을 몰아넣어서 도메인에 의존해야하는 부분을 제한하고 있다고 생각하지 않아요.

모듈에서 단지 assembler는 converter 역할. api service는 aggregator 역할을 한다고 생각해요.
그래서 결론적으로 모듈 전체에서 도메인에 의존하는 것이 뭐가문제? 냐는 답변이 나오는거같네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아하 무슨 말씀인지 이해했어요.
다른 곳에서도 이미 동일한 형태로 사용하는 부분이 있기도 해서 VendorType 직접 사용으로 수정했어요.


여기서부터는 저의 생각인데요.
의존이 발생하는 영역을 제한하는 것이 좋다고 생각해요.
그 이유는 상위 모듈 전반적으로 하위 모듈에 의존하게 된다면, 나중에 하위 모듈의 변화가 상위 모듈에 전반적으로 영향을 미칠 수 있다는 걱정 때문이에요.

DTO의 생성자에서 Entity를 직접 받지 않고 Assembler라는 converter를 추가한 이유도 DTO에서 하위 모듈의 의존성이 발생하는 것을 막기 위함이었어요.

위에선 문제점만 얘기했지만, 어떻게 보면 하위 모듈의 변화를 별도의 작업 없이 상위 모듈에 반영할 수 있다는 장점도 있어보여요 ㅎㅎ

정답이 있는 문제 같진 않아서 저의 생각 공유? 정도로 봐주시면 좋을 것 같아요!

Copy link
Collaborator

@Youhoseong Youhoseong May 7, 2023

Choose a reason for hiding this comment

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

네.

저는 정말 중요한건 변할 수 있는 것과 변하지 않을 것을 구분하는 거라고도 생각해요

모든 걸 변할 수 있는 것으로 취급하고 의존을 떼어내려고만 하면 오히려 많은 불편함을 가져오는 것 같아요. (저는 요새 Assembler가 그러한 존재라고 생각하기도 해요.) 의존을 떼어내려고 시작했지만, 코드를 작성하는데 있어서는 강하게 정해진 구조와 보일러플레이트에 지치기도 하는 것 처럼..

=> 이 관점을 많이 반영한 아키텍쳐도 존재하기도 하죠.

적절한 타협(지름길), 그리고 상황에 따른 판단도 중요하다고 생각해요.

  1. 위 관점에서 우리는 Entity 객체와 DTO를 분리하는 것은 불편함을 불러오지만 불편함보단 유연함이 가져오는 장점이 크다고 생각해요. (그러니까 하고있겠죠??)

  2. 위 관점에서 도메인에 정의된 Enum을 DTO의 세부 필드 중 하나로 쓰지 않는건 변화에 유연한걸까?
    => 글쎄...

그래서 제 생각의 결론은 이러해요.

의존을 발생하는 영역을 분리하고 싶다면. => 이렇게 헷갈려하는 상태를 냅두기보다는 모듈 단위로 분리해서 제대로 제한해보는게 좋아보여요.
하지만.. 이정도는 타협하자.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좋은 내용이네요 ㅎㅎ

저도 1번처럼 Entity와 DTO 분리는 엄격해야 한다고 생각해요.
하지만 Enum과 같은 value type들은 충분히 허용 가능할 것 같아요. String같은 타입보다 의미가 명확해지기도 하구요.

이런 방향에서 Assembler는 확실히 과해 보이긴 하네요.
만약 Assembler를 제거한다면 from 팩토리 메소드로 같은 방법으로도 충분히 대체 가능해보여요.

@github-actions
Copy link

github-actions bot commented May 7, 2023

테스트통과 🤖

@github-actions
Copy link

github-actions bot commented May 7, 2023

테스트통과 🤖

@heoboseong7 heoboseong7 merged commit a183c99 into main May 7, 2023
@heoboseong7 heoboseong7 deleted the issue-#306 branch May 7, 2023 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feat] 설정 연결된 계정 API
2 participants