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

[4기 - 황창현] Week2-3 JdbcTemplate을 활용한 바우처 CRUD 기능 구현 #774

Merged
merged 63 commits into from
Jul 16, 2023

Conversation

Hchanghyeon
Copy link
Member

📌 과제 설명

2주차 과제

기본
2-1

  • 바우처 관리 애플리케이션에 단위테스트를 작성해보세요.
    • 가능한 많은 단위 테스트코드를 작성하려고 노력해보세요.
    • 엣지 케이스(예외 케이스)를 고려해서 작성해주세요..

2-2

  • 바우처 관리 애플리케이션에서도 과정에서 다루었던 고객을 적용해보세요.
    • customers 테이블 정의 및 추가
    • CustomerRepository 추가 및 JdbcTemplate을 사용해서 구현

2-3

  • 바우처 정보를 DB로 관리해보세요.(1주차엔 파일로 관리)
    • 바우처에 엔터티에 해당하는 vouchers 테이블을 한번 정의해보세요.
    • 바우처 레포지토리를 만들어보세요. (JdbcTemplate을 사용해서 구현)
    • 기존의 파일에서 바우처를 관리한 것을 vouchers 테이블을 통해서 CRUD가 되게 해보세요.

심화
2-4

  • 특정 고객에게 바우처를 할당할 수 있습니다.
  • 고객이 어떤 바우처를 보유하고 있는지 조회할 수 있어야 합니다.
  • 고객이 보유한 바우처를 제거할 수 있어야 합니다.
  • 특정 바우처를 보유한 고객을 조회할 수 있어야 합니다.

👩‍💻 요구 사항과 구현 내용

요약

  • Jdbc를 활용하여 콘솔에서 바우처를 CRUD 할 수 있도록 Controller, Service, Repository를 구현했습니다.

Voucher

  • NamedParameterJdbcTemplate을 이용
  • 바우처 저장 기능(save)
  • 바우처 업데이트 기능(update)
  • 바우처 찾기 기능(findById, findAll)
  • 바우처 삭제 기능(deleteById, deleteAll)
  • 레이어 간 데이터 이동시 dto를 활용하여 이동할 수 있도록 dto객체 생성
  • 테스트 코드 구현

✅ PR 포인트 & 궁금한 점

이전 PR을 마치지 않고 PR을 올리게되어서 이전 Customer와 관련된 파일들도 Files changed에서 나오고 있습니다. 이번 PR에서 작업한 것은 Customer를 제외한 나머지이며 Voucher와 관련된 파일들 위주로 봐주시면 될 것 같습니다! 🙏

아래 궁금한 점은 따로 Files changed에도 미리 리뷰 달아놓았습니다!

궁금한점

  1. 계층 간 데이터가 이동할 때 DTO를 사용하는 것으로 알고있습니다. 현재 프로그램에서도 콘솔에서 입력받은 값을 DTO로 변환해서 Service까지 전달하고 Serivce에서 Entity로 변환하여 Repository로 넘기고 있습니다. Repository에서 반환된 값의 경우 따로 DTO로 변환하지 않고 바로 Console로 넘겼었다가 DTO로 변경해서 Console로 넘기는 구조로 변경했는데 이러한 구조를 가져가는 것이 맞는지 궁금합니다!
    (저는 현 프로그램에서는 Repository -> View로 데이터가 넘어갈 때 해당 Entity의 값이 변환될 일이 없다고 생각되어 그냥 Entity를 넘겨도 되지 않을까? 생각했던 것 같습니다.)

