-
Notifications
You must be signed in to change notification settings - Fork 0
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
[feat] 식당 좋아요 관련 api #43
Conversation
private final HeartCommandService heartCommandService; | ||
|
||
@PostMapping("/stores/{storeId}/heart") | ||
public HankkiResponse<CreateHeartResponse> heartStore(@UserId Long userId, @PathVariable Long storeId) { |
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.
command DTO 네이밍처럼 HeartCreateResponse가 더 적절하지 않을까요?
@@ -36,4 +36,8 @@ public class Store extends BaseTimeEntity { | |||
@Column(nullable = false) | |||
private boolean isDeleted; | |||
|
|||
public void updateHearCount(boolean isDeleted) { |
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 문으로 변경해주세요
@@ -24,6 +24,10 @@ public User getUser(final long userId) { | |||
.orElseThrow(() -> new NotFoundException(UserErrorCode.USER_NOT_FOUND)); | |||
} | |||
|
|||
public User getUserReference(final long userId) { |
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.
Long으로 변경해주세요
UserInfo userInfo = UserInfo.createMemberInfo(user, null); | ||
userInfoSaver.saveUserInfo(userInfo); | ||
userInfoUpdater.saveUserInfo(userInfo); | ||
} | ||
|
||
private void validateRefreshToken(final String refreshToken, final long userId) { |
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.
Long으로 변경해주세요
|
||
private final HeartRepository heartRepository; | ||
|
||
public Optional<Heart> findByUserIdAndStoreId(Long userId, Long storeId) { |
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.
여기 final 키워드가 빠진 것 같습니다.
.orElseThrow(()-> new NotFoundException(StoreErrorCode.STORE_NOT_FOUND)); | ||
} | ||
|
||
public Store getStoreReference(Long 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.
final를 달아주세용
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.
반영할거 반영해주세요~
|
||
@PostMapping("/stores/{id}/hearts") | ||
public HankkiResponse<HeartCreateResponse> createHeartStore(@UserId Long userId, @PathVariable Long id) { | ||
return HankkiResponse.success(CommonSuccessCode.CREATED, heartCommandService.createHeart(StorePostCommand.of(userId, id))); | ||
} | ||
|
||
@DeleteMapping("/stores/{id}/hearts") | ||
public HankkiResponse<HeartDeleteResponse> deleteHeartStore(@UserId Long userId, @PathVariable Long id) { | ||
return HankkiResponse.success(CommonSuccessCode.OK, heartCommandService.deleteHeart(StoreDeleteCommand.of(userId, 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.
매개변수에 final 추가해주세요
heartFinder.findByUserIdAndStoreId(userId, storeId) | ||
.ifPresent(heart -> { | ||
throw new ConflictException(HeartErrorCode.ALREADY_EXISTED_HEART); | ||
}); | ||
} |
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.
existsby로 boolean 값만 받아오는 것이 더 나아 보입니다.
뭔가 ValidateCreate이 중복돼서 의미가 한번에 안 와닿는데,
validateCreateStoreHeart는 어떨까요?
private void validateDeleteStoreHeart(final Long userId, final Long storeId) { | ||
heartFinder.findByUserIdAndStoreId(userId, storeId) | ||
.orElseThrow(() -> new ConflictException(HeartErrorCode.ALREADY_DELETED_HEART)); | ||
} | ||
|
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.
여기도 existby로 바꿔주시고
사소하지만 애초에 좋아요가 없는 경우도 에러코드 상수의 이름은 ALREADY_DELETED_HEART보다 모든 경우를 나타낼 수 있도록 변경해주세요
Heart heart = Heart.createHeart(userFinder.getUserReference(userId), storeFinder.getStoreReference(storeId)); | ||
heartUpdater.saveHeart(heart); | ||
} | ||
|
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.
굳이 heart 변수 안 써도 될 거 같습니다. 바로 saveHeart에 넣어주세요
validateDeleteStoreHeart(userId, storeId); | ||
heartDeleter.deleteHeart(userId,storeId); | ||
decreaseStoreHeartCount(storeId); | ||
return HeartDeleteResponse.of(storeId, false); |
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보단 객체를 받아주세요
private void increaseStoreHeartCount(final Long storeId) { | ||
storeFinder.getStore(storeId).increaseHeartCount(); | ||
} | ||
|
||
private void decreaseStoreHeartCount(final Long storeId) { | ||
storeFinder.getStore(storeId).decreaseHeartCount(); | ||
} |
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.
매개변수로 storeId를 받기보다 Store자체를 받아주세요.
storeFinder.getStore를 호출하는 것은 increaseStoreHeartCount함수의 책임을 넘어섭니다.
위에서 finder에서 getReferenceById로 lazyloading해서 가져오고 여기에서는 increaseHeartCount만 호출해주세요
Heart heart = Heart.createHeart(userFinder.getUserReference(userId), storeFinder.getStoreReference(storeId)); | ||
heartUpdater.saveHeart(heart); |
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.
객체를 매개변수로 받고 한줄로 줄여주세요. heart변수 만들 필요 없어 보입니다.
public void decreaseHeartCount() { | ||
heartCount--; | ||
} | ||
|
||
public void increaseHeartCount() { | ||
heartCount++; | ||
} |
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.
사소하지만 가독성 위해 this를 추가하는 것은 어떨까요
public static StoreDeleteCommand of(Long userId, Long storeId) { | ||
return new StoreDeleteCommand(userId, storeId); | ||
} | ||
} | ||
|
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.
여기도 통일성을 위해 final 붙여주세요
Related Issue 📌
close #42
Description ✔️
To Reviewers
삭제 시에 상태코드를 204와 200 중 고민하였는데, 클라 측에서 삭제 후에 응답값을 요청하기도 하였고 아래 링크에 의하면 응답 메시지가 삭제 이후의 상태를 설명하는 경우에는 200을 써도 된다고 기술되어 있어 상태코드를 200으로 설정하였습니다.
https://developer.mozilla.org/ko/docs/Web/HTTP/Methods/DELETE
더하여 현재 식당의 좋아요 갯수를 조정하는 부분을 isDeleted로 아래와 같이 한 함수에서 처리하였는데
위의 함수를 조건문으로 처리하는 대신
decreaseHeartCount
와IncreaseHeartCount
2개의 함수로 나누어 구현할 지 고민 중입니다. 의견 부탁드립니다.좋아요 생성 응답값
좋아요 삭제 응답값