🚀 2단계 - 장바구니 기능 by hongmoSung · Pull Request #38 · next-step/spring-cart

hongmoSung · 2023-05-14T08:31:40Z

사용자 기능 구현
사용자 설정 페이지 연동
장바구니 기능 구현
장바구니 페이지 연동

- 사용자 기능 구현 - 사용자 설정 페이지 연동 - 장바구니 기능 구현 - 장바구니 페이지 연동

Gomding

안녕하세요 홍모님!!
장바구니 미션 2단계도 빠르게 진행해주셨네요 👏 👏 👏
1단계 merge 하며 남겼던 리뷰는 확인이 안된것 같아 리마인드 드립니다..!
몇 가지 코멘트 남겼으니 확인부탁드려요 😄

궁금한 점 있으면 언제든 편하게 DM 남겨주세요~

Gomding · 2023-05-15T08:28:30Z

+        CartMember cartMember;
+        try {
+            cartMember = jdbcTemplate.queryForObject(sql, carMemberRowMapper(), email, password);
+        } catch (Exception e) {
+            return Optional.empty();
+        };
+        return Optional.ofNullable(cartMember);


CartMember 변수를 할당하는 과정이 필요하지 않아 보여요!
변수를 할당하지 않고 각각의 상황에 바로 반환하는 것은 어떻게 생각하시나요?

Suggested change

CartMember cartMember;

try {

cartMember = jdbcTemplate.queryForObject(sql, carMemberRowMapper(), email, password);

} catch (Exception e) {

return Optional.empty();

};

return Optional.ofNullable(cartMember);

try {

return Optional.ofNullable(jdbcTemplate.queryForObject(sql, carMemberRowMapper(), email, password));

} catch (Exception e) {

return Optional.empty();

};

Gomding · 2023-05-15T08:29:57Z

+    private RowMapper<CartMember> carMemberRowMapper() {
+        return (rs, rowNum) -> {


해당 메서드를 호출할 때마다 RowMapper 가 생성될것으로 보여요 :)
필드로 만들지 않으신 이유가 있을까요?

Suggested change

private RowMapper<CartMember> carMemberRowMapper() {

return (rs, rowNum) -> {

private final RowMapper<CartMember> carMemberRowMapper = (rs, rowNum) -> {

앗 처음에 알려주신 내용을 바로 이해하지 못하여서 따로 메서드로 분리하여 만들었었습니다.
알려주신 내용처럼 필드로 만들어 수정하였습니다.

Gomding · 2023-05-15T08:31:56Z

        'https://i.namu.wiki/i/6dLtHMNcLvz8BWVrtEB9CNEPqjDEhLIbtEpnZdIcbxZ4TZPc-P-Yk69qoEWSIaCudos3zI0mT-jw89Lm881FHg.webp',
        now());

+create table CART_MEMBER


CART_MEMBER 라는 이름은 CART 라는 서비스에 너무 한정되어 있는 느낌이군요 🤔
나중에 다른 기능에서 사용하면 어색한 테이블이 되지 않을까요? 🤔

Gomding · 2023-05-15T08:37:10Z

+    }
+
+    @Override
+    public List<ViewCartItem> getCartItems(Long memberId) {


Repository 에서 presentation 영역의 DTO를 반환해주고 있군요 🤔
persistance 영역에서 View 의 내용까지 알아버리게되니
응답값의 변화에 persistance 영역이 영향을 받게 될 수 있겠네요..!

entity 라는 객체를 만들어 주셨음에도 View 영역의 DTO 를 따로 사용하신 이유가 궁금해요!

화면에 노출해야할 데이터를 맞추기 위해
join 으로 필요한 데이터를 맞추다 보니 View 영역의 dto 가 적합하다 생각이 들어
사용하였습니다. 😅

Repository 에서는 저장소에 있는 데이터를 조회하고 응답해주는 것 까지가 책임이라고 생각해요!

View 에게 어떻게 응답될 지는 관심사 밖이 아닐까요? 🤔
Entity 객체나 도메인 객체에 매핑하는 방법도 고려해볼 수 있겠네요 😄

Gomding · 2023-05-15T08:41:07Z

+        int updateCount = jdbcTemplate.update(sql, id);
+        if (updateCount != 1) {
+            throw new RuntimeException(id + "에 해당하는 장바구니 아이탬을 삭제하는데 오류가 발생하였습니다.");
+        }


1단계 merge 하면서 남겼던 부분에 대한 의견도 확인부탁드려요 ! 😄

delete 시 DAO 는 updateCount 만을 반환하고
예외처리는 호출하는 쪽에서 해보면 어떨까요?

DAO 는 가능하면 바깥에서 어떻게 사용될 지에 관심을 가지지 않도록 구현하는것이 좋습니다 :)
어떤 로직에서는 삭제할 때 대상이 없어도 예외가 발생하지 않아야할 수도 있겠죠?

#34 (comment)

Gomding · 2023-05-15T08:42:43Z

+            return Optional.ofNullable(new AuthInfo(email, password));
+        }
+
+        return Optional.empty();


인증 값을 추출할 수 없으면 Optional.emtpy() 를 반환하도록 해주셨군요 💯

Gomding · 2023-05-15T08:46:12Z

+        Optional<AuthInfo> optionalAuthInfo = auth.extract(request);
+
+        if (optionalAuthInfo.isEmpty()) {
+            return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
+
+        }


사용자 인증을 위해 같은 로직이 중복사용되고 있군요!

Interceptor 와 ArgumanetResolver 에 대한 학습 테스트를 진행해볼까요?

https://www.baeldung.com/spring-mvc-handlerinterceptor

https://www.baeldung.com/spring-mvc-custom-data-binder#1-custom-argument-resolver

Gomding · 2023-05-15T08:52:34Z

+import java.util.Optional;
+
+@Repository
+public class MemberJdbcRepository implements MemberRepository {


테스트들을 적절하게 작성해보셨으면 좋겠어요~!!
스프링에서 제공하는 테스트 도구들을 활용해보죠 :)

Spring 프레임워크를 사용한 테스트도 작성해보면 어떨까요?

간단하게 @jdbcTest 라는 애너테이션을 사용해서 Repository 테스트부터 작성해보시면 좋겠어요!
https://www.baeldung.com/spring-jdbctemplate-testing

+) 더 나아가서 Service , Controller 테스트도 작성해보시면 좋겠군요 😄
Controller 테스트는 ProductIntegrationTest 에 샘플 테스트가 있으니 다른 path 들도 테스트를 작성해볼 수 있겠네요!

