[Code Review] 환이 code review #17
Conversation
There was a problem hiding this comment.
환이님께서 어느정도의 개발경험을 가지고 계신지는 잘은 모르겠습니다.
고민 포인트, 질문에 대해 남기라고 직접 말까지 남겼는데 없으셔서 눈에 띄는 포인트만 리뷰 남겼습니다.
그리고, 개인적인 느낌을 적어보도록 할게요.
-
커멋의 단위
혹시 커밋은 어느정도 단위로 작성하면 좋을지 고민해보신 적 있을까요?
커밋의 단위에 대해 찾아보면 좋을 것 같아요.
참고로, 현재 커밋해주신 단위는 리뷰어들이 보기에는 좋지 않은 단위입니다.
리뷰시 커밋의 내용을 보고 어떠한 작업을 했는지 맥락을 파악할 수 있어야 하거든요 -
오버엔지니어링
코드에 보이는 거로는 CQRS, AOP, QueryDSL 등 초심자에는 적합하지 않은 기술을 사용해주신 것을 볼 수 있어요.
제가 워크북 내용은 모릅니다만, 워크북에서 강제로 하는 부분이라면 어쩔 수 없이 해야겠죠.
다만, 도구나 기술을 사용할 때 맥락과 이유를 명확히 알고 사용하면 좋을 것 같아요.
지금은 공부하는 입장이니 이것저것 경험해보면 좋겠지만 그런 내용에 대해 숙지하는게 필요해요.
관련해서 질문 드리자면,
- CQRS 패턴을 지금 서비스 규모에서 필요한 선택이었을까요?
- QueryDSL을 쓸 정도의 복잡하고, 동적인 쿼리 핸들링이 필요했을까요?
- 네이밍, 패키징
크게 중요하게 느끼지 않을 수 있지만, 중요한 내용이어서 짚고 가겠습니다.
협업을 하게 되면, 변수 네이밍, 함수 네이밍을 잘 짓는게 굉장히 중요합니다.
혼자 작업을 하면 다 아는 내용이다보니 괜찮을 겁니다.
그러나, 다른 동료들과 작업을 하게 된다면, 다른 동료가 봐도 이해가 잘 되는 네이밍인지 고민해보면 좋을 것 같습니다.
패키징도 중요하게 관리해야 합니다.
관련해서는 객체지향에 대해 공부해보시면 좋을 것 같은데요.
클래스를 작성하고 함수를 작성할때도 SRP를 지키는게 힘들더라도, 적절한 단위의 책임을 가지고 있는지
God객체가 만들어지지는 않았는지, 변경에 유연한 구조를 가지고 있는지 등등
코드가 변화에 유연한 구조를 가지고 있는지도 고민해보고 공부해보면 좋을 것 같습니다.
| @Getter | ||
| @Builder | ||
| @NoArgsConstructor(access = AccessLevel.PROTECTED) | ||
| @AllArgsConstructor |
There was a problem hiding this comment.
@NoArgsConstructor에는 PROTECTED 되어 있는데 @AllArgsConstructor에는 패키지 접근에 관한 내용이 없네요.
혹시 이런 어노테이션이 왜 사용되는지 알고 있을까요?
|
|
||
| @Enumerated(EnumType.STRING) | ||
| @Column(columnDefinition = "VARCHAR(15) DEFAULT 'OPTIONAL'") | ||
| private RequiredYN requiredYN; |
There was a problem hiding this comment.
클래스명, 변수의 네이밍이 좋은 변수명인 것 같지는 않아요. 네이밍에 관해 고민해보면 좋을 것 같아요
| @@ -0,0 +1,5 @@ | |||
| package umc.study.domain.enums; | |||
|
|
|||
| public enum DelYN { | |||
| use_sql_comments: true | ||
| hbm2ddl: | ||
| auto: create | ||
| default_batch_fetch_size: 1000 |
There was a problem hiding this comment.
JPA 옵션들이 많이 설정되어 있네요. 각각의 옵션들이 무엇을 의미하는지 알고 계신가요?
| runtimeOnly 'mysql:mysql-connector-java:23.0.1' // MySQL 커넥터 추가 | ||
| annotationProcessor 'org.projectlombok:lombok' | ||
| implementation 'org.springframework.boot:spring-boot-starter-data-jpa' | ||
| implementation 'org.hibernate.orm:hibernate-core:6.0.2.Final' // Hibernate 6.0.2 이상 |
| auto: create | ||
| default_batch_fetch_size: 1000 |
| @@ -25,7 +25,7 @@ public class Question extends BaseEntity { | |||
| @Column(columnDefinition = "VARCHAR(15) DEFAULT 'NOT_DELETED'") | |||
| private DelYN delYN; | |||
|
|
|||
| @ManyToOne(fetch = FetchType.LAZY) | |||
| @ManyToOne(fetch = FetchType.EAGER) | |||
| @@ -36,4 +41,31 @@ public static Member toMember(MemberRequestDTO.JoinDto request){ | |||
| .age(request.getAge()) | |||
| .build(); | |||
| } | |||
|
|
|||
| // 내 리뷰 목록 조회 | |||
| public static MemberResponseDTO.MyReviewPreviewDTO myReviewPreviewDTO(Review review){ | |||
There was a problem hiding this comment.
변환하는 목적의 메소드로 보이는데 네이밍이 단순 변수명에서 사용하는게 적합한 네이밍으로 보이네요. 의도가 있으실까요?
|
|
||
| @RestController | ||
| @RequiredArgsConstructor | ||
| @Validated |
There was a problem hiding this comment.
이 어노테이션은 무엇을 의미하는지, 왜 사용하시는지 알 수 있을까요?
| ) | ||
| .logout((logout) -> logout | ||
| .logoutUrl("/logout") | ||
| .logoutSuccessUrl("/login?logout") |
No description provided.