Open
Conversation
Collaborator
LSH0809
commented
Nov 15, 2021
LSH0809
commented
- 기존의 컨트롤러의 분기 -> 서비스 레이어에서 분기 발생하도록 수정
- 서비스레이어에서 분기 발생할 수 있도록 메소드 정리
- 기존의 컨트롤러의 분기 -> 서비스 레이어에서 분기 발생하도록 수정 - 서비스레이어에서 분기 발생할 수 있도록 메소드 정리
samkimuel
requested changes
Nov 16, 2021
Collaborator
samkimuel
left a comment
samkimuel
left a comment
There was a problem hiding this comment.
리팩토링 하셨군요👍🏻
코멘트 하나 남겼으니 확인해주세욥!!
Comment on lines
94
to
100
| if (existsSameBid(id, size, bidRequest.userId(), DealStatus.BIDDING.getStatus())) { | ||
| SellingBid sellingBid = findSellingBid(id, size, bidRequest.userId()); | ||
| sellingBid.updateSellingBid(bidRequest.price(), bidRequest.deadline()); | ||
|
|
||
| return sellingBid.toBidResponse(bidRequest); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
이 부분이 업데이트 로직인가용??
구매 입찰 내역을 업데이트 하게 되면 최저가 업데이트도 필요하다고 생각됩니다!
예를 들어, 지금 업데이트 하는 구매 입찰 내역이 최저가였는데, 더 높은 가격으로 수정하게 되면 최저가도 바뀌게 될 것 같아용
Collaborator
Author
There was a problem hiding this comment.
8ce85a3 : 아! 제가 수정하면서 이 부분을 놓쳤군요.. 알려주셔서 감사합니다!!! 이렇게 작성하니 일단 서비스 레이어에서 분기가 발생하도록 수정했는데 코드가 분리될 부분이 또 보이네요.. 이 부분도 나중에 한번 고쳐보겠습니다!!!
buri-1029
approved these changes
Nov 18, 2021
Comment on lines
-38
to
-48
| if (sellingService.existsSameBid( | ||
| id, | ||
| size, | ||
| bidRequest.userId(), | ||
| DealStatus.BIDDING.getStatus()) | ||
| ) { | ||
| return ResponseEntity.ok(ApiResponse.of( | ||
| sellingService.updateSellingBid(id, size, bidRequest))); | ||
| } | ||
| return ResponseEntity.ok(ApiResponse.of( | ||
| sellingService.registerSellingBid(id, size, bidRequest))); |
Collaborator
There was a problem hiding this comment.
상태에 따른 서비스 로직을 구별해 준 부분을 컨트롤러 단이 아닌 서비스 단에서 해주는 게 맞는 것 같고 리팩토링 하니 가독성도 좋아 보이네요 굳굳
Collaborator
Author
- 기존의 최저가 변경 메소드 위치를 메소드 최하단으로 변경 - 위 로직이 모두 정상적으로 진행될 때 업데이트가 되도록 수정
- return 으로 인해 최저가 갱신이 되지 않는 것을 수정 - updateLowestPrice 메소드를 추가
samkimuel
requested changes
Dec 31, 2021
Collaborator
samkimuel
left a comment
samkimuel
left a comment
There was a problem hiding this comment.
조금(?) 늦게 리뷰를 남기네요 ㅎ,,, 파이팅입니다!
| @ApiOperation(value = "판매입찰등록 및 수정", notes = "상품Id와 신발사이즈,입찰정보(dto)를 통해 입찰을 등록 혹은 갱신합니다.") | ||
| @PutMapping("/{id}") | ||
| public ResponseEntity<ApiResponse<BidResponse>> registerSellingBid( | ||
| public ResponseEntity<ApiResponse<BidResponse>> registerSellingBidRefactor( |
Collaborator
There was a problem hiding this comment.
깃에서 버전컨트롤이 되기 떄문에 메서드명은 그대로 가셔도 좋을 것 같아욥!
Comment on lines
-38
to
-48
| if (sellingService.existsSameBid( | ||
| id, | ||
| size, | ||
| bidRequest.userId(), | ||
| DealStatus.BIDDING.getStatus()) | ||
| ) { | ||
| return ResponseEntity.ok(ApiResponse.of( | ||
| sellingService.updateSellingBid(id, size, bidRequest))); | ||
| } | ||
| return ResponseEntity.ok(ApiResponse.of( | ||
| sellingService.registerSellingBid(id, size, bidRequest))); |
Comment on lines
+93
to
99
| if (existsSameBid(id, size, bidRequest.userId(), DealStatus.BIDDING.getStatus())) { | ||
| SellingBid sellingBid = findSellingBid(id, size, bidRequest.userId()); | ||
| sellingBid.updateSellingBid(bidRequest.price(), bidRequest.deadline()); | ||
| updateLowestPrice(bidRequest, productOption); | ||
| return sellingBid.toBidResponse(bidRequest); | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
existsSameBid의 매개변수들을 이용해서 find + ~ 쿼리 메서드를 만든다면 Optional로 반환받을 수 있을 것 같고, 그렇게 되면 if문은 사용하지 않아도 될 것 같습니다!
buri-1029
approved these changes
Jan 3, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.