-
Notifications
You must be signed in to change notification settings - Fork 171
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
[5기 - 김현우] Spring Boot Part3 Weekly Mission 제출합니다. #972
base: ASak1104-hwkim-week3
Are you sure you want to change the base?
[5기 - 김현우] Spring Boot Part3 Weekly Mission 제출합니다. #972
Conversation
case "0" -> ConsoleApplication.main(args); | ||
case "1" -> WebApplication.main(args); |
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.
상수화로 enum같은걸로 해두면 클린코드 적으로 좋을거 같습니다.
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.
실행할 스프링 애플리케이션의 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EXISTQuery랑 이거랑 어떤게 좋을지 한번 고민해보세요!
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.
좋은 고민거리인 것 같습니다!
Map<String, Object> customerFields = new HashMap<>() { | ||
}; |
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.
Map<String, Object> customerFields = new HashMap<>() { | |
}; | |
Map<String, Object> customerFields = new HashMap<>() |
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.
commit 81d6f1e
} | ||
|
||
@GetMapping("/{id}") | ||
public String viewCustomerById(@PathVariable("id") UUID id, Model model) { |
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.
public String viewCustomerById(@PathVariable("id") UUID id, Model model) { | |
public String viewCustomerById(@PathVariable UUID id, Model model) { |
생략 가능합니다 :)
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.
commit 649f7ff
import team.marco.voucher_management_system.web_app.controller.CustomerController; | ||
|
||
@ControllerAdvice(basePackageClasses = CustomerController.class) | ||
public class ViewAdvice { |
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.
500에러 같은 경우에 방어 해놔도 좋습니다 :)
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.
500 에러는 아래 conversation에 있는 VoucherErrorController에서 처리하는 걸 의도해서 우선 다른 부분을 반영해보겠습니다!
import org.springframework.web.bind.annotation.GetMapping; | ||
|
||
@Controller | ||
public class VoucherErrorController implements ErrorController { |
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.
👍
public ResponseEntity<Voucher> create(CreateVoucherRequest createVoucherRequest) { | ||
Voucher voucher = voucherService.create(createVoucherRequest); | ||
|
||
return ResponseEntity.ok(voucher); |
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
리소스 생성될때 201 status created를 내려주는데 현재는 200이 내려가네요!
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.
api 명세 규칙을 제대로 지키질 못했네요
감사합니다!
commit 51bfeea
if (!isDeleted) { | ||
return ResponseEntity.noContent() | ||
.build(); | ||
} | ||
|
||
return ResponseEntity.ok() | ||
.build(); |
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.
commit f077d7d
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filering같은 경우엔 queryString으로 필터링을 하면 좋을거 같습니다.
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.
commit f949bd9
@@ -0,0 +1,6 @@ | |||
package team.marco.voucher_management_system.web_app.dto; | |||
|
|||
public record CreateCustomerRequest( |
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.
springValidate를 한번 써보시면 좋겠습니다~
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.
return voucherRepository.findAll(); | ||
} | ||
|
||
public Optional<Voucher> findById(UUID 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.
repository.findById 값이 null일 때 raise 하지 않는 이유가 있을까요??
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.
말씀해주신대로 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
voucher를 가진 customer를 가져오는 의미를 function이 가지면 더 이해가 잘될 것 같습니다.
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.
기능적인 의미만을 생각해서 말씀해주신 부분을 간과한 것 같습니다.
감사합니다!
commit 70a79d9
@@ -21,16 +20,14 @@ public VoucherCustomerFacadeImpl(VoucherRepository voucherRepository, CustomerRe | |||
} |
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.
퍼사드 패턴을 꼭 사용해야할까요?? fasade를 이용해서 복잡한 로직을 단순화된 인터페이스로 제공하는 것도 아니고 각 서비스에서의 의존성을 줄여주는 용도도 아니라는 생각이 생각이 들어서 각 서비스 내부에 들어가도 괜찮다는 생각이 듭니다.
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.
확실히 현재 상황에서 퍼사드 클래스는 불필요한 것 같습니다.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 방향으로 다형성을 이용할 수 있을지 잘 모르겠어서 현재는 해당 인터페이스와 같은 추상화가 필요하지 않을 것 같습니다.
👩💻 요구 사항
바우처 관리 Command-line Application
(기본) 서비스 관리페이지 개발하기
(기본) 바우처 서비스의 API 개발하기
🚀 애플리케이션 실행
프로젝트 초기 설정 및 애플리케이션 실행은 README.md 파일을 참고해주세요!
💡 고민했던 점