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

전남대 BE_안원모_3주차 과제(0,1,2단계) #251

Merged
merged 41 commits into from
Jul 14, 2024

Conversation

Wonmoan
Copy link

@Wonmoan Wonmoan commented Jul 11, 2024

2주차 과제 step3의 코멘트 부분을 수정해서 반영했습니다.

테스트에서 미흡한 부분이 있으면 피드백 주시면 감사하겠습니다!

감사합니다.

Copy link
Author

@Wonmoan Wonmoan left a comment

Choose a reason for hiding this comment

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

추가로 layered architecture 패키지 구조 반영해서 코드 정리하였습니다.

@Wonmoan Wonmoan changed the title 전남대 BE_안원모_3주차 과제(0,1단계) 전남대 BE_안원모_3주차 과제(0,1,2단계) Jul 12, 2024
Copy link

@pkeugine pkeugine left a comment

Choose a reason for hiding this comment

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

안녕하세요 원모님,
미션 잘 진행해주시고 있습니다.
일단 change request 로 두고, 리뷰 확인한 뒤에 다음 스텝으로 넘어가보아요.
각 코멘트에 모두 답변 달아주세요!

src/main/java/gift/config/DataLoader.java Outdated Show resolved Hide resolved
Comment on lines 20 to 31
@Override
public void run(String... args) throws Exception {
for (int i = 3; i <= 10; i++) {
Product product = new Product.ProductBuilder()
.name("Product" + i)
.price(BigDecimal.valueOf(10.00 * i))
.imageUrl("http://example.com/product" + i)
.description("Description for Product" + i)
.build();
productRepository.save(product);
}
}

Choose a reason for hiding this comment

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

data.sql 과 해당 방식을 함께 사용하게 된 이유가 궁금해요.

Copy link
Author

Choose a reason for hiding this comment

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

음 .. 다음 step인 step3 에서 페이지네이션 작성하여 확인할 때 편의성을 위해 작성하였습니다.
data.sql에는 현재 product2 까지 2개의 상품으로 초기 데이터가 작성 되어 있는데,
이 경우, 상품 여러개를 추가하여 페이지네이션 작동을 확인 할 때 data.sql파일을 업데이트를 계속해서 해줘야 할 것 같아 해당 방식을 사용하였습니다.

Choose a reason for hiding this comment

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

하나의 방식으로 통일하는게 좋을 것 같네요.

Copy link
Author

@Wonmoan Wonmoan Jul 15, 2024

Choose a reason for hiding this comment

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

음.. 위시 리스트 구현방법에 대해 질문 드리고 다음 step에 반영 하겠습니다.

 @Override
    public void run(String... args) throws Exception {
        for (int i = 3; i <= 10; i++) {
            Product product = new Product.ProductBuilder()
                .name("Product" + i)
                .price(BigDecimal.valueOf(10.00 * i))
                .imageUrl("http://example.com/product" + i)
                .description("Description for Product" + i)
                .build();
            productRepository.save(product);
        }
    }

현재 구현된 방법은 상품에 대한 구체적인 정보가 단순히 인덱스만 증가되어 추가되는 형태입니다.
하지만 실제 상품의 경우는 이름도 다 다르고 imageurl, description같은 경우에도 인덱싱뿐만 아니라 설명하는 내용이 다를텐데 이런 요구사항을 어떻게 반영해야 할까요?

기존의 방법으로 data.sql에 상품의 구성요소들을 등록하는 방법으로 통일하는 편이 좋을까요?
아니면, JSON, CSV와 같은 상품 정보를 담은 외부 파일에서 데이터를 읽어와서 초기화 하는 방법으로 통일하는 편이 좋을가요?

추후에 위시 리스트가 추가적으로 어떻게 구현 될진 모르겠지만 어떤 방법이 앞으로의 작업을 진행하는데 있어서 더 나은 방향일까 고민이 됩니다.

Copy link
Author

@Wonmoan Wonmoan Jul 16, 2024

