-
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
[4기 - 전선희] SpringBoot Part3 Weekly Mission 첫 번째 PR 제출합니다! #858
base: funnysunny08-week3
Are you sure you want to change the base?
[4기 - 전선희] SpringBoot Part3 Weekly Mission 첫 번째 PR 제출합니다! #858
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.
선희님 3주차 과제까지 정말 고생많으셨습니다!!! 👏👏👏
-
제가 실행 시켜봤는데 기본 데이터나 스키마 SQL이 없어서 테스트 해볼 수 없었네요.!! 실행할 수 있도록 추가해주면 더욱 자세하게 확인할 수 있을 것 같아요! 그리고 진입할 수 있는 인덱스 페이지가 있었으면 좋을 것 같네요. 😄
-
데브코스 과제를 단순히 제출을 위한 과제로 생각안하셨으면 좋겠습니다! 제출만 하는 것은 사실 큰 의미가 없습니다. 테스트 코드도 많이 작성해보시고, 다이어그램도 그려보고, 리드미에 내용도 추가하는 등 다양한 시도도 해보시면서 하나의 작은 프로그램 혹은 서비스를 만든다고 생각해보시면서 완성도를 높이면 좋을 것 같습니다!! 👍 (강요는 아닙니다~~!! JPA과제가 바쁘면 먼저 JPA 과제를 하는게 맞습니다.)
spring.datasource.username=root | ||
spring.datasource.password=0828 |
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.
보안 정보나 개인 정보를 깃허브에 올리시면 안됩니다.. 😢
@@ -0,0 +1,89 @@ | |||
15:20:48.186 [main] ERROR c.p.s.r.v.JdbcVoucherRepository - Got empty result |
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.
log 파일도 .gitignore에 추가해서 안올라오도록 하시면 좋을 것 같아요!
import static org.hamcrest.MatcherAssert.assertThat; | ||
import static org.hamcrest.Matchers.*; | ||
|
||
@SpringJUnitConfig |
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.
@JdbcTest
에 대해서 알아보셔도 좋을 것 같아요!
@TestMethodOrder(MethodOrderer.OrderAnnotation.class) | ||
@TestInstance(TestInstance.Lifecycle.PER_CLASS) |
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.
테스트 코드를 하나의 라이프사이클로 관리하시는 이유가 궁금합니다~
protected ApiResponse<Object> handleException(final Exception error, final HttpServletRequest request) throws IOException { | ||
return ApiResponse.error(Error.INTERNAL_SERVER_ERROR); | ||
} | ||
|
||
/** | ||
* custom error | ||
*/ | ||
@ExceptionHandler(CustomException.class) | ||
protected ResponseEntity<ApiResponse> handleSoptException(CustomException e) { | ||
return ResponseEntity.status(e.getHttpStatus()) | ||
.body(ApiResponse.error(e.getError(), e.getMessage())); |
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.
CustomException
은 final
이 아니네요.. 선희님은 handleException
의 매개변수를 왜 final
로 하셨나요? 그리고 이유가 있다면 통일성을 지키면 좋을 것 같아요! 달라야만 하는 이유가 있나요?
|
||
@AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
public class FixedAmountVoucher implements Voucher { | ||
private final String voucherName = "FixedAmountVoucher"; |
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.
VoucherType
이 있는데 voucherName
을 따로 두신 이유가 궁금합니다!
private final String voucherName = "PercentDiscountVoucher"; | ||
private final UUID voucherId; | ||
private final long discount; | ||
private final String discountUnit = "%"; |
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.
선희님은 어떤 필드에 static
을 사용하는게 적합하다고 생각하시나요?
switch (voucherType) { | ||
case PERCENT_DISCOUNT: | ||
discountUnit = "%"; | ||
break; | ||
case FIXED_AMOUNT: | ||
discountUnit = "$"; | ||
break; | ||
} |
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.
discountUnit
을 voucherType
의 필드로 두면 분기문이 필요 없을 것 같아요!
@Transactional | ||
public VoucherResponseDto createVoucher(VoucherCreateRequestDto requestDto) { | ||
Voucher voucher = null; | ||
if (VoucherType.valueOf(requestDto.getVoucherType()) == VoucherType.FIXED_AMOUNT) { |
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에서 VoucherType을 가져오는 것은 어떤가요?
|
||
@Transactional | ||
public void deleteVoucher(String voucherId) { | ||
Voucher voucher = voucherRepository.findById(UUID.fromString(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.
삭제할 데이터가 없으면 예외상황인가요? 선희님은 어떻게 생각하시나요~?
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에 대한 테스트 코드만 작성하셨는데 이유가 있으신지 궁금합니다ㅎ
@GetMapping("/{voucherId}") | ||
@ResponseStatus(HttpStatus.OK) | ||
public ApiResponse<VoucherResponseDto> getVoucherById(@PathVariable String voucherId) { | ||
return ApiResponse.success(Success.GET_VOUCHER_SUCCESS, voucherService.getVoucherById(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.
모든걸 한 줄에 표현하는건 가독성이 좋지 않은 문제가 있어요.
저라면 id로 값을 꺼낸 변수를 success 인자로 넣어줄 것 같아요.
그게 더 눈에 잘 들어오지 않을까요?
import lombok.Getter; | ||
import lombok.NoArgsConstructor; | ||
|
||
@Getter |
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 static void chkDiscountValue(long discount) { | ||
if (discount <= 0) throw new IllegalArgumentException("유효하지 않은 할인 금액"); |
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.
한 줄에 해주는 것도 좋지만, 중괄호가 있는 것이 전체적인 통일성 측면에서 좋을 것 같아요.
if (VoucherType.valueOf(requestDto.getVoucherType()) == VoucherType.FIXED_AMOUNT) { | ||
voucher = FixedAmountVoucher.newInstance(UUID.randomUUID(), requestDto.getDiscount()); | ||
} else if (VoucherType.valueOf(requestDto.getVoucherType()) == VoucherType.PERCENT_DISCOUNT) { | ||
voucher = PercentDiscountVoucher.newInstance(UUID.randomUUID(), requestDto.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.
else if를 안쓰고 어떻게 바꿔볼 수 있을까요?
else if를 쓰지말라고 하는 이유가 많은데 왜 인지 아실까요?
📌 과제 설명
6주차 강의에서 배운 내용들을 활용하여 바우처 관리 페이지 및 기능 페이지, API를 만들었습니다!!
👩💻 요구 사항과 구현 내용
✅ PR 포인트 & 궁금한 점