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 Weekly Mission 여섯 번째 PR 제출합니다. #871
base: smart-sangmin
Are you sure you want to change the base?
[4기 - 박상민] SpringBoot Weekly Mission 여섯 번째 PR 제출합니다. #871
Conversation
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.
리뷰 남겼습니다~
public class Page<T> { | ||
private final List<T> contents; | ||
private final int numberOfElements; | ||
private final Pageable pageable; |
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.
Pageable
을 인스턴스 멤버로 가지고 있을 필요가 있나요?
Page<T>
가 가진 정보와 중복된 데이터가 많아서, 필요한 데이터만 들고있으면 될 것 같아서요
|
||
import java.util.UUID; | ||
|
||
public record CustomerUpdateDto( |
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.
validation은 필요 없을까요?
import java.util.UUID; | ||
import java.util.concurrent.ConcurrentHashMap; | ||
|
||
@Repository |
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.
MemoryVoucherRepository
profile로 분리해줘도 좋을 것 같아요
spring: | ||
config: | ||
activate: | ||
on-profile: web |
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.
이건 없어도 되지 않나요?
@GetMapping("/vouchers") | ||
@ResponseStatus(HttpStatus.OK) | ||
public Page<VoucherResponse> findVouchers(@ModelAttribute Pageable pageable) { | ||
return voucherService.findVoucherByPage(pageable); | ||
} | ||
|
||
@GetMapping("/all") | ||
@ResponseStatus(HttpStatus.OK) | ||
public VoucherResponses listVouchers() { | ||
return voucherService.list(); | ||
} |
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는 합쳐도 좋을 것 같네요
@GetMapping("/{email}") | ||
@ResponseStatus(HttpStatus.OK) | ||
public CustomerResponse findCustomer(@PathVariable @Valid @Email String email) { | ||
return customerService.findByEmail(email); | ||
} |
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.
@Valid
가 동작하나요??
public UUID save(Voucher voucher) { | ||
if (storage.containsKey(voucher.getVoucherId())) { | ||
throw new IllegalStateException(ALREADY_EXISTS); | ||
} | ||
storage.put(voucher.getVoucherId(), voucher); | ||
return voucher.getVoucherId(); | ||
} |
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.
이미 저장되어 있으면 예외를 던지는 이유가 있을까요?
📌 과제 설명
✅ 피드백 반영사항
#865 (review)
save 랑 update는 에러 발생시키도록 변경하였고 find는 Optional.empty() 반환하도록 변경하였습니다.