Gomding · 2023-05-15T08:53:20Z

+    }
+
+    public List<ViewMember> getMembers() {
+         return memberRepository.getMembers().stream().map(ViewMember::new)


메서드 체이닝을 할 때는 각 메서드마다 개행해주는 것이 가독성이 좋다고 생각해요~

Suggested change

return memberRepository.getMembers().stream().map(ViewMember::new)

return memberRepository.getMembers()

.stream()

.map(ViewMember::new)

- next-step#34

- next-step#38

Gomding

안녕하세요 홍모님!
Interceptor 잘 구현해주셨군요 👍
추가로 몇가지 코멘트 남겼으니 확인 부탁드려요!!

궁금한 점 있으시면 언제든 편하게 DM 남겨주세요 🙇

Gomding · 2023-05-17T12:17:06Z

+import java.util.List;
+
+@Service
+public class ItemService {


어떤 Item 에 대한 Service 인지 클래스이름만 보고 확신할 수 없어보여요!
CartItemService 는 어떻게 생각하시나요?

Gomding · 2023-05-17T12:17:32Z

+
+    private final MemberRepository memberRepository;
+    private final ProductRepository productRepository;
+    private final ItemRepository itemRepository;


ItemRepository 도 같은 의견입니다 😄

Gomding · 2023-05-17T12:20:24Z

+
+    public void addToCart(AuthInfo authInfo, RequestAddItem addItem) {
+        CartMember member = memberRepository.getMember(authInfo.getEmail(), authInfo.getPassword())
+                .orElseThrow(() -> new RuntimeException(authInfo + "에 해당하는 맴버가 없습니다."));


현재 Application 에서 에러가 발생하면 클라이언트에게 어떻게 응답되고 있을까요? 😄

ControllerAdvice와 ExceptionHandler 에 대해 학습해볼까요!
https://tecoble.techcourse.co.kr/post/2021-05-10-controller_advice_exception_handler/

Gomding · 2023-05-17T12:20:54Z

+import java.util.Optional;
+
+@Component
+public class MyInterceptor implements HandlerInterceptor {


HandlerInterceptor 를 사용해주셨군요 👍

Gomding · 2023-05-17T12:24:05Z

+        Optional<AuthInfo> optionalAuthInfo = auth.extract(request);
+
+        if (optionalAuthInfo.isEmpty()) {
+            return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build();
+
+        }


Interceptor 에서 사용자 인증을 진행했는데 한번 더 검사하시는 이유가 궁금해요!

만약 AuthInfo 라는 정보가 필요하다면 ArgumentResolver 를 사용해보는건 어떨까요 :)

Gomding · 2023-05-17T12:25:57Z

+    @Override
+    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) {
+        Optional<AuthInfo> optionalAuthInfo = auth.extract(request);
+        return optionalAuthInfo.isPresent();
+    }


Header 에 사용자 정보가 들어있는지만 확인하면 사용자 인증이 됐다고 할 수 있을까요? 🤔

사용자의 정보가 실제 사용자 정보와 일치하는지 확인해주어야하지 않을까요?
현재는 email 과 password 정보가 들어있으니 저장소에 있는 사용자 정보와 비교해볼 수 있겠네요 😄

Gomding · 2023-05-17T12:26:27Z

+import java.util.Optional;
+
+@Component
+public class MyInterceptor implements HandlerInterceptor {


인증을 위한 Interceptor 이니 그에 맞는 이름을 지어주면 어떨까요? 🙌

Gomding · 2023-05-17T12:33:59Z

+
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+class MemberJdbcRepositoryTest {


@JdbcTest 를 활용하면 Repository 빈을 을 주입받아 사용할 수 있습니다 :)
스프링에서 테스트를 쉽게할 수 있도록 제공하는 기능이죠 😄

+) @Sql 애너테이션을 사용하면 sql 스크립트 실행이 가능합니다 :)

단 @JdbcTest 애너테이션을 사용하면 테스트 실행 전~후 를 트랜잭션 범위로 잡아서
각 테스트 메서드 실행 후 롤백을 실행합니다.

즉, 각 테스트 메서드에서 실행한 쿼리들은 롤백이 되어 테스트 독립적으로 실행이 가능해집니다.

sql 스크립트 대신 @BeforeEach에서 더미데이터를 넣어주는건 어떨까요?

Suggested change

class MemberJdbcRepositoryTest {

@Sql("classpath:test-data.sql")

@JdbcTest(

includeFilters = {

@ComponentScan.Filter(type = FilterType.ANNOTATION, value = Repository.class)

}

)

class MemberJdbcRepositoryTest {

@Autowired

private MemberRepository memberRepository;

Gomding · 2023-05-17T12:37:32Z

+import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
+
+@Configuration
+public class WebConfig implements WebMvcConfigurer {


WebConfig 설정 👍

Gomding · 2023-05-17T12:39:14Z

+@Service
+public class ItemService {


Repository 테스트를 작성해주셨으니 다음은 Service 테스트도 작성해볼까요? 😄
@SpringBootTest 애너테이션을 사용하면 테스트에서 스프링 환경과 비슷하게 테스트가 가능합니다 :)
빈을 주입받는것도 가능하죠 :)

hongmoSung added 3 commits May 14, 2023 14:43

feat: 사용자 기능 구현

ee6cbc8

feat: 사용자 설정 페이지 연동

d789920

feat: 🚀 2단계 - 장바구니 기능

6e29ed3

- 사용자 기능 구현 - 사용자 설정 페이지 연동 - 장바구니 기능 구현 - 장바구니 페이지 연동

Gomding requested changes May 15, 2023

View reviewed changes

hongmoSung added 2 commits May 16, 2023 22:34

refactor: 1단계 상품관리 기능 리뷰반영

13de596

- next-step#34

refactor: 2단계 상품관리 기능 리뷰반영

2ca87f3

- next-step#38

Gomding requested changes May 17, 2023

View reviewed changes

		private RowMapper<CartMember> carMemberRowMapper() {
		return (rs, rowNum) -> {

	private RowMapper<CartMember> carMemberRowMapper() {
	return (rs, rowNum) -> {
	private final RowMapper<CartMember> carMemberRowMapper = (rs, rowNum) -> {


		import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

		class MemberJdbcRepositoryTest {

-class MemberJdbcRepositoryTest {
+@Sql("classpath:test-data.sql")
+@JdbcTest(
+        includeFilters = {
+                @ComponentScan.Filter(type = FilterType.ANNOTATION, value = Repository.class)
+        }
+)
+class MemberJdbcRepositoryTest {
+    @Autowired
+    private MemberRepository memberRepository;

Conversation

hongmoSung commented May 14, 2023

Uh oh!

Gomding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gomding May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gomding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Gomding May 15, 2023 •

edited

Loading