-
Notifications
You must be signed in to change notification settings - Fork 5
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
[review] f1-washpedia #144
base: refactor/review
Are you sure you want to change the base?
Conversation
YunNote
commented
Feb 17, 2024
- 리뷰 진행
@@ -20,8 +20,12 @@ | |||
public class CommonCodeController { | |||
|
|||
private final CommonCodeService commonCodeService; | |||
|
|||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
만약 버저닝을 해야한다면 어떻게 관리할 수 있을까요?
URL로도 관리할 수 있지만 header를 통해서도 관리하는 방법에 대해 보시면 좋을것 같습니다!
@GetMapping("/{codeName}") | ||
public ResponseEntity<ApiResponse<List<CommonCodeDto>>> getCommonCode (@PathVariable String codeName){ | ||
|
||
// | ||
List<CommonCodeDto> codes = commonCodeService.getCodes(codeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반환타입을 저장하는 변수를 만드는것도 좋지만 사용처가 없다면 바로 return 문에 넣어주는것은 어떨까요 ?return
ApiResponse.toResponseEntity(GET_COMMON_CODE_SUCCESS, commonCodeService.getCodes(codeName));
@@ -17,6 +17,7 @@ public class CommonCodeService { | |||
|
|||
public List<CommonCodeDto> getCodes(String codeName) { | |||
|
|||
// | |||
return commonCodeRepository.findAllByUpperNameAndIsUsed(codeName,true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
항상 true만 반환하는데 spring data jpa 에서 제공하는 true를 그냥 써도 될것같습니다!
참고 사이트 전달드립니다
@@ -25,6 +25,7 @@ public boolean preHandle(HttpServletRequest request, HttpServletResponse respons | |||
boolean result = true; | |||
String requestToken = request.getHeader("Authorization"); | |||
|
|||
// | |||
if (requestToken == null || requestToken.isEmpty()) { throw new BusinessException(AcceptInterceptorErrorCode.DOSE_NOT_EXIST_REQUEST_TOKEN); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objects.isNull 을 활용하여 null에 대해 명시적으로 관리하면 좋을것 같습니다! 또한 2가지 조건에대해서도 처리 가능합니다!
module-api/src/main/java/com/kernel360/main/controller/MainContoller.java
Show resolved
Hide resolved
@@ -124,6 +125,8 @@ public boolean idDuplicationCheck(String id) { | |||
|
|||
@Transactional(readOnly = true) | |||
public boolean emailDuplicationCheck(String email) { | |||
|
|||
// | |||
Member member = memberRepository.findOneByEmail(email); | |||
|
|||
return member != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 null처리 하는 부분도 여러가지로 보면 좋을듯 합니다.
- return Objects.nonNull(member);
- return memberRepository.existMemberByEmail(email) 과 같이 메서드 레벨 반환
@@ -200,6 +204,7 @@ public Map<String, Object> getCarInfo(String token) { | |||
|
|||
@Transactional | |||
public void saveWashInfo(WashInfoDto washInfoDto, String token) { | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
POST 및 PUT에 대해서 전반적인 값에 대한 validation처리가 필요할듯합니다.
등록 및 수정에서 올바르지 않은 값들이 DB에 들어가면 원하지 않은 에러들을 마주할 확률이 높습니다.
따라서 등록 및 수정에서는 validation처리가 타이트하게 들어가는것이 좋을것 같습니다
@@ -19,6 +19,7 @@ public class ProductController { | |||
|
|||
private final ProductService productService; | |||
|
|||
// | |||
@GetMapping("/products") | |||
ResponseEntity<ApiResponse<List<ProductDto>>> findProductList(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProductDto를 2개 이상의 컨트롤러에서 같이 사용하고 있습니다.
해당 Response에서 고려해보면 좋을만한 내용은 다음과 같습니다.
- API가 개발진행됨에 따라 2개의 API가 동일한 방향으로 Response가 개발되는지?
만약 조금이라도 다른방향으로 개발이 진행된다면 해당 API는 변경이 필요할것 같습니다!.
저는 개인적으로 API와 Request,Response는 한쌍으로 작업하는 편입니다! 참고부탁드립니다!
@@ -16,6 +16,8 @@ public class Auth extends BaseEntity { | |||
@Id | |||
@GeneratedValue(strategy = GenerationType.SEQUENCE, generator = "auth_id_gen") | |||
@SequenceGenerator(name = "auth_id_gen", sequenceName = "auth_auth_no_seq") | |||
|
|||
// | |||
@Column(name = "auth_no", nullable = false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JPA기본 설정시 underscore로 할지 camelcase로 컬럼명을 지정할지 정의할 수 있습니다
해당 옵션을 확인해보면 @column을 줄일 수 있을듯 합니다.
@@ -13,6 +13,7 @@ | |||
|
|||
@Getter | |||
@MappedSuperclass | |||
// 중복 | |||
@EntityListeners(AuditingEntityListener.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 설정이 중복되어 설정되어있는듯합니다.
spring application root에 설정 or configuration 파일에 단건 설정해주시는것도 좋을듯합니다.