-
Notifications
You must be signed in to change notification settings - Fork 170
[4기 - 배준일] SpringBoot Part3 WeeklyMission 제출합니다. #868
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
base: bjo6300/w3
Are you sure you want to change the base?
Conversation
[2기-C] 정하영 SpringBoot Part1 Weekly Mission 제출입니다.
This reverts commit 2cb94a0.
…evert-239-main Revert Main Branch
[2기-C] 정다현 SpringBoot Part1 Weekly Mission 제출합니다.
…evert-218-week1 Revert main Branch prgrms-be-devcourse#218
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.
준일님 많이 바쁘셨을텐데 3주차 과제까지 고생하셨습니다~~👍👍
웹으로 전환하면서 무엇을 느끼셨을지 궁금하네요~ 그리고 Swagger는 어떻게 적용하신거죠..? 😅
전체적으로 아직 완성된 코드가 아닌것 같은 느낌이 들었습니다~ 여유 시간이 되신다면 좀 더 연습해보는 시간을 가져보시길 권장드립니다~~ 😄
@SpringBootApplication | ||
public class CommandLineApplication implements CommandLineRunner { | ||
|
||
private static final Logger LOG = LoggerFactory.getLogger(CommandLineApplication.class); | ||
private final ConsoleRunner consoleRunner; | ||
|
||
public CommandLineApplication(ConsoleRunner consoleRunner) { | ||
this.consoleRunner = consoleRunner; | ||
} | ||
|
||
public static void main(String[] args) { | ||
LOG.info("voucher가 실행되었습니다."); | ||
SpringApplication.run(CommandLineApplication.class, args); | ||
} | ||
|
||
@Override | ||
public void run(String... args) { | ||
consoleRunner.run(); | ||
} | ||
} |
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.
CommandLineRunner를 CommandLineApplication에서 implements하게 되면 웹과 커멘드어플리케이션이 동시에 실행하게 될 것 같네요!! 같이 실행이 되어야 하는 이유가 있나요?
저라면 Spring Profile
을 활용해서 web
과 console
환경을 분리할 수 있도록 할 것 같습니다~!
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class ConsoleRunner implements Runnable { |
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.
위에서도 언급했지만 저라면 해당 객체를 Runnable
이 아닌 CommandLineRunner
을 구현하도록 할 것 같네요. 혹시 차이점이 있을까요?
public CustomerJdbcRepository(DataSource dataSource) { | ||
this.template = new NamedParameterJdbcTemplate(dataSource); | ||
} |
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.
VoucherJdbcRepository
에서는 NamedParameterJdbcTemplate
로 주입 받는데 다르게 하신 이유가 있나요? 일관성을 지키지 않으면 같이 개발하는 동료들이 헷갈릴 가능성이 생길 수 있습니다~
LOG.error("customer가 수정되지 않았습니다."); | ||
throw new RuntimeException("customer가 수정되지 않았습니다."); |
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.
RuntimeException
보다 의미를 더욱 잘 표현할 수 있는 예외를 사용하면 좋을 것 같습니다! 그리고 log를 보고 어떤 고객이 수정에 실패했는지 알 수 있나요? 로그에 수정되지 않았다는 메시지만 잔뜩 있으면 디버깅에 활용할 수 있을까요?
public abstract class Voucher { | ||
|
||
public abstract UUID getVoucherId(); | ||
|
||
public abstract double discountedPrice(long beforeDiscount); | ||
|
||
public abstract VoucherType getVoucherType(); | ||
|
||
public abstract long getDiscount(); | ||
} |
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.
제가 생각하기는 현재의 객체 형태나 구조는 인터페이스같은데 추상 클래스로 하신 이유가 있나요?
@Transactional | ||
public void createVoucher(VoucherType inputVoucherType, Long inputDiscount) { | ||
VoucherCreateRequest voucherCreateRequest = voucherMapper.toCreateRequest(UUID.randomUUID(), | ||
inputVoucherType, | ||
inputDiscount); | ||
Voucher voucher = voucherFactory.create(voucherCreateRequest); | ||
|
||
save(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.
Controller
에서 VoucherCreateRequest
를 받고 있는데 다시 매개변수로 풀고 난 후 또 다시 재조립하고 있는 것 같은데 이유가 궁금하네요..
public List<Voucher> findVouchersByVoucherType(VoucherType voucherType) { | ||
return findVouchersByVoucherType(voucherType) | ||
.stream() | ||
.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.
무한 반복을 하고 있네요..😅
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.
voucherMemoryRepository인걸 깜빡했네요 😅
12e5b0c
|
||
@GetMapping("/vouchers/{voucherId}") | ||
public ResponseEntity<VoucherResponse> getVoucher( | ||
@Valid @PathVariable String 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.
무엇을 검증하고 있나요..? @Valid
의 동작에 대해서 공부해보시면 좋을 것 같아요~! (참고)
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에 대해 자세히 모르고 있었네요...! 감사합니다!
@Controller | ||
@RequiredArgsConstructor | ||
@Slf4j | ||
public class VoucherWebController { | ||
|
||
private final VoucherService voucherService; |
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.
불필요한 어노테이션 제거했습니다!
43292e2
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.
ExceptionHandler 와 ControllerAdvice에 대해서도 알아보시죠~~ (참고 자료)
@Builder | ||
@Getter | ||
@Setter | ||
@AllArgsConstructor | ||
@NoArgsConstructor | ||
public class VoucherCreateRequest { | ||
|
||
private UUID voucherId; | ||
private VoucherType voucherType; | ||
private Long discountAmount; | ||
|
||
public VoucherCreateRequest(VoucherType voucherType, Long discountAmount) { | ||
this.voucherType = voucherType; | ||
this.discountAmount = discountAmount; | ||
} | ||
} |
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에서도 final
을 적용할 수 있는 방법에 대해 알아보시면 좋을 것 같네요~! 그리고 바우처를 생성하는 요청인데 외부에서 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.
안녕하세요 준일님 :)
과제하느라 고생하셨습니다 👍
조금 더 보충하면 좋겠다고 드는 부분들이 있는데,
- 준일님 코드 전부에 일관성이 있었으면 좋겠습니다. 지금은 뭔가 정리되지 않은 느낌들이 꽤 있어요. 이건 주성님이 리뷰 남겨주신 것도 있어서 확인해주시면 좋을 것 같아요.
- 로그를 남기고 계신데, 제 생각에 의미있는 로그가 없는 것 같아요. 단순 텍스트를 로그로 남기는 것은 불필요한 용량만 차지할 수 있다는 것을 생각해주시면 좋겠어요!
위 부분들 + 리뷰 남긴 것들 잘 챙기시면 다음 미션때 더 발전된 모습을 기대할 수 있을 것 같아요 ㅎㅎ
수고하셨습니다!
} | ||
|
||
public static void main(String[] args) { | ||
LOG.info("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.
실행되었다는 로그를 남길 필요가 있을까요?
어플리케이션 실행에 대한 로그는 이 부분이 없어도 남을 것 같은데, 어떻게 생각하시나요?
private void checkCustomerName() { | ||
if (this.customerName.isBlank()) { | ||
throw new IllegalArgumentException("이름은 빈칸이 될 수 없습니다."); | ||
} | ||
if (this.customerName.length() > CUSTOMER_NAME_LIMIT) { | ||
throw new IllegalArgumentException("이름은 " + CUSTOMER_NAME_LIMIT + "자를 넘을 수 없습니다."); | ||
} | ||
} | ||
|
||
private void checkCustomerEmail() { | ||
if (this.customerEmail.isBlank()) { | ||
throw new RuntimeException("이메일은 빈칸이 될 수 없습니다."); | ||
} | ||
if (this.customerEmail.length() > CUSTOMER_EMAIL_LIMIT) { | ||
throw new IllegalArgumentException("이름은 " + CUSTOMER_EMAIL_LIMIT + "자를 넘을 수 없습니다."); | ||
} | ||
} |
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.
유효성 검사를 할 때, check도 좋지만 validate를 더 일반적으로 사용하고 있는 것 같아요.
좋은 함수 이름 짓기에 대한 글들을 찾아보시면 좋을 것 같네요 👍
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.
함수 네이밍에 대해 공부해보겠습니다!
43292e2
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.
int saved = template.update(sql, param); | ||
|
||
if (saved != SUCCESS_EXECUTE) { | ||
LOG.error("customer가 저장되지 않았습니다."); |
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.
준일님은 로그를 남기는 목적 무엇일까요?
해당 로그를 보고 어떤 customer가 저장되지 않았는지 알 수 없지 않을까요?
voucherCreateRequest.getDiscountAmount()); | ||
|
||
return ResponseEntity.status(HttpStatus.CREATED) | ||
.body("voucher가 생성되었습니다. " + voucherCreateRequest.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.
respose body에 "voucher가 생성되었습니다. "과 같은 텍스트를 넣으신 이유가 있을까요?
제가 API를 받아보는 입장이라면 불필요한 정보일 것 같아요.
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에 어떤 메세지를 보내야할지 고민했습니다. 생성 시에는 생성된 voucher의 Id만 보내도 API를 받는 입장에서는 상관없을까요?
또한 삭제 시에도 메세지없이 상태코드로만 판단해도 괜찮은지 궁금합니다!
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를 요청하고 응답받을 때, 메시지가 필요할까에 대해서 생각해보면 저는 전혀 필요없다고 생각합니다.
우리가 원하는 데이터의 이동만 필요할 뿐, "voucher가 생성되었습니다." 같은 것은 status 코드로 충분히 표현할 수 있다고 생각이 듭니다.
삭제 시에도 마찬가지라고 생각합니다.
|
||
|
||
|
||
|
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.
수정했습니다!
094be92
📌 과제 설명
(기본) 바우처 서비스 관리페이지 개발하기
(기본) 바우처 서비스의 API 개발하기
<실행 후 링크>
Thymeleaf link : http://localhost:8080/
Rest API link (swagger로 테스트 할 수 있습니다.) : http://localhost:8080/swagger-ui/index.html#/
✅ 피드백 반영사항
✅ PR 포인트 & 궁금한 점