Skip to content

Product 부분 완료했습니다.#13

Open
nihaojohn0609 wants to merge 15 commits intodoit-web-study:mainfrom
nihaojohn0609:main
Open

Product 부분 완료했습니다.#13
nihaojohn0609 wants to merge 15 commits intodoit-web-study:mainfrom
nihaojohn0609:main

Conversation

@nihaojohn0609
Copy link

User부분 합치지 않아서 error나는 부분은 주석처리 해서 올립니다.

Copy link
Member

@jjunhub jjunhub left a comment

Choose a reason for hiding this comment

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

테스트 코드까지 작성해보시느라 고생하셨습니다!!
페이징 관련해서 어려우셨을 것 같은데, 잘 마무리해주셨네요 :))

return categoryService.updateCategory(categoryId, categoryUpdateRequest);
}

@DeleteMapping("/{categoryId}")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@DeleteMapping("/{categoryId}")
@DeleteMapping("/categories/{categoryId}")

여러 상황들을 고려하였을 때, 위처럼 수정하는 게 어떨까요?
만약 변경하지 않은 채로 클라이언트 측에서 api 요청 보내는 엔드포인트를 몇 개 살펴보면 아래와 같습니다.

  • /api/1 -> 카테고리 id가 1인 카테고리 삭제
  • /api/2 -> 카테고리 id가 2인 카테고리 삭제

이렇게만 보면 api 요청이 어떤 것을 의미하는지 모호해질 것 같아서, 앞에 어떤 종류의 데이터를 다루는지가 endpoint에 들어가는 것이 RESTful한 API에 조금 더 가까워질 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

의견 감사합니다!!

}

if (productName == null || productDescription == null || productPrice == null || productStock == null || categoryId == null) {
return ProductUpdateResponse.fromError("400001", "상품 이름, 상품 설명, 상품 가격, 상품 재고, 카테고리 ID는 필수로 입력해주셔야 합니다.");
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
Author

Choose a reason for hiding this comment

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

헛 참고해보겠습니다!!


@RequiredArgsConstructor
@Service
@Transactional(readOnly = true)
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
Author

Choose a reason for hiding this comment

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

감사합니다!

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.

3 participants