[5기 - 김현우] Spring Boot Part3 Weekly Mission 제출합니다. #972
Conversation
ASak1104
changed the title
| case "0" -> ConsoleApplication.main(args); | ||
| case "1" -> WebApplication.main(args); |
There was a problem hiding this comment.
상수화로 enum같은걸로 해두면 클린코드 적으로 좋을거 같습니다.
There was a problem hiding this comment.
실행할 스프링 애플리케이션의 main 메서드를 enum에서 Consumer로 관리하는 방향으로 수정해봤습니다!
commit 7412a89
| .map(UUID::toString) | ||
| .anyMatch(id -> Objects.equals(id, voucherId)); | ||
| public boolean hasVoucher(UUID voucherId) { | ||
| Optional<Voucher> optionalVoucher = voucherRepository.findById(voucherId); |
There was a problem hiding this comment.
EXISTQuery랑 이거랑 어떤게 좋을지 한번 고민해보세요!
| Map<String, Object> customerFields = new HashMap<>() { | ||
| }; |
There was a problem hiding this comment.
| Map<String, Object> customerFields = new HashMap<>() { | |
| }; | |
| Map<String, Object> customerFields = new HashMap<>() |
| } | ||
|
|
||
| @GetMapping("/{id}") | ||
| public String viewCustomerById(@PathVariable("id") UUID id, Model model) { |
There was a problem hiding this comment.
| public String viewCustomerById(@PathVariable("id") UUID id, Model model) { | |
| public String viewCustomerById(@PathVariable UUID id, Model model) { |
생략 가능합니다 :)
| import team.marco.voucher_management_system.web_app.controller.CustomerController; | ||
|
|
||
| @ControllerAdvice(basePackageClasses = CustomerController.class) | ||
| public class ViewAdvice { |
There was a problem hiding this comment.
500에러 같은 경우에 방어 해놔도 좋습니다 :)
There was a problem hiding this comment.
500 에러는 아래 conversation에 있는 VoucherErrorController에서 처리하는 걸 의도해서 우선 다른 부분을 반영해보겠습니다!
| import org.springframework.web.bind.annotation.GetMapping; | ||
|
|
||
| @Controller | ||
| public class VoucherErrorController implements ErrorController { |
| public ResponseEntity<Voucher> create(CreateVoucherRequest createVoucherRequest) { | ||
| Voucher voucher = voucherService.create(createVoucherRequest); | ||
|
|
||
| return ResponseEntity.ok(voucher); |
There was a problem hiding this comment.
REST API
리소스 생성될때 201 status created를 내려주는데 현재는 200이 내려가네요!
There was a problem hiding this comment.
api 명세 규칙을 제대로 지키질 못했네요
감사합니다!
commit 51bfeea
| if (!isDeleted) { | ||
| return ResponseEntity.noContent() | ||
| .build(); | ||
| } | ||
|
|
||
| return ResponseEntity.ok() | ||
| .build(); |
| @GetMapping("/createdAt") | ||
| public List<Voucher> findByCreateAt(@RequestParam("from") | ||
| @DateTimeFormat(pattern = DATE_PATTERN) | ||
| LocalDateTime from, | ||
| @RequestParam("to") | ||
| @DateTimeFormat(pattern = DATE_PATTERN) | ||
| LocalDateTime to) { | ||
| return voucherService.findByCreateAt(from, to); | ||
| } | ||
|
|
||
| @GetMapping("/type/{type}") | ||
| public List<Voucher> findByType(@PathVariable("type") VoucherType voucherType) { | ||
| return voucherService.findByType(voucherType); | ||
| } |
There was a problem hiding this comment.
filering같은 경우엔 queryString으로 필터링을 하면 좋을거 같습니다.
| @@ -0,0 +1,6 @@ | |||
| package team.marco.voucher_management_system.web_app.dto; | |||
|
|
|||
| public record CreateCustomerRequest( | |||
There was a problem hiding this comment.
springValidate를 한번 써보시면 좋겠습니다~
| return voucherRepository.findAll(); | ||
| } | ||
|
|
||
| public Optional<Voucher> findById(UUID id) { |
There was a problem hiding this comment.
repository.findById 값이 null일 때 raise 하지 않는 이유가 있을까요??
There was a problem hiding this comment.
말씀해주신대로 NoSuchElementException을 발생시키고 ControllerAdvice를 통해 noContent Response를 리턴하도록 한 곳에서 관리하는 것도 좋은 방법일 것 같습니다!
제가 해당 코드에서 예외를 발생하지 않은 이유는 아래와 같이 200과 noContent에 대한 분기를 Controller에서 관리하기 위해서 였습니다.
// class VoucherController
@GetMapping("/{id}")
public ResponseEntity<Voucher> findById(@PathVariable("id") UUID id) {
Optional<Voucher> optionalVoucher = voucherService.findById(id);
return optionalVoucher.map(ResponseEntity::ok)
.orElse(ResponseEntity.noContent()
.build());
}| return walletRepository.unlink(customerId, voucherId); | ||
| } | ||
|
|
||
| public List<Customer> findCustomersByVoucherId(String voucherId) { | ||
| public List<Customer> findCustomersByVoucherId(UUID voucherId) { | ||
| List<UUID> customerIds = walletRepository.getCustomerIds(voucherId); | ||
|
|
||
| return voucherCustomerFacade.getCustomers(customerIds); |
There was a problem hiding this comment.
voucher를 가진 customer를 가져오는 의미를 function이 가지면 더 이해가 잘될 것 같습니다.
There was a problem hiding this comment.
기능적인 의미만을 생각해서 말씀해주신 부분을 간과한 것 같습니다.
감사합니다!
commit 70a79d9
| @@ -21,16 +20,14 @@ public VoucherCustomerFacadeImpl(VoucherRepository voucherRepository, CustomerRe | |||
| } | |||
There was a problem hiding this comment.
퍼사드 패턴을 꼭 사용해야할까요?? fasade를 이용해서 복잡한 로직을 단순화된 인터페이스로 제공하는 것도 아니고 각 서비스에서의 의존성을 줄여주는 용도도 아니라는 생각이 생각이 들어서 각 서비스 내부에 들어가도 괜찮다는 생각이 듭니다.
There was a problem hiding this comment.
확실히 현재 상황에서 퍼사드 클래스는 불필요한 것 같습니다.
2주차 과제에서는 VoucherRepository가 저장과 전체 조회 기능만을 제공했기 때문에 퍼사드 패턴을 사용했지만
commit
join 쿼리와 limit 1을 통해 존재 여부를 판단하는 쿼리를 추가하면 해당 클래스를 제거하고 성능과 구조에 대해 개선할 수 있을 것 같습니다!!
| @@ -6,9 +6,9 @@ | |||
| import team.marco.voucher_management_system.model.Voucher; | |||
|
|
|||
| public interface VoucherCustomerFacade { | |||
There was a problem hiding this comment.
어떤 방향으로 다형성을 이용할 수 있을지 잘 모르겠어서 현재는 해당 인터페이스와 같은 추상화가 필요하지 않을 것 같습니다.
👩💻 요구 사항
바우처 관리 Command-line Application
(기본) 서비스 관리페이지 개발하기
(기본) 바우처 서비스의 API 개발하기
🚀 애플리케이션 실행
프로젝트 초기 설정 및 애플리케이션 실행은 README.md 파일을 참고해주세요!
💡 고민했던 점