feat: add gon code review project - #49
Conversation
geniusjun
left a comment
geniusjun
left a comment
There was a problem hiding this comment.
안녕하세요 곤! 코드 잘 봤습니다.
저는 UMC 8,9기 스프링 파트를 수료한 노을입니다.
구현 자체는 정말 빠르게 할 수 있는 시대이기에, 코드 스타일이나 표현보다는 아키텍처 관점에서 고민해볼 만한 내용들을 중심으로 코멘트를 남겨봤습니다.
결국 AI를 잘 활용하는 사람과 그렇지 않은 사람의 차이는, 단순히 코드를 생성하는 것이 아니라 전체 구조를 설계하고 판단할 수 있는 능력에서 나온다고 생각합니다.
이번 기회에 리뷰 내용을 하나씩 찾아보고 고민해보시면 분명 많은 도움이 될 거예요. 궁금한 점이 생기면 언제든 편하게 질문 주셔도 됩니다!
데모데이 준비로 많이 바쁘시겠지만, AI를 활용해 빠르게 구현하면서도 한 발짝 뒤에서 전체 아키텍처를 바라볼 수 있는 엔지니어가 되시길 응원하겠습니다. 😊
| import org.springframework.security.web.SecurityFilterChain; | ||
|
|
||
| @EnableWebSecurity //Spring Security를 활성화 | ||
| @Configuration |
There was a problem hiding this comment.
클래스에 @configuration 어노테이션을 붙히면 이 클래스는 빈들을 관리하는 특수 컨테이너 클래스 취급이 됩니다.
따라서 아래 있는 @bean 객체들은 Spring 프레임워크가 런타임시에 커스텀 빈으로 등록하여 관리합니다.
그렇다면 Spring 프레임워크가 빈을 등록하는 방식은 어떻게 될까요?!
-> 클래스명으로 등록해놓고(securityconfig) 프록시를 생성하여 스프링 AOP의 이점을 극대화 합니다.
하지만 현재 빈으로 등록할 클래스명이 많이 겹칩니다. config/SecurityConfig.java, security/SecurityConfig.java
이렇게 되면 스프링이 어떤 빈을 런타임시에 객체에 주입시켜야할지 헷갈려 합니다!
| @RequestMapping("/v1") | ||
| public class MissionController { | ||
|
|
||
| private final MissionServiceImpl missionService; |
There was a problem hiding this comment.
현재 MissionController가 구현체인 MissionServiceImpl를 의존하고 있습니다. MissionService 인터페이스의 존재 이유가 희미해집니다.
관련해서 DIP에 대해서 알아보시면 좋을 것 같습니다.
| @Service | ||
| @RequiredArgsConstructor | ||
| @Transactional | ||
| public class AuthService { |
There was a problem hiding this comment.
다른 도메인들은 인터페이스 -> 구현체 형식이던데, Auth 도메인만 구현체로 구성되어 있습니다.
구현체 하나로 충분할 것 같다면 지금과 같이 인터페이스를 두지 않는 것이 코드 두께를 줄일 수 있는 실용적인 선택이라고 봅니다.
하지만 이런 클래스가 하나씩 늘어날 수록 코드 일관성은 떨어질 것이라고 생각하는데, 같은 방식으로 통일하자는 팀원의 말에 곤님은 어떻게 답변할 것인가요!
| private final UserService userService; | ||
|
|
||
| //마이페이지 | ||
| @PostMapping("/users/me") |
There was a problem hiding this comment.
getInfo인데, POST method인 것이 조금 어색합니다. 보편적으로 GET이라고 생각이 드는데, 의도된 바가 있을까요?
| String reviewComment, | ||
| @NotNull(message = "별점 평가는 필수입니다.") | ||
| @Min(value=1,message="별점은 최소 1점 이상이어야 합니다. ") | ||
| @Max(value=5,message="벌점은 최대 5점입니다.") |
There was a problem hiding this comment.
DTO의 역할은 무엇이라고 생각하시나요? DTO는 Data Transfer Object입니다.
별점이 1~5점이라는 도메인 제약조건이 이렇게 데이터 전송 계층에서 새롭게 추가되는게 어색해 보입니다.
더군다나 엔티티쪽에는 이런 제약조건이 없어 Review.builder().star(100).build()로 별점 100점도 가능할 것 같습니다.
프로젝트 설계 초반에 열심히 데이터 정합성을 지켜가며 Entity 설계할텐데, 이를 지키는 쪽으로 코드 설계하면 더 좋을 것 같습니다.
| // 유저 객체에서 정보 추출 | ||
| SocialType providerId; | ||
| String socialUid; | ||
| Map<String, Object> attributes = oAuthMember.getAttribute("kakao_account"); |
There was a problem hiding this comment.
SocialType에 GOOGLE, FACEBOOK 등 다른 타입도 설정해놓은 것을 보아 추후에 다른 로그인도 구현할 것으로 보입니다.
그렇다면 이 AuthService를 추상화 시킬 필요가 있습니다. 현재 바로 kakao_account가 하드코딩 되어있어 추상화 시키기 어려워질 수 있습니다.
나중에 구글 로그인 추가할때 다른 구현체를 계속 추가하는 식으로 진행해도 괜찮겠지만, 이렇게 되면 클래스명도 바꿀 필요가 있어 보입니다.
| package com.example.umc10th.domain.store.exception; | ||
|
|
||
| public class StoreException extends RuntimeException { | ||
| public StoreException(String message) { |
There was a problem hiding this comment.
다른 예외 코드들은 BaseErrorCode로 던지기에 4xx, 3xx 이런식으로 잘 에러코드가 보일텐데, Store도메인의 에러코드만 String이여서 500으로 던져질 것 같습니다.
| AuthMember authMember = new AuthMember(user); | ||
|
|
||
| String accessToken = jwtUtil.createAccessToken(authMember); | ||
| String refreshToken = jwtUtil.createRefreshToken(authMember); |
There was a problem hiding this comment.
저는 이 AuthService가 너무 많은 것을 알고 있다고 생각합니다.
jwtUtil.createRefreshToken()를 호출하면서 Security 클래스안에 어떤 토큰을 쓰는지(어떻게 만드는지까지) 알고 있습니다.
AuthService의 "역할은 이메일/비밀번호가 맞는지 확인하고, 세션(토큰)을 내어준다" 정도면 충분할 것 같습니다.
| throw new AuthException(AuthErrorCode.INVALID_PASSWORD); | ||
| } | ||
|
|
||
| AuthMember authMember = new AuthMember(user); |
There was a problem hiding this comment.
AuthMember는 Security쪽의 인증 결과를 담은 객체로 쓰이는데, 이 객체를 서비스로직에서 생성해서 jwtUtil에 넘기고 있어요!
서비스는 인증이 필요한 시점에 이메일이나 비밀번호를 인증을 담당하는 쪽(Security)에 넘겨서 그쪽에서 생성하게끔 하는 것이 좋아보입니다!
결론적으로 하고 싶은 말은 "역할과 책임을 분리해서 의존성의 방향을 단방향으로 바꿔보자!" 입니다. 조금 더 찾아보시면서 공부해보시면 큰 도움이 될 것 같아요!
2 participants
📂 관련 이슈
🛠️ 작업 사항