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

CLEANUP: Reduce indent depth of SMGetResult merge method. #709

Merged
merged 1 commit into from
Dec 27, 2023

Conversation

uhm0311
Copy link
Collaborator

@uhm0311 uhm0311 commented Dec 27, 2023

Old, New 포함 SMGetResult의 Merge 메소드의 불필요한 Indent Depth를 줄였습니다.

Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

if (comp == 0) {
// Duplicated bkey. Compare the "cache key".
int keyComp = result.compareKeyTo(mergedResult.get(pos));
if (unique) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분 로직이 변경된 것 같은 데, 확인 바랍니다.

private void mergeTrimmedKeys(final List<SMGetTrimKey> eachTrimmedResult) {
if (eachTrimmedResult.isEmpty()) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

다른 경우에서도 eachResult가 empty인 경우 return 하는 코드가 없기 때문에,
일관되게 위의 if 문은 제거해도 될 것 같습니다.

@uhm0311 uhm0311 marked this pull request as draft December 27, 2023 08:55
@uhm0311 uhm0311 marked this pull request as ready for review December 27, 2023 08:58
@uhm0311
Copy link
Collaborator Author

uhm0311 commented Dec 27, 2023

@jhpark816

반영했습니다.

Copy link
Collaborator

@jhpark816 jhpark816 left a comment

Choose a reason for hiding this comment

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

리뷰 완료

}
if (!doInsert) { // UNIQUE

if (comp != 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

현재는 단순 리팩토링이므로, 여기 부분을 그대로 유지하면 좋겠습니다.

if (comp == 0) {
  // Duplicated bkey. ....
  ...
}

여기 코드를 이해하기 쉬운 코드로 리팩토링한다면, 충분히 다른 코드가 될 수도 있습니다.
이러한 리팩토링 시에 다시 조정하면 좋겠습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

indentation depth만 줄이려는 데 너무 초점이 맞춰져 있는 것 같기도 합니다.
중간에 continue하는 것보다 기존 코드가 읽기에도 나은 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반영했습니다.

@jhpark816 jhpark816 merged commit 97ed4b0 into naver:develop Dec 27, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants