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

팀2 리뷰요청 부탁드립니다 #73

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

limbaba1120
Copy link
Collaborator

No description provided.

@xorwns118
Copy link
Contributor

xorwns118 commented Jul 26, 2024

@limbaba1120 limbaba1120 changed the title fix: [#72] 팀2 리뷰요청 부탁드립니다 [#72] 팀2 리뷰요청 부탁드립니다 Jul 26, 2024
@limbaba1120 limbaba1120 changed the title [#72] 팀2 리뷰요청 부탁드립니다 팀2 리뷰요청 부탁드립니다 Jul 26, 2024
@yoowonyoung
Copy link

질문: 현업에서는 패키지 구조를 어떻게 구성하는지 궁금합니다.

이 질문은 어떤 아키텍처를 적용 하였느냐에 따라 다릅니다. 전형적인 계층형 아키텍처를 이용 한다면 프리젠테이션 패키지 / 서비스 패키지/ 데이터 엑세스 패키지등으로 구분한 후 각 패키지 내에서 세부 도메인로 구분을 할수도 있을 것입니다. 또, 헥사고날 아키텍처를 사용 한다면 도메인 패키지 / 인프라 스트럭처 패키지 등으로 구분해면서 세부적으로 포트 패키지, 어댑터 패키지등도 추가할수 있겟죠. 이것과 관련해서는 헥사고날 아키텍처를 공부해보면 좋을것 같아요.

https://tech.osci.kr/hexagonal-architecture/
https://tech.kakaobank.com/posts/2311-hexagonal-architecture-in-messaging-hub/

@yoowonyoung
Copy link

질문 : 컨트롤러를 생성하고 관리하는 방법

일단 기본적으로 너무 큰 컨트롤러는 지양하는것이 좋습니다. 리팩토링을 위한 대표적인 Bad Smell중 하나가 Larger Class니까요. 그렇다면 얼마나 세부적으로 나눌것이냐~ 인데, 이는 저는 이것을 세부 도메인별로 나눠서 관리 하는것을 선호 합니다.
User에 관한것이라도 User에 대한 기본 CRUD를 하는 UserBasicController / User의 MyPage관련 기능을하는 UserMyPageController 이런식으로요. 대신 이름은 너무 Common한것을 사용하는것은 피해야 합니다. 지금 질문주신 케이스 자체가 딱, UserBasicController 가 되어야하는데 UserController로 되어있어서 너무 포괄적 의미를 가지게 되어서 그런것 같아요.

@yoowonyoung
Copy link

질문 : 제공되지 않는 API 데이터는 어떤 방식으로 직접 수집하고 추가 하는 방법

Spring에 존재하는 Spring scheduler나 Spring batch를 이용해서 batch Job을 만들어서 주기적으로 갱신이 되도록 구성 하는게 일반적입니다.

@yoowonyoung
Copy link

질문: 메소드의 반환 타입이 List 로 넘기고 있는데 좀 더 좋은 방법이 있는지 궁금합니다.

따로 응답객체를 만들어서 필요한 정보만 반환 할 수 있도록 구성하는것이 좋습니다! Entity를 서비스 계층 이상으로 노출시킨다면, Entity가 수정될 위험이 있기 때문에, 응답 객체를 만들어서 반환 하는걸 추천 합니다~

@yoowonyoung
Copy link

먼저 질문에 대한 코멘트를 남겨두었고, 전체적인 리뷰 추가로 작성할예정입니다~

@yoowonyoung
Copy link

테스트 코드가 없음

불필요한 주석이 남아있음

무의미한 코드 (https://github.com/limbaba1120/hackathon2-RecoMovie/blob/6a357066d3511426369fed54b225bc49bd59a7c1/src/main/java/com/hackathonteam2/recomovie/admin/controller/adminRestController.java#L48)

Entity를 그대로 Controller에 노출 시키는것이 아닌, DTO를 사용해서 DTO를 노출시키는것은 좋지만 현재 코드에서는 별 의미가 없는 것으로 보임. 도메인 모델의 완전성과 순수성에 관한 논쟁은 항상 고민거리이니 읽어보면 좋을것(https://velog.io/@leejh3224/%EB%B2%88%EC%97%AD-%EB%8F%84%EB%A9%94%EC%9D%B8-%EB%AA%A8%EB%8D%B8-%EC%88%9C%EC%88%98%EC%84%B1-vs-%EB%8F%84%EB%A9%94%EC%9D%B8-%EB%AA%A8%EB%8D%B8-%EC%99%84%EC%A0%84%EC%84%B1)

Region별 영화관 목록을 JSON파일로 다루던데 기왕이면 DB를 쓰는게 낫지 않을까요

전체적으로 예외처리가 메서드에서 throws를 사용하고 말고 있습니다. ControllerAdvice를 사용하고 있지도 않는것으로 보이는데, 예외 처리를 좀 더 신경쓰면 좋을것 같습니다

Long parameter list 등 리팩토링을 위한 Bad smell들이 보입니다. 전반적인 점검이 필요 해보입니다

MovieRepository 는 아무런 메서드가 없습니다. Paging을 하지도 않는것 같은데, JpaRepository를 쓸 필요가 있을까요?

native query를 사용할 필요가 있을까요? (https://github.com/limbaba1120/hackathon2-RecoMovie/blob/6a357066d3511426369fed54b225bc49bd59a7c1/src/main/java/com/hackathonteam2/recomovie/review/repository/ReviewRepository.java#L13)

@yoowonyoung
Copy link

일단 눈에 띄는것은 이정도네요~ 추가적인 코멘트가 필요한 부분이 있다면 말씀 해주세요~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants