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: 공동 모임장 구현 #449

Merged
merged 26 commits into from
Oct 13, 2024
Merged

feat: 공동 모임장 구현 #449

merged 26 commits into from
Oct 13, 2024

Conversation

mikekks
Copy link
Member

@mikekks mikekks commented Oct 9, 2024

👩‍💻 Contents

모임 상세 조회

스크린샷 2024-10-12 오후 2 48 49

내가 만든 모임 조회

스크린샷 2024-10-12 오후 2 43 15

📝 Review Note

  • 내 모임 조회 로직에서, 공동모임장인 모임들을 불러오는 쿼리 가 필요할 것 같은데 이것까지 넣으면 너무 PR 이 커질 것 같아서 다음 PR에 구현해보도록 하겠습니다!

📣 Related Issue

✅ 점검사항

  • docker-compose.yml 파일에 마이그레이션 한 API의 포워딩을 변경해줬나요?
  • Spring Secret 값을 수정하거나 추가했다면 Github Secret에서 수정을 해줬나요?
  • Nestjs Secret 값을 수정하거나 추가했다면 Docker-Compose.yml 파일 및 인스턴스 내부의 .env 파일을 수정했나요?

@mikekks mikekks added the 🎁 feature 새로운 기능 label Oct 9, 2024
@mikekks mikekks self-assigned this Oct 9, 2024
Copy link

height bot commented Oct 9, 2024

Link Height tasks by mentioning a task ID in the pull request title or commit messages, or description and comments with the keyword link (e.g. "Link T-123").

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

Copy link
Member

@hoonyworld hoonyworld left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 민규님!
공동 모임장 기능을 잘 구현하신 것 같습니다!

다만, MeetingV2ServiceImpl에서 다양한 도메인의 레포지토리가 의존하고 있는데, 예외처리가 적절히 구현되지 않는다면 위험한 접근 방법이라고 생각이 듭니다.

직접 레포지토리를 의존하는 것 보다는, 서비스 레이어의 메서드 내에서 예외처리를 구현해서 해당 도메인의 서비스 레이어를 의존하도록 하는게 그 방법이 될 수 있을 것 같습니다.
고생하셨습니다!

@mikekks
Copy link
Member Author

mikekks commented Oct 12, 2024

고생하셨습니다 민규님! 공동 모임장 기능을 잘 구현하신 것 같습니다!

다만, MeetingV2ServiceImpl에서 다양한 도메인의 레포지토리가 의존하고 있는데, 예외처리가 적절이 구현되지 않는다면 위험한 접근 방법이라고 생각이 듭니다.

직접 레포지토리를 의존하는 것 보다는, 서비스 레이어의 메서드 내에서 예외처리를 구현해서 해당 도메인의 서비스 레이어를 의존하도록 하는게 그 방법이 될 수 있을 것 같습니다. 고생하셨습니다!

리뷰 감사합니다!!
꼼꼼하게 리뷰 해주신 것 같아서 감사드립니다 ㅎㅎ

해당 리뷰 중 "MeetingV2ServiceImpl에서 다양한 도메인의 레포지토리가 의존하고 있는데, 예외처리가 적절이 구현되지 않는다면 위험한 접근 방법이라고 생각이 듭니다." 내용에 대해 얘기해보고 싶습니다!

  1. 말씀 중에 "예외처리가 적절이 구현되지 않는다면 위험한 접근 방법" 라는 얘기를 좀 더 풀어서 말씀해주실 수 있을까요??

  2. 저는 서비스가 다른 서비스를 의존하기 보다는 레이어를 하나 더 만드는 것을 제안하고 싶습니다! 서비스가 다른 서비스를 의존하게 되면 순환참조를 하기 쉬운 환경이라고 생각합니다! 그래서 서비스 레이어와 레포지토리 레이어 중간에 기본적인 CRUD 기능을 하는 레이어가 있으면 좋을 것 같습니다! 예를 들어, facade-service 또는 Usecase-service 관계라고 할까요?
    하지만 이런 구조의 경우 트레이드 오프로 코드의 복잡성이 매우 늘어난다고 생각합니다! 그래서 제가 1번에서 어떤 이유로 위험하다고 생각하시는지 궁금합니다!

@hoonyworld
Copy link
Member

hoonyworld commented Oct 12, 2024

  • 말씀 중에 "예외처리가 적절이 구현되지 않는다면 위험한 접근 방법" 라는 얘기를 좀 더 풀어서 말씀해주실 수 있을까요??
  • 저는 서비스가 다른 서비스를 의존하기 보다는 레이어를 하나 더 만드는 것을 제안하고 싶습니다! 서비스가 다른 서비스를 의존하게 되면 순환참조를 하기 쉬운 환경이라고 생각합니다! 그래서 서비스 레이어와 레포지토리 레이어 중간에 기본적인 CRUD 기능을 하는 레이어가 있으면 좋을 것 같습니다! 예를 들어, facade-service 또는 Usecase-service 관계라고 할까요?
    하지만 이런 구조의 경우 트레이드 오프로 코드의 복잡성이 매우 늘어난다고 생각합니다! 그래서 제가 1번에서 어떤 이유로 위험하다고 생각하시는지 궁금합니다!

1번

위험하다고 말한 상황을 풀어서 설명하자면, 예외 처리가 누락될 경우 다른 도메인 및 상위 계층에까지 영향을 미칠 수 있다는 의미였습니다!

예를 들어, 특정 도메인의 서비스 레이어에서 레포지토리를 접근할 때 예외 처리가 구현되어 있다면, 해당 서비스 레이어를 통해 호출할 경우에는 예외 처리를 따로 신경 쓰지 않아도 됩니다. 하지만 레포지토리를 직접 호출하는 경우에는 이러한 예외 처리가 누락될 수 있습니다.

이로 인해 예외 발생 시 상위 계층에서 예외 상황에 적절하게 대응하지 못하게 되어, 전체적인 비즈니스 로직의 안정성을 해칠 위험이 있을 것 같다고 생각이 듭니다

2번

맞습니다 서비스 순환참조가 발생할 수 있습니다. 그래서 저도 서비스 facade나 유즈케이스를 도입하면 좋을 것 같다고 생각이 드는데요!!
트레이드 오프에 대해서의 제 의견은 메이커스의 집단 특성상 장기적인 안정성, 유지보수성, 확장성 측면에서 얻는 이점이 크기 때문에, 코드 복잡성이 증가하는 단점을 상쇄한다고 생각합니다.

@mikekks mikekks merged commit 01dbc3a into develop Oct 13, 2024
1 check passed
@mikekks mikekks deleted the feat/#447-2 branch October 13, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎁 feature 새로운 기능 size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: 공동 모임장 구현
2 participants