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
base: wonu606
Are you sure you want to change the base?
Conversation
|
||
public CustomerController(CustomerService service) { | ||
this.service = service; | ||
converterManager = new CustomerControllerConverterManager(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new로 생성해야만 하는 이유가 있는지 궁금합니다.
List<CustomerResult> results = service.getCustomerList(); | ||
List<CustomerResponse> responses = results.stream() | ||
.map(rs -> converterManager.convert(rs, CustomerResponse.class)) | ||
.collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List results를 convter한테 넘겨서 받을순 없을까요?
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()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTO가 늘어나거나 삭제될때마다 이 코드도 수정해야 할것같아요.
1:1로 메소드로 맵핑하면 어떨까요?
@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); | ||
} |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
record 사용해도 될것같아요
this.uuid = uuid; | ||
Type = type; | ||
this.value = value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
통일성을 맞춰주세요
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
접근지정자를 지정안하신 의도가 있을까요?
|
||
@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); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메소드랑 쿼리랑 다른일을 하고있어요
VoucherReader reader; | ||
VoucherStore store; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
접근지정자를 지정하면 좋을것같아요
@Override | ||
public OwnedCustomerResultSet mapRow(ResultSet rs, int rowNum) throws SQLException { | ||
return new OwnedCustomerResultSet(rs.getString("customer_id")); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 클래스랑 메소드의 의미를 명확하게 해주면 좋을것 같아요
안녕하세요 원우님 과제하느냐 고생많으셨습니다.
관점의 차이인데 개인적으로는 둘이 일치해서 같은 걸 사용하면 단점이 생길 수 있다고 생각해요. |
There was a problem hiding this 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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consumes 속성에 대해서도 알아보셔요ㅎㅎ
public Class<OwnedVouchersRequest> getSourceType() { | ||
return OwnedVouchersRequest.class; | ||
} | ||
|
||
@Override | ||
public Class<OwnedVouchersParam> getTargetType() { | ||
return OwnedVouchersParam.class; |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/vouchers
스프링이 알아서 붙여주긴할텐데. 어딘 붙고 어딘 안붙으면 안돼요.
this.uuid = uuid; | ||
Type = type; | ||
this.value = value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type
이 변수명이었구나;
📌 과제 설명
로직
데이터베이스 설계
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점
분할하니 의미 없는 convert만 처리하여 넘겨준다고 느껴졌습니다.
2 계층을 1개의 계층이라 보고 묶어도 괜찮을까요?
Reader가 뱉는 DTO가 일치하고, Store가 뱉는 DTO가 일치한다는 것을 알 수 있었습니다.
이 경우도 Reader와 Store를 나누는 것 중 이점이라고 볼 수 있을까요? 아니면 프로젝트가 작아서 생기는 우연인지 궁금합니다.