Comment on lines 8 to 22
@Getter
public class CustomerResponse {

private UUID customerId;
private String customerName;
private String customerEmail;
private CustomerType customerType;

public CustomerResponse(Customer customer) {
this.customerId = customer.getCustomerId();
this.customerName = customer.getCustomerName();
this.customerEmail = customer.getCustomerEmail();
this.customerType = customer.getCustomerType();
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

계층 간 데이터가 이동할 때 DTO를 사용하는 것으로 알고있습니다. 현재 프로그램에서도 콘솔에서 입력받은 값을 DTO로 변환해서 Service까지 전달하고 Serivce에서 Entity로 변환하여 Repository로 넘기고 있습니다. Repository에서 반환된 값의 경우 따로 DTO로 변환하지 않고 바로 Console로 넘겼었다가 DTO로 변경해서 Console로 넘기는 구조로 변경했는데 이러한 구조를 가져가는 것이 맞는지 궁금합니다!
(저는 현 프로그램에서는 Repository -> View로 데이터가 넘어갈 때 해당 Entity의 값이 변환될 일이 없다고 생각되어 그냥 Entity를 넘겨도 되지 않을까? 생각했던 것 같습니다.)

Copy link

@hanjo8813 hanjo8813 Jul 7, 2023

Choose a reason for hiding this comment

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

이건 JPA를 공부하게 되면 알게 될 부분인데요, (영속성 컨텍스트의 생명주기와 관련된 부분)
JPA 특성상 Entity는 Controller 레이어(Presentation)로 올라가지 않는것이 좋습니다.

지금은 동작원리를 잘 모르지만, Entity를 Service 레이어까지만 사용한다고 생각하시고 DTO로 변환해서 리턴해주시면 될것 같습니다. (습관들이기!)

Copy link
Member Author

Choose a reason for hiding this comment

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

넵 알겠습니다!!! :) DTO로 변환하는 습관을 들이도록 하겠습니다!!

@Hchanghyeon Hchanghyeon marked this pull request as ready for review July 7, 2023 15:39
Copy link

@hanjo8813 hanjo8813 left a comment

Choose a reason for hiding this comment

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

창현님 수고하셨습니다~
큰 요구사항이 없어서 리뷰는 간단히 달았습니다!

Comment on lines +19 to +23
@SpringBootTest
public class CustomerServiceTest {

@Autowired
private CustomerService customerService;

Choose a reason for hiding this comment

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

VoucherService는 SpringBoot + Transactional 롤백을 사용한 테스트를 경험해봤으니,
Customer쪽에서는 mocking을 통한 완벽한 단위테스트를 짜보시는걸 추천드립니다~~

Copy link
Member Author

Choose a reason for hiding this comment

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

BDD Mockito를 활용하여 테스트 코드 작성해보았습니다!
save와 update의 경우 Service에서 Request로 전달하는 입력값과 Repository에서 Customer로 전달하는 입력값이 달라서 처리할 수 없다고 판단되어 따로 작성하지 못했습니다.

적용 커밋 : 6ca5617

Comment on lines 58 to 71
@Override
public Voucher findById(UUID voucherId) {
String sql = "select * from vouchers where voucher_id = :voucherId";

SqlParameterSource param = new MapSqlParameterSource()
.addValue("voucherId", voucherId);
try {
Voucher voucher = template.queryForObject(sql, param, voucherRowMapper());
return voucher;
} catch (EmptyResultDataAccessException e) {
log.error("찾는 바우처가 없습니다." + e);
throw new NoSuchElementException("찾는 바우처가 없습니다.");
}
}

Choose a reason for hiding this comment

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

리턴값을 optional로 감싸고, 찾는 값이 없을 경우에 대한 판단은 service 레이어에서 하도록 하는게 더 좋을것 같아요~

Copy link
Member Author

Choose a reason for hiding this comment

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

말씀하신 대로 optional로 감싸서 orElseThrow사용해서 처리했습니다!

적용 커밋 : f4ae3f5


public VoucherListResponse findAll() {
List<Voucher> voucherList = voucherRepository.findAll();
return new VoucherListResponse(voucherList.stream().map(VoucherResponse::new).collect(Collectors.toList()));

Choose a reason for hiding this comment

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

java 17에 추가된 컬렉션 문법을 씁시다!

Suggested change
return new VoucherListResponse(voucherList.stream().map(VoucherResponse::new).collect(Collectors.toList()));
return new VoucherListResponse(voucherList.stream().map(VoucherResponse::new).toList());

Copy link
Member Author

@Hchanghyeon Hchanghyeon Jul 10, 2023

Choose a reason for hiding this comment

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

헉 소나린트에 나왔었는데 제가 못봤었나보네요!! 감사합니다!! 수정했습니다!

적용 커밋 : c448834

Comment on lines 48 to 56
public CustomerListResponse findAll() {
List<Customer> customerList = customerRepository.findAll();
return new CustomerListResponse(customerList.stream().map(CustomerResponse::new).collect(Collectors.toList()));
}

public CustomerListResponse getBlackList() {
List<Customer> customerList = customerRepository.getBlackList();
return new CustomerListResponse(customerList.stream().map(CustomerResponse::new).collect(Collectors.toList()));
}

Choose a reason for hiding this comment

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

요기도 toList!

Copy link
Member Author

Choose a reason for hiding this comment

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

적용 커밋 : c448834

Comment on lines +121 to +123
}

}

Choose a reason for hiding this comment

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

스스로 클래스 끝줄 개행 컨벤션을 세워놓으면 일관된 코드 작성에 도움을 줍니다.
요기 빼고 다른 클래스들은 붙여서 마무리하신것 같네요.
이런 디테일도 있구나~ 하고 넘어가시면 됩니다 ㅎㅎ
(참고로 스프링은 한줄 띄웁니다 ㅋ.ㅋ)

Copy link
Member Author

Choose a reason for hiding this comment

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

저는 반대로 생각하고,, 보이는 것마다 다 닫고 있었는데 반대였네요..!

적용 커밋 : c0a59ae

Choose a reason for hiding this comment

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

한 줄 띄우는게 정답은 아니라서요.
스프링 라이브러리들을 보면 띄우는걸 컨벤션으로 잡아놨고.. 다른 라이브러리들 보면 닫는걸 컨벤션으로 잡은곳도 있습니다!

Copy link

@mentor-tyler mentor-tyler left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다~

private final CustomerController customerController;
private final Console console;

public void run() {

Choose a reason for hiding this comment

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

Suggested change
public void run() {
public void menu() {

메서드 이름이 고민되긴 하군요 ㅎ

Copy link
Member Author

@Hchanghyeon Hchanghyeon Jul 12, 2023

Choose a reason for hiding this comment

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

콘솔 어플리케이션이라는 클래스에서 직접 적인 실행을 시키므로 말씀하신 대로 menu가 더 어울릴 것 같습니다! 감사합니다!! 👍

적용 커밋 : 76bde6c

private void selectCustomer() {
CustomerListResponse customerList = customerController.findAll();

if (customerList.getCustomerList().isEmpty()) {

Choose a reason for hiding this comment

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

리스트가 비어있는걸 사용할땐 npe에 주의하기위해 유틸을 사용하곤 하는것 같습니다.
org.apache.commons.collections4.isEmpty

Copy link
Member Author

@Hchanghyeon Hchanghyeon Jul 12, 2023

Choose a reason for hiding this comment

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

npe도 항상 고려하여 CollectionUtils.isEmpty()를 사용하도록 하겠습니다!!

적용 커밋 : 76bde6c

}

private void selectCustomer() {
CustomerListResponse customerList = customerController.findAll();

Choose a reason for hiding this comment

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

Suggested change
CustomerListResponse customerList = customerController.findAll();
CustomerListResponse customerListResponse = customerController.findAll();

네이밍에 진심인지라 ㅎ

Copy link
Member Author

@Hchanghyeon Hchanghyeon Jul 12, 2023

Choose a reason for hiding this comment

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

저도 이거를 되게 고민을 많이했는데 뭔가 콘솔에서는 명시적으로 customerListResponse보다는 customerList라는 것이 더 이해하기 쉬울 것이라 생각했던 것 같습니다! 다시 생각해보면 요청에 대한 응답 결과를 반환시켜주는거니까 Response가 붙는게 더 자연스럽운 것 같네요! 감사합니다!!

적용 커밋 : 76bde6c


switch (voucherType) {
case FIXED -> {
Validator.fixedAmountValidate(discount);
return new FixedAmountVoucher(Long.parseLong(discount));
return new FixedAmountVoucher(voucherId, discount);

Choose a reason for hiding this comment

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

이 경우 중괄호와 return 을 빼도 되지 않을까 싶네요 ㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

말씀하신대로 switch문 앞에서 return시키고 중괄호와 return 제외시켰습니다!

적용 커밋: 410e4c2

@@ -45,11 +47,12 @@ public void loadingBlackListToMemory() {

while ((line = bufferedReader.readLine()) != null) {
String[] readLine = line.split(",");
saveIfBlacklistedCustomer(readLine[0], readLine[1]);
saveIfBlacklistedCustomer(readLine[0], readLine[1], readLine[2], readLine[3]);

Choose a reason for hiding this comment

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

readLine split 했을때 4개가 안되면 어떻게 될까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

IndexOutOfBoundsException이 발생할 것 같습니다! 해당 부분은 Voucher 2-2 부분 피드백 반영사항에 추가해서 추후 현재 보고 계시는 w2-3과 머지해서 반영하도록 하겠습니다!


@Override
public List<Voucher> findAll() {
return new ArrayList<>(voucherMap.values());

Choose a reason for hiding this comment

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

데이터가 없을 경우 빈 객체를 내리는게 맞을까요 Optional 로 감싸는게 맞을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

데이터가 없을 경우 빈 객체를 내리는게 맞는 것 같습니다! try catch로 묶고 get으로 가져올 때 Null을 발생시키면 empty를 내리는 것으로 처리했습니다!

적용 커밋 : fe1eecf

@@ -8,34 +9,57 @@
@NoArgsConstructor(access = AccessLevel.PRIVATE)
public final class Validator {

Choose a reason for hiding this comment

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

이 클래스의 역할이 점점 커지진 않을지 걱정되네요. 이 안에서도 메서드 단위가 아니라 클래스 단위로 책임과 역할을 구분할수 있을지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 저도 요구사항이 늘어남에 따라 역할이 비대해지고 있는 느낌이 들었습니다! 우선 이렇게 선택했던 이유는 바우처 미션이 검증해야되는 부분이 엄청 많지 않다고 생각이 들었고 한 곳에 모아서 해도 될 것 같다는 생각이 들었었습니다!

추가적으로 질문을 드리고싶은게 있습니다!

멘토님께서는 Validator를 이용하여 View(Input)단에서 처리하는 것이 좋다고 생각하시나요? 아니면 DTO나 Domain에서 해당 클래스에서 검증 로직을 만들어 검증하는 것이 좋다고 생각하시나요?! 🤔

DTO나 Domain에서 검증 로직 메소드를 만들어놓을 경우 클래스 별로 분리가 되기도하고 도메인에서 생성하면 마지막에 들어가기전에 검증되는 것이라 더 안전하다고 생각했습니다! 하지만 클래스 별로 검증 로직이 들어가게될 경우 똑같은 검증 로직인데 여기저기 생기게 될 수도 있다는 생각이 들었습니다!

어떤 방법이 좋은 것일까요?

import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

public class EnumTest {

Choose a reason for hiding this comment

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

(뭔가 리뷰를 했던 기억이 있는데... 아님 말고요 ㅋㅋ)
enum을 테스트하는 클래스 인건 맞는데 이 안에서 테스트의 대상이 다수가 되는것 같네요.
각 클래스에 해당하는 테스트 클래스를 만드는게 맞아보여요.

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다!! 2-2에서 리뷰해주셔서 반영된 상태인데 현재 2-3과 merge하지 않아서 아직 이렇게 나오네요ㅠㅠ 얼른 merge하도록 하겠습니다!!

public class CustomerTest {

@Test
@DisplayName("고객을 생성한다.")

Choose a reason for hiding this comment

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

Suggested change
@DisplayName("고객을 생성한다.")
@DisplayName("고객을 생성할수 있다.")

기대값에 대한 테스트가 이루어지면 좋겠어요. 그에 대한 assertion을 목표로 하구요.

Copy link
Member Author

Choose a reason for hiding this comment

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

기대값에 대한 테스트가 이루어진다는게 어떤 의미인지 아직 정확하게 모르겠습니다! 조금만 더 자세하게 말씀해주실 수 있을까요? 🙏

우선 DisplayName의 경우 모두 한다. 보다는 할 수 있다. 라고 네이밍 변경처리 했습니다! w2-4 PR에 종합적으로 반영시켰습니다!

Comment on lines +238 to +239
// then
jdbcTemplateCustomerRepository.deleteAll();

Choose a reason for hiding this comment

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

무엇을 검증하게 될까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

작업을 하다가 깜빡하고 놓친 것 같습니다! 해당 부분이 Week2-4에서 반영한 피드백과 겹쳐서 2-4에서 리팩토링 반영하였습니다!

@Hchanghyeon Hchanghyeon merged commit 5c0df3b into prgrms-be-devcourse:changhyeonh/w1 Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants