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 Weekly Mission 여섯 번째 PR 제출합니다. #871

Open
wants to merge 24 commits into
base: smart-sangmin
Choose a base branch
from

Conversation

smartandhandsome
Copy link

@smartandhandsome smartandhandsome commented Aug 6, 2023

📌 과제 설명

  • 바우처 API
  • 고객 API
  • Pagantion 구현

✅ 피드백 반영사항

#865 (review)
save 랑 update는 에러 발생시키도록 변경하였고 find는 Optional.empty() 반환하도록 변경하였습니다.

Copy link
Member

@dojinyou dojinyou left a comment

Choose a reason for hiding this comment

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

수고하셨습니다

Copy link

@seung-hun-h seung-hun-h left a 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;

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(

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

Choose a reason for hiding this comment

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

MemoryVoucherRepository profile로 분리해줘도 좋을 것 같아요

Comment on lines +20 to +23
spring:
config:
activate:
on-profile: web

Choose a reason for hiding this comment

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

이건 없어도 되지 않나요?

Comment on lines +36 to +46
@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();
}

Choose a reason for hiding this comment

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

두 api는 합쳐도 좋을 것 같네요

Comment on lines +36 to +40
@GetMapping("/{email}")
@ResponseStatus(HttpStatus.OK)
public CustomerResponse findCustomer(@PathVariable @Valid @Email String email) {
return customerService.findByEmail(email);
}

Choose a reason for hiding this comment

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

@Valid가 동작하나요??

Comment on lines +17 to +23
public UUID save(Voucher voucher) {
if (storage.containsKey(voucher.getVoucherId())) {
throw new IllegalStateException(ALREADY_EXISTS);
}
storage.put(voucher.getVoucherId(), voucher);
return voucher.getVoucherId();
}

Choose a reason for hiding this comment

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

이미 저장되어 있으면 예외를 던지는 이유가 있을까요?

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