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기 - 유원우] SpringBoot Part3 Weekly Mission 제출합니다. #864

Open
wants to merge 37 commits into
base: wonu606
Choose a base branch
from

Conversation

wonu606
Copy link

@wonu606 wonu606 commented Jul 25, 2023

📌 과제 설명

로직

image
  • Voucher는 생성이 가능합니다.
  • Voucher는 가지고 있는 Customer를 탐색할 수 있습니다.
  • Voucher를 assgin Wallet하게 되면 지갑에 등록이 되고 Customer는 할당되지 않는 상태로 추가됩니다.
image
  • Customer는 생성 가능합니다.
  • Customer는 가지고 있는 Voucher를 탐색할 수 있습니다.
image
  • Customer는 지갑에 등록된 Voucher를 등록할 수 있고, Voucher가 등록되어 있지 않는다면 등록이 되지 않습니다.

데이터베이스 설계

image

  • Voucher와 Wallet은 연관이 있다고 생각하여 외래키를 사용하여 묶었습니다.
  • Customer는 아이디로도 충분하다고 탐색이 가능하다 판단하여 외래키를 사용하지 않았습니다.

image

  • Converter와 DTO를 이용하여 패키지가 단방향 의존할 수 있게 구현하였습니다.
  • Controller에서 여러 Service를 참조하는 것보다 Service에서 참조하는 것이 각 계층의 역할에 바람직하다고 느껴 수정하였습니다.

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

  • Thymeleaf
  • API

✅ PR 포인트 & 궁금한 점

  • Converter를 구현하면서 Repository와 Domain을 분리하여 두었는데, 굳이 2개를 분할할 필요를 느끼지 못했습니다.
    분할하니 의미 없는 convert만 처리하여 넘겨준다고 느껴졌습니다.
    2 계층을 1개의 계층이라 보고 묶어도 괜찮을까요?
  • Repository와 Reader, Store로 나누었을 때 아직 마스터 슬레이브 부하에 대해 느끼진 못했지만,
    Reader가 뱉는 DTO가 일치하고, Store가 뱉는 DTO가 일치한다는 것을 알 수 있었습니다.
    이 경우도 Reader와 Store를 나누는 것 중 이점이라고 볼 수 있을까요? 아니면 프로젝트가 작아서 생기는 우연인지 궁금합니다.


