[2단계 - 주문 기능 구현] 도기(김동호) 미션 제출합니다. #82
Conversation
- 비밀번호가 같은지 확인하는 테스트 코드 추가
| String authorization = webRequest.getHeader(HttpHeaders.AUTHORIZATION); | ||
| validateNull(authorization); | ||
|
|
||
| String[] authHeader = authorization.split(" "); | ||
| validateAuthType(authHeader[AUTH_TYPE_INDEX]); | ||
|
|
||
| String[] credentials = encode(authHeader[AUTH_VALUE_INDEX]); |
There was a problem hiding this comment.
이전 미션들중 인터셉터와 리졸버의 역할을 분리해서 미션을 진행했던걸로 기억하는데
리졸버에서 인증과정까지 처리한 이유가 있나요?
클래스명과 역할이 잘 분리되지 않은것 같아서요
There was a problem hiding this comment.
사실 기존에 제공된 뼈대 코드여서 별 의심 없이 사용했습니다 😅
말씀해주신대로 Interceptor를 두어, 검증과 인증에 대한 책임을 부여하겠습니다 !!
|
|
||
| private static final String DEFAULT_MESSAGE = "인증 정보가 잘못 되었습니다."; | ||
|
|
||
| public AuthenticationException() { | ||
| super(DEFAULT_MESSAGE); | ||
| } | ||
|
|
||
| public AuthenticationException(final String message) { | ||
| super(message); | ||
| } |
There was a problem hiding this comment.
새롭게 정의한 exception이 너무 많은건 아닐까요?
유지 보수 측면에서 별로 좋은점이 없을것 같습니다. 실제 실무라면 이런 방법으로 예외를 생성하면 100개는 넘을것 같아요
참고로 제 회사에서는 클라(프론트)와 특별한 협의를 하는게 아닌 경우은 기존 정의된 몇개의 exception만 사용하고 있습니다
There was a problem hiding this comment.
�모든 상황에 대해 커스텀 예외를 생성한다면, 발생한 예외를 직접적으로 파악할 수 있을 것 같아요. 하지만 확실히 개수가 많아져 유지보수가 힘들어질 것 같습니다.
그렇다면, 커스텀 예외도 추상화 하는 것이 중요할 것 같은데요. 예를 들어, DB에 존재하지 않는 데이터를 조회할 때 발생하는 ~~NotFoundException의 경우 DataNotFoundException로 통일하고 어떤 데이터를 찾지 못했는 지에 대한 정보는 message로 나타내�는 방법을 생각해보았습니다.
그리고 기존에 제공된 뼈대 코드를 참고하여, 주문과 관련된 예외는 OrderException으로 추상화를 하고 그 안에 세부적인 예외들을 inner-class 로 만들어 관리하는 형식으로 하였습니다.
괜찮은 방법인 지 확신이 잘 서지 않는데요. 위 방법에 대한 영이의 의견이 궁금합니다 !
There was a problem hiding this comment.
음 회사마다 정책이 다르겠지만
제 회사의 경우 클라이언트(프론트)쪽에서 에러 타입을 통해 별도로 핸들링이 필요하지 않는이상
기본 baseException을 정의하고 이를 활용합니다.
서버 내에서 확인하는 용도는 결국 메세지 및 stacktrace가 중요하지 어떤 타입인지는 크게 중요하지 않을거라 생각합니다
유지보수 측면에서 확실히 커스텀 예외가 많아지면 번거로울것 같네요
예외를 매번 생성하는 시간도 많이들테고 이를 위한 팀 컨벤션등도 필요하겠네요
There was a problem hiding this comment.
말씀하신 baseException이라고 하면, IllegalArgumentException 같은 기본으로 제공되는 RuntimeException을 의미하시는걸까요? 또, 예외를 터트릴 때 어떠한 이유에서 발생했는 지 예외 메시지를 넣어주시나요??
| @AllArgsConstructor | ||
| @Getter |
| } | ||
| private final Long id; | ||
| private final String name; | ||
| private final int price; |
There was a problem hiding this comment.
bigDecimal 사용을 고려해보면 어떨까요?
저는 돈과 관련된건 거의 필수라고 생각하는데요
int의 경우 서비스가 확장되어 달러가 들어오기만해도 문제가 생기고
소수점 연산에서 오차가 발생할 수 있을 것 같네요
There was a problem hiding this comment.
아무래도 금전과 관련된 도메인은 가장 중요한 것이 신뢰성 및 데이터의 확실성이니, 말씀하신대로 BigDecimal 타입이 필수불가결하겠네요.
앞으로 개발하는 데에 있어서 정말 필요한 인사이트를 알려주셔서 감사합니다 👍
피드백 반영하겠습니다 !!
src/main/java/cart/domain/Point.java
Outdated
| public static final int MIN_USAGE_VALUE = 3_000; | ||
| private static final double POINT_RATE = 0.05; | ||
|
|
||
| private final int value; |
| public Long createProduct(ProductAddRequest productAddRequest) { | ||
| Product product = new Product(productAddRequest.getName(), productAddRequest.getPrice(), productAddRequest.getImageUrl(), productAddRequest.getStock()); | ||
|
|
||
| return productDao.insert(product); | ||
| } | ||
|
|
||
| public void updateProduct(Long productId, ProductAddRequest productAddRequest) { | ||
| Product newProduct = new Product(productId, productAddRequest.getName(), productAddRequest.getPrice(), productAddRequest.getImageUrl(), productAddRequest.getStock()); | ||
|
|
||
| productDao.update(newProduct); | ||
| } |
There was a problem hiding this comment.
변경 혹은 생성된 객체를 반환하여
response로 내려주면 좋을것 같네요
There was a problem hiding this comment.
위 코멘트에서 관련 질문을 남겨두었습니다!
| private final CartItemDao cartItemDao; | ||
|
|
||
| public Long save(final CartItem cartItem) { | ||
| CartItemEntity cartItemEntity = new CartItemEntity(cartItem.getMember().getId(), cartItem.getProduct().getId(), cartItem.getQuantityValue()); |
There was a problem hiding this comment.
getMemberId, getProductId
메서드를 별도로 만드는게 어떤가요
혹은
enttiy생성자에서 member, product를 받아 id를 get해서 저장할수도 있겠네요
| private final OrderDao orderDao; | ||
| private final OrderItemDao orderItemDao; | ||
|
|
||
| public long order(final Member member, final OrderRequest orderRequest) { |
There was a problem hiding this comment.
createOrder인지 updateOrder인지 명확한 메서드명을 정하면 좋을것 같습니다!
There was a problem hiding this comment.
아직까진 주문 수정 기능이 존재하지 않아, 단순히 주문한다 라는 의미로 order라고 명명했습니다.
하지만 충분히 주문과 관련된 여러 기능이 추가될 수 있음에 따라, 말씀하신대로 createOrder가 적절하겠네요 !
| List<CartItemInfoRequest> cartItemInfos = orderRequest.getCartItems(); | ||
|
|
||
| validateOwner(member, cartItemInfos); | ||
| validateTotalProductPrice(cartItemInfos, orderRequest.getTotalProductPrice()); |
There was a problem hiding this comment.
request에 주문 금액을 프론트에서 다 계산해서 주나요?
금액 계산 및 포인트 계산은 주문이 생성되는 시점에 서버에서 해줘야할것 같은데
request에는 어떤 상품이 주문되는지와 포인트 사용 유무만 담겨져오고
실제 결제 할 금액에대한 계산 및 포인트 사용은 주문이 생성되는 시점에 해야할것 같아요
There was a problem hiding this comment.
현재 프론트와 백엔드 모두 주문 금액을 계산해주고 있습니다.
이유는 사용자에게 보여줄 화면에 선택한 상품의 총 가격을 실시간으로 보여주기 위함과 동시에, 요청에 포함된 상품 가격과 실제 DB에 저장된 상품 가격이 다를 수 있다는 issue를 바탕으로 해당 로직을 추가했습니다.
|
|
||
| private static final String FORMAT = "yyyy-MM-dd HH:mm:ss"; | ||
|
|
||
| public static String asString() { |
There was a problem hiding this comment.
entity 에서 LocalDateTime 혹은 Instant로 관리되면 db에 시간은 잘 저장되지 않나요?
There was a problem hiding this comment.
네, DB에 저장은 잘 됩니다.
하지만 API 명세에 따르면 'yyyy-MM-dd HH:mm:ss' 형식으로 반환을 해야 했습니다.
따라서 처음에 LocalDateTime으로 관리했을 때, DB에서 값을 꺼내오면 가운데에 'T' 가 추가되는 문제가 발생했습니다.
이 문제를 해결하고자 위와 같은 방법을 적용했습니다.
혹시 더 나은 방법이나 키워드가 있다면 알려주시면 감사하겠습니다 🙇♂️
There was a problem hiding this comment.
보통은 중간에 T 가 있는 UTC를 자주 사용하는것 같은데
yyyy-MM-dd HH:mm:ss 이 형식을 사용한 이유가 궁금합니다!🙏
There was a problem hiding this comment.
팀원 전체가 UTC 존재에 대해 잘 모르고 있어서 중간에 'T' 를 포함하지 않았던 것 같습니다.
보통은 프론트에게 반환할 때에도 중간에 'T'를 포함해서 반환하나요??
- ArgumentResolver로 생성된 member로 또 다시 member를 찾는 로직 제거
|
안녕하세요 영이~ 피드백 반영 해보았습니다. Member Login 시, Interceptor와 ArgumentResolver의 역할은 어디까지인가?Interceptor에서 AuthType이 Basic인지? null인지? 등을 체크하는 것은 문제없다고 생각합니다. 따라서 스스로 판단하에, Interceptor에서는 Basic Type인지? null은 아닌지? 정도만 체크하고, DB에 접근 후에 이뤄지는 검증들은 ArgumentResolver에서 진행하는 식으로 결정했습니다. 커스텀 예외의 추상화를 진행해 보았는데요. 처음 해보는 방식이다 보니, 적절하게 수행했는지 확신이 잘 서지 않습니다. |
|
|
||
| @Override | ||
| public void addInterceptors(final InterceptorRegistry registry) { | ||
| registry.addInterceptor(new AuthValidateInterceptor()) |
There was a problem hiding this comment.
interceptor는 보통 bean으로 등록하여 사용하는것 같습니다.
빈으로 등록된 interceptor라면 빈으로 등록된 memberReposiotry를 호출할 수 있을것이고 이를 활용해 interceptor에서 등록된 member인지를 확인할 수 있을것 같네요!
그리고 interceptor에서는 request.setAttribute 나 session 등을 활용하여서 email, password를 넘기고
argumentResolver에서는 member라는 객체를 만들어서 반환하면 좋을것 같네요
|
|
||
| @Override | ||
| public void addArgumentResolvers(final List<HandlerMethodArgumentResolver> resolvers) { | ||
| resolvers.add(new MemberArgumentResolver(memberRepository)); |
There was a problem hiding this comment.
스프링 빈으로 등록된 객체(memberRepository)를 스프링 빈이 아닌 memberArgumentResolver의 생성자로 넣어도 괜찮을까요?
| } | ||
|
|
||
| public Member usePoint(final Point usePoint) { | ||
| point.validateUsePoint(usePoint); |
There was a problem hiding this comment.
subtract 메서드에서 체크해주면 더 안전하고 더 간결할것 같아요!
| final Long orderId, | ||
| final Long productId, | ||
| final String productName, | ||
| final int productPrice, |
| public void validateUsablePoint(final int usePoint) { | ||
| if (0 < usePoint && usePoint < MIN_USAGE_VALUE) { | ||
| throw new OrderException.IllegalUsePoint("최소 사용 기준 포인트보다 작은 값의 포인트는 사용할 수 없습니다."); | ||
| } | ||
| } |
| public Product updateStock(final Quantity quantity) { | ||
| validateStock(quantity); | ||
|
|
||
| return new Product(id, name, price, imageUrl, stock - quantity.getValue()); | ||
| } | ||
|
|
||
| private void validateStock(final Quantity quantity) { | ||
| if (stock < quantity.getValue()) { | ||
| throw new OrderException.InsufficientStock("상품의 남은 수량보다 많은 주문량입니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
validateStock은 굳이 메서드로 안빼도 될것 같아요
너무 메서드를 잘게 나누는것도 가독성을 떨어뜨릴수 있을것 같습니다!
| private final ProductInfoRequest product; | ||
|
|
||
| private CartItemInfoRequest() { | ||
| this(null, null, null); |
There was a problem hiding this comment.
빈 생성자가 parsing할때 꼭 필요 하던가요??
보통은 final을 빼고 빈생성자를 만들거나
빈생성자를 없애는 방향을 택하는것 같아요
| public static class IllegalUsePoint extends OrderException { | ||
| public IllegalUsePoint(final int remainPoint, final int usePoint) { | ||
| super("사용 포인트가 잔여 포인트보다 많습니다. 잔여 포인트: " + remainPoint + " 사용 포인트: " + usePoint); | ||
| } | ||
|
|
||
| public IllegalUsePoint(final String message) { | ||
| super(message); | ||
| } | ||
| } | ||
|
|
||
| public static class MismatchedTotalPrice extends OrderException { | ||
| public MismatchedTotalPrice(final String message) { | ||
| super(message); | ||
| } | ||
| } | ||
|
|
||
| public static class MismatchedTotalProductPrice extends OrderException { | ||
| public MismatchedTotalProductPrice(final String message) { | ||
| super(message); | ||
| } | ||
| } | ||
|
|
||
| public static class InsufficientStock extends OrderException { | ||
| public InsufficientStock(final String message) { | ||
| super(message); | ||
| } | ||
| } |
| @@ -0,0 +1,36 @@ | |||
| package cart.exception; | |||
|
|
|||
| public class OrderException extends RuntimeException { | |||
안녕하세요 영이!
우테코 첫 미션인 자동차 경주 미션 이후로 다시 뵙네요!!! 반갑습니다 🙌
그럼 리뷰 잘 부탁드립니다 :)
리뷰 범위입니다.