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

1단계 - 상품 관리 기능 #25

Merged
merged 25 commits into from
May 5, 2023
Merged

Conversation

hjhello423
Copy link
Member

@hjhello423 hjhello423 commented May 4, 2023

안녕하세요 아라님 😀
최홍준입니다. 2주 차 미션 진행되는 동안 잘 부탁드립니다!!
step1 리뷰 부탁드려요!

Copy link
Member

@xrabcde xrabcde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 홍준님 ~ 2주차 미션 리뷰를 맡은 조아라입니다 🙇🏻‍♀️
연휴인데도 빠르게 미션 진행해주셨네요
코드를 보니 이미 Spring 을 많이 사용해보신 것 같아요 ㅎㅎ

1단계 요구사항에 맞게 잘 구현해주셔서 바로 merge 하겠습니다 ~
다음 단계에서 만나요 🙌

Comment on lines +15 to +20
## 요구사항

* 상품 목록 페이지 연동
- [x] `index.html` 파일을 이용하여 상품 목록이 노출되는 페이지를 완성
- [x] `/` url로 접근할 경우 상품 목록 페이지를 조회할 수 있도록 구현
- [x] `/` url로 접근할 경우 `index.html` 페이지 반환
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우와 꼼꼼한 요구사항 작성 👍🏻👍🏻

Comment on lines +34 to +39
@GetMapping("/settings")
public String settings(Model model) {
// TODO step2에서 진행

return "settings";
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2단계 뼈대코드도 벌써 준비해주셨네요 👏

Comment on lines +34 to +35
@PutMapping("/{id}")
public ResponseEntity<ProductResponse> updateProduct(@PathVariable long id,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지금은 entity가 몇 개 없어서 헷갈리지 않을 수 있지만
어떤 id 인지를 구체적으로 작성해주면 더 좋을 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 의견 감사합니다!
id 대신 productId는 어떨까요? 메서드 인자랑 path 모두 변경해 줄 수 있겠네요.
다음 단계에서 반영해 보도록 하겠습니다.

Comment on lines +39 to +54
private void validateName(String name) {
if (!StringUtils.hasText(name)) {
throw new ServiceException(ErrorType.INVALID_PRODUCT_NAME);
}
}

private void validateImage(String image) {
if (!StringUtils.hasText(image)) {
throw new ServiceException(ErrorType.INVALID_PRODUCT_IMAGE);
}
}

private void validatePrice(Money price) {
if (Objects.isNull(price)) {
throw new ServiceException(ErrorType.INVALID_PRODUCT_PRICE);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null 체크는 dto에서 spring validation을 이용할 수도 있어요

Copy link
Member Author

@hjhello423 hjhello423 May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 도메인의 필드에 관련된 validation을 도메인 쪽에 넣어주려고 하는 편인데요.
해당 도메인에서 스펙도 확인 가능하고 해당 스펙을 만족하는 객체만 생성할 수 있기 때문에 선호합니다.
spring validation도 상황에 따라 활용하는 편이기는 한데요.
presentation에서 검증하고 서비스 레이어로 넘어가지 않도록 할 수 있는 게 장점이라고 생각해요.

아라님은 어떤 생각을 하고 있는지 궁금해요!

Comment on lines +3 to +9
public enum ErrorType {

INVALID_MONEY("0 이상의 값이 필요합니다."),
INVALID_PRODUCT_NAME("잘못된 상품명입니다."),
INVALID_PRODUCT_IMAGE("잘못된 상품 이미지입니다."),
INVALID_PRODUCT_PRICE("잘못된 상품 가격입니다."),
PRODUCT_NOT_FOUND("상품을 찾을 수 없습니다.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

커스텀 익셉션 사용과 에러타입 정의까지 👍🏻👍🏻

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아... 밤에 리뷰하면 안될거같은 테스트 이미지네요 🫠

Comment on lines +37 to +38
@DisplayName("상품 생성 실패 테스트")
@Test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외테스트 💯

Comment on lines +20 to +23
public static Money of(long value) {
if (value < MINIMUM_VALUE) {
throw new ServiceException(INVALID_MONEY);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

상품 가격이니 1원 이상의 값으로 받아도 될 것 같아요

image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생각해 보니 현재 미션의 요구 사항에서는 0원짜리 메뉴는 없을 수 있겠군요 🤔

@xrabcde xrabcde merged commit fadb4c9 into next-step:hjhello423 May 5, 2023
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.

2 participants