Choose a reason for hiding this comment

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

4주차 step1과제를 좀 진행 해보면서
위와 같은 초기데이터 설정은 페이지네이션 작동이 잘 되는지 확인할 때
여러 상품이 잘 등록,조회가 될수 있나? 테스트할 때 사용되기 좋은 방식이지 실제로 초기 데이터를 등록 할 때는 좋은 방법이 아닌 것 같다라고 생각이 들었습니다.

제가 이해한 4주차 step1은 상품등록에 있어 등록 할 수 있는 카테고리의 범주가 한정 되어있다고 생각했습니다.
(교환권, 상품권, 뷰티, 패션, 식품, 리빙/도서, 레저/스포츠, 아티스트/캐릭터, 유아동/반려, 디지털/가전, 카카오프렌즈, 트렌드 선물, 백화점)
저 로직은 여러 상품을 만들어 잘 작동 되는 지 확인하는 용으로 사용하고
앞으로 진행할 프로젝트에서는 data.sql 방식을 사용할까 합니다.

src/main/java/gift/controller/MemberController.java Outdated Show resolved Hide resolved
Comment on lines +41 to +43
} catch (Exception e) {
throw new RuntimeException("Error during registration: " + e.getMessage(), e);
}

Choose a reason for hiding this comment

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

예외를 잡아서 다시 runtime exception 으로 바꿔주시는 이유가 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

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

음.. exception을 잡아서 다시 RuntimeException으로 예외를 잡아서 다시 던지면서
예외 메시지를 추가하거나 로깅을 통해 디버깅 정보를 더 자세하게 나타내보려 했습니다.
변경하는 편이 코드 가독성에 더 도움이 된다면 변경하겠습니다.

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.

@PostMapping("/register")
    public ResponseEntity<Map<String, String>> register(@Valid @RequestBody MemberDTO memberDTO) {
        Member member = memberDTO.toEntity();
        Member savedMember = memberService.createMember(member);
        Map<String, String> response = new HashMap<>();
        response.put("token", jwtService.generateToken(savedMember));
        return ResponseEntity.ok(response);
    }

GlobalExceptionHandler 에서 에외처리 할 수 있도록
기존의 예외를 다시 던지는 코드 수정했습니다.

README.md Show resolved Hide resolved
src/main/resources/application.properties Show resolved Hide resolved
src/test/java/gift/WishRepositoryTest.java Outdated Show resolved Hide resolved
src/test/java/gift/MemberRepositoryTest.java Show resolved Hide resolved
Comment on lines 16 to 18
@NotNull
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "member_id", nullable = false)

Choose a reason for hiding this comment

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

FetchType.LAZY 로 설정하신 이유가 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

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

FetchType.LAZY를 사용하면 해당 연관된 엔티티가 필요할 때 로드 됨으로
이렇게 하면 초기 로딩 시 불필요한 데이터 로드를 방지하여 필요한 시점에만 로드 함으로써
좀 더 나은 성능을 보일 것 이라 생각했습니다.

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.

음 .. 필요할 때마다 데이터 로드하는 방식이 더 나을 거라고 생각했는데,
현재 서비스에선 wish 엔티가 작업될 때 member ,product 정보가 항상 포함 되어 함께 사용 되는 것 같다고 생각이 드네요
FetchType.LAZY 에서 FetchType.EAGER 로 변경하겠습니다.

src/main/java/gift/domain/Product.java Show resolved Hide resolved
Copy link

@pkeugine pkeugine left a comment

Choose a reason for hiding this comment

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

안녕하세요 원모님
리뷰 반영 + 반영 사항 표시해주시느라 정말 고생 많이하셨네요.
대박입니다 👍

이 PR 은 일단 머지하도록 할게요.
추가로 남긴 댓글은 시간나면 한 번 확인해주세요!

@pkeugine pkeugine merged commit 6d2ef27 into kakao-tech-campus-2nd-step2:wonmoan Jul 14, 2024
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