-
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?
Changes from all commits
ddd61fb
c2a17d0
f1d0672
07cb76d
c4e1569
19a24d0
c90f43c
82db412
e0fcb63
1fc0f4e
060196a
5af31d2
44918ee
4d7d388
789b0cb
927b0e7
3afa661
dd9617a
e90ba9f
c4daa01
d3cd247
a4773a2
71676fa
242c2f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,12 @@ | |
public class CommonCodeController { | ||
|
||
private final CommonCodeService commonCodeService; | ||
|
||
// | ||
@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 commentThe reason will be displayed to describe this comment to others. Learn more. 반환타입을 저장하는 변수를 만드는것도 좋지만 사용처가 없다면 바로 return 문에 넣어주는것은 어떨까요 ?return |
||
|
||
return ApiResponse.toResponseEntity(GET_COMMON_CODE_SUCCESS, codes); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,12 +17,14 @@ 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 commentThe reason will be displayed to describe this comment to others. Learn more. 항상 true만 반환하는데 spring data jpa 에서 제공하는 true를 그냥 써도 될것같습니다! 참고 사이트 전달드립니다 |
||
.stream() | ||
.map(CommonCodeDto::from) | ||
.toList(); | ||
} | ||
|
||
// | ||
public List<CommonCodeDto> getCodesMapper(String codeName) { | ||
|
||
return commonCodeMapper.findAllByUpperNameAndIsUsed(codeName,true) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Objects.isNull 을 활용하여 null에 대해 명시적으로 관리하면 좋을것 같습니다! 또한 2가지 조건에대해서도 처리 가능합니다! |
||
|
||
if (!validRequestToken(requestToken)) { throw new BusinessException(AcceptInterceptorErrorCode.FAILED_VALID_REQUEST_TOKEN); } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,17 +18,22 @@ | |
@RestController | ||
@RequestMapping | ||
@RequiredArgsConstructor | ||
|
||
// | ||
public class MainContoller { | ||
chan99k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
private final ProductService productService; | ||
private final MainService mainService; | ||
|
||
// | ||
@GetMapping("/banner") | ||
ResponseEntity<ApiResponse<BannerDto>> getBanner() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. API는 복수형으로 작성하시는걸 추천드립니다 |
||
|
||
return ApiResponse.toResponseEntity(BannerBusinessCode.GET_BANNER_DATA_SUCCESS, mainService.getBanner()); | ||
} | ||
|
||
|
||
@GetMapping("/recommend-products") | ||
// | ||
ResponseEntity<ApiResponse<Page<RecommendProductsDto>>> getRecommendProducts(Pageable pageable) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pageable를 바로사용해도 좋지만 클래스로 만들어서 Custom하게 사용할 수 있도록 해보시는것도 좋아보입니다! |
||
Page<RecommendProductsDto> recommendProductList = productService.getRecommendProductList(pageable); | ||
|
||
|
@@ -37,6 +42,8 @@ ResponseEntity<ApiResponse<Page<RecommendProductsDto>>> getRecommendProducts(Pag | |
} | ||
|
||
@GetMapping("/products/rank") | ||
|
||
// | ||
ResponseEntity<ApiResponse<Page<ProductDto>>> getProducts( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Controller에서 반환하는 Response타입명을 DTO를 제거하고 Response를 붙여 보시는건 어떠세요? |
||
@RequestParam(name = "sortType", defaultValue = "viewCnt-order") Sort sortType, Pageable pageable) { | ||
Page<ProductDto> productDtos = sortType.sort(productService, pageable); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
import org.springframework.data.domain.Pageable; | ||
|
||
|
||
// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sort가 Enum인데 패키지의 위치가 controller에 있습니다. 패키지의 위치 조절이 필요해 보입니다 . |
||
public enum Sort { | ||
|
||
VIEW_COUNT_PRODUCT_ORDER("viewCnt-order") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,10 +9,13 @@ | |
/** | ||
* DTO for {@link com.kernel360.member.entity.Member} | ||
*/ | ||
|
||
|
||
public record MemberDto(Long memberNo, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Request용으로 사용하고 있다면 Dto라는 명 대신 Request로 변경하는것은 어떠세요? |
||
String id, | ||
String email, | ||
String password, | ||
// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. String 대신 Enum이 있다면 Enum으로 받는것도 추천드립니다. |
||
String gender, | ||
String age, | ||
LocalDate createdAt, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,8 @@ static MemberInfo of( | |
return new MemberInfo(id, email, gender, age); | ||
} | ||
|
||
|
||
// | ||
public Member toEntity() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Entity를 만드는 책임을 Request 대신 Mapper와 같이 생성하는 클래스를 만들어보시면 어떨까요 ? |
||
return Member.of( | ||
this.id, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,6 +59,7 @@ protected Member getNewJoinMemberEntity(MemberDto requestDto) { | |
int ageOrdinal; | ||
|
||
try { | ||
// enum으로 받으면 다음과 같은 작업을 할 필요가 없음 . | ||
genderOrdinal = Gender.valueOf(requestDto.gender()).ordinal(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enum으로 Request를 받는다면 해당 부분은 자연스럽게 없어도 될것 같습니다! |
||
ageOrdinal = Age.valueOf(requestDto.age()).ordinal(); | ||
} catch (Exception e) { | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. 해당 null처리 하는 부분도 여러가지로 보면 좋을듯 합니다.
|
||
|
@@ -188,6 +191,7 @@ public Map<String, Object> getCarInfo(String token) { | |
Member member = memberRepository.findOneById(id); | ||
CarInfoDto carInfoDto = CarInfoDto.from(member.getCarInfo()); | ||
|
||
// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 고정된 값들을 반환하는것 같은데 class로 분리해보심이 어떠실까요 ? |
||
return Map.of( | ||
"car_info", carInfoDto, | ||
"segment_options", commonCodeService.getCodes("segment"), | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. POST 및 PUT에 대해서 전반적인 값에 대한 validation처리가 필요할듯합니다. |
||
String id = JWT.ownerId(token); | ||
Member member = memberRepository.findOneById(id); | ||
WashInfo washInfo = WashInfo.of(washInfoDto.washNo(), washInfoDto.washCount(), washInfoDto.monthlyExpense(), washInfoDto.interest()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,10 +15,13 @@ | |
|
||
@Slf4j | ||
@RestController | ||
// | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mypage라는 URL을 표기해야 하는지 url에 대한 재정의도 해보면 좋을것 같습니다. |
||
@RequestMapping("/mypage") | ||
@RequiredArgsConstructor | ||
public class MyPageController { | ||
private final MemberService memberService; | ||
|
||
// | ||
private final ProductService productService; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 미사용 서비스는 제거하면 좋을것 같습니다! |
||
|
||
@GetMapping("/member") | ||
|
@@ -45,6 +48,7 @@ ResponseEntity<ApiResponse<Void>> memberDelete(@RequestHeader("Authorization") S | |
|
||
|
||
@PostMapping("/member") | ||
// | ||
ResponseEntity<ApiResponse<String>> validatePassword(@RequestBody String password, @RequestHeader("Authorization") String authToken) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 비밀번호를 단건으로전달받더라도 class로 받아 validation처리를 축해주시면 좋을것 같습니다. |
||
memberService.changePassword(password, authToken); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. ProductDto를 2개 이상의 컨트롤러에서 같이 사용하고 있습니다.
만약 조금이라도 다른방향으로 개발이 진행된다면 해당 API는 변경이 필요할것 같습니다!. 저는 개인적으로 API와 Request,Response는 한쌍으로 작업하는 편입니다! 참고부탁드립니다! |
||
final List<ProductDto> productDtoList = productService.getProductList(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. JPA기본 설정시 underscore로 할지 camelcase로 컬럼명을 지정할지 정의할 수 있습니다 |
||
private Long authNo; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
|
||
@Getter | ||
@MappedSuperclass | ||
// 중복 | ||
@EntityListeners(AuditingEntityListener.class) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 설정이 중복되어 설정되어있는듯합니다. |
||
public class BaseRawEntity { | ||
// FIXME :: createdAt 자동 생성시, update 로직 진행중에 null or transient 문제가 발생. 임시방편으로 직접 지정하였으나 이후 수정 필요 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,12 +16,13 @@ | |
public interface ProductRepository extends JpaRepository<Product, Long> { | ||
Page<Product> findByProductNameContaining(String keyword, Pageable pageable); | ||
|
||
// | ||
Page<Product> findAllByOrderByViewCountDesc(Pageable pageable); | ||
|
||
Page<Product> findTop5ByOrderByProductNameDesc(Pageable pageable); | ||
|
||
Page<Product> findAllBySafetyStatusEquals(SafetyStatus safetyStatus, Pageable pageable); | ||
|
||
거 | ||
Page<Product> findAllByOrderByCreatedAtDesc(Pageable pageable); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 미사용 메서드들은 제거해보시는거 어떨까요 |
||
|
||
@Query(value = "SELECT p FROM Product p WHERE p.productName = :productName " | ||
|
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를 통해서도 관리하는 방법에 대해 보시면 좋을것 같습니다!