public CustomerController(CustomerService service) {
this.service = service;
converterManager = new CustomerControllerConverterManager();
Copy link

Choose a reason for hiding this comment

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

new로 생성해야만 하는 이유가 있는지 궁금합니다.

Comment on lines +55 to +58
List<CustomerResult> results = service.getCustomerList();
List<CustomerResponse> responses = results.stream()
.map(rs -> converterManager.convert(rs, CustomerResponse.class))
.collect(Collectors.toList());
Copy link

Choose a reason for hiding this comment

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

List results를 convter한테 넘겨서 받을순 없을까요?

Comment on lines +9 to +20
private final List<TypedConverter<?, ?>> converterList;

public CustomerControllerConverterManager() {
converterList = new ArrayList<>();
converterList.add(new CustomerCreateParamConverter());
converterList.add(new CustomerCreateResponseConverter());
converterList.add(new CustomerResponseConverter());
converterList.add(new OwnedVouchersParamConverter());
converterList.add(new OwnedVoucherResponseConverter());
converterList.add(new WalletDeleteParamConverter());
converterList.add(new WalletRegisterParamConverter());
}
Copy link

Choose a reason for hiding this comment

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

DTO가 늘어나거나 삭제될때마다 이 코드도 수정해야 할것같아요.

1:1로 메소드로 맵핑하면 어떨까요?

Comment on lines +62 to +87
@PostMapping("/owned-vouchers")
public ResponseEntity<List<OwnedVoucherResponse>> getOwnedVouchersByCustomer(
@RequestBody OwnedVouchersRequest request) {
OwnedVouchersParam param = converterManager.convert(request, OwnedVouchersParam.class);
List<OwnedVoucherResult> results = service.findOwnedVouchersByCustomer(param);

List<OwnedVoucherResponse> responses = results.stream()
.map(rs -> converterManager.convert(rs, OwnedVoucherResponse.class))
.collect(Collectors.toList());

return new ResponseEntity<>(responses, HttpStatus.OK);
}

@PostMapping("/wallet/delete")
public ResponseEntity<Void> deleteWallet(@RequestBody WalletDeleteRequest request) {
WalletDeleteParam param = converterManager.convert(request, WalletDeleteParam.class);
service.deleteWallet(param);
return new ResponseEntity<>(HttpStatus.OK);
}

@PostMapping("/wallet/register")
public ResponseEntity<Void> registerToWallet(@RequestBody WalletRegisterRequest request) {
WalletRegisterParam param = converterManager.convert(request, WalletRegisterParam.class);
service.registerToWallet(param);
return new ResponseEntity<>(HttpStatus.CREATED);
}
Copy link

Choose a reason for hiding this comment

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

rest api url의 규칙에 맞지 않는것 같습니다

@@ -0,0 +1,20 @@
package com.wonu606.vouchermanager.controller.customer.response;

public class CustomerResponse {
Copy link

Choose a reason for hiding this comment

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

record 사용해도 될것같아요

Comment on lines +10 to +13
this.uuid = uuid;
Type = type;
this.value = value;
}
Copy link

Choose a reason for hiding this comment

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

통일성을 맞춰주세요

Choose a reason for hiding this comment

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

Type이 변수명이었구나;

@Transactional
public class CustomerJdbcRepository implements CustomerRepository {

CustomerReader reader;
Copy link

Choose a reason for hiding this comment

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

접근지정자를 지정안하신 의도가 있을까요?

Comment on lines +27 to +34

@Override
public void deleteByCustomerId(String email) {
String deletionSql = "DELETE FROM customer WHERE email = :email";
Map<String, Object> params = new HashMap<>();
params.put("email", email);
namedParameterJdbcTemplate.update(deletionSql, params);
}
Copy link

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 +20
VoucherReader reader;
VoucherStore store;
Copy link

Choose a reason for hiding this comment

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

접근지정자를 지정하면 좋을것같아요

Comment on lines +10 to +13
@Override
public OwnedCustomerResultSet mapRow(ResultSet rs, int rowNum) throws SQLException {
return new OwnedCustomerResultSet(rs.getString("customer_id"));
}
Copy link

Choose a reason for hiding this comment

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

이 클래스랑 메소드의 의미를 명확하게 해주면 좋을것 같아요

@devYSK
Copy link

devYSK commented Jul 29, 2023

안녕하세요 원우님 과제하느냐 고생많으셨습니다.
전반적으로 중복되는 코드들이 많아서 리뷰 보시고 한번 맞춰 정리하면 될 것 같습니다.

  • Repository와 Reader, Store로 나누었을 때 아직 마스터 슬레이브 부하에 대해 느끼진 못했지만,
Reader가 뱉는 DTO가 일치하고, Store가 뱉는 DTO가 일치한다는 것을 알 수 있었습니다.
이 경우도 Reader와 Store를 나누는 것 중 이점이라고 볼 수 있을까요? 아니면 프로젝트가 작아서 생기는 우연인지 궁금합니다.

관점의 차이인데 개인적으로는 둘이 일치해서 같은 걸 사용하면 단점이 생길 수 있다고 생각해요.
혹여나 나중에 요구사항이 둘이 바뀌게 된다면 의도치않게 많은 부분을 수정해야 겠죠.
현재는 프로젝트가 작아서 생기는 우연일 수 있지 않을까? 생각합니다

Copy link

@WooSungHwan WooSungHwan left a comment

Choose a reason for hiding this comment

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

원우님 바우처 과제하시느라 수고 많으셨습니다. 끝까지 성실하게 마무리해주셔서 감사드리고 다음에는 바우처 과제에서 아쉬웠던 점과 좋았던 점을 보완하고 다음과제에서 더 좋은 모습으로 만나요~

converterManager = new CustomerControllerConverterManager();
}

@PostMapping("/create")

Choose a reason for hiding this comment

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

consumes 속성에 대해서도 알아보셔요ㅎㅎ

Comment on lines +15 to +21
public Class<OwnedVouchersRequest> getSourceType() {
return OwnedVouchersRequest.class;
}

@Override
public Class<OwnedVouchersParam> getTargetType() {
return OwnedVouchersParam.class;

Choose a reason for hiding this comment

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

추후엔 이렇게 .class를 호출해서 Class 타입을 처리하는 리플렉션을 이용한 방법보단 쉬운 방법을 택해보시죠.

import org.springframework.web.bind.annotation.RequestMapping;

@Controller
@RequestMapping("vouchers")

Choose a reason for hiding this comment

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

/vouchers
스프링이 알아서 붙여주긴할텐데. 어딘 붙고 어딘 안붙으면 안돼요.

Comment on lines +10 to +13
this.uuid = uuid;
Type = type;
this.value = value;
}

Choose a reason for hiding this comment

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

Type이 변수명이었구나;

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.

None yet

3 participants