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

[FIX] 둘러보기-유저 모든 작품 조회로 수정 #181

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

kjy-asl
Copy link
Contributor

@kjy-asl kjy-asl commented Jun 1, 2024

Related Issue 🚀

Work Description ✏️

  • 둘러보기 - 유저 에서 대표작만 표시에서 모든 작업물 표시로 수정.

PR Point 📸

쿼리 튜닝이 좀 필요해 보이는데 나중에 손 볼 시간이 있었으면 좋겠습니당 도와주세요 선생님

Copy link
Member

@dragontaek-lee dragontaek-lee left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~! 로직 좋은 것 같습니다!
개인적으로 stream을 잘 쓰는 것을 좋아해서 코멘트 남겨두었어요.
그냥 작성한 코드라 틀릴 수 도 있어서 참고만 부탁드립니다!

추가적으로 페이지네이션에 대해서 고민해볼 필요가 있을 것 같습니다! 한번 회의해보면 좋을거 같아요

@@ -5,6 +5,8 @@
import com.gam.api.domain.work.entity.Work;
import lombok.Builder;

import java.time.LocalDateTime;
Copy link
Member

Choose a reason for hiding this comment

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

혹시 이거 쓰이는 부분이 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉 이 부분은 실수로 넣었네욥

}
Map<User, Boolean> scrapMap = new HashMap<User,Boolean>(); // user 별 스크랩 한 경우와 아닌 경우 식별을 위한 map
for (UserScrapUserQueryDto user:users) {
scrapMap.put(user.user(), user.scrapStatus());
Copy link
Member

@dragontaek-lee dragontaek-lee Jun 1, 2024

Choose a reason for hiding this comment

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

user객체를 비교하기보다는 userID만으로도 key를 세팅하면 좋을거 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

객체를 비교하게 되면 성능이 안 좋아 지나용??

Copy link
Member

Choose a reason for hiding this comment

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

넵 우선 Map 자체의 메모리 크기가 커지는 단점이 있고,
제가 알기로는 객체를 키값으로 하면 키값 해싱할 때 더 많은 작업이 소요되는 것으로 알고 있어요! 이부분은 조금 더 알아보도록 하겠습니당

.findFirst().get();
}
Map<User, Boolean> scrapMap = new HashMap<User,Boolean>(); // user 별 스크랩 한 경우와 아닌 경우 식별을 위한 map
for (UserScrapUserQueryDto user:users) {
Copy link
Member

@dragontaek-lee dragontaek-lee Jun 1, 2024

Choose a reason for hiding this comment

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

forEach / stream을 쓰면 좋을거 같아요! 그리고 두번 순회하기 보다는, 한번 순회하면서 두개하는게 좋아보여요.
scrapMap.put(user.user(), user.scrapStatus());
workAll.addAll(dto.user().getWorks());

혹은 아래와 같이 모든작업을 각각 한번의 파이프에 다 처리해도 될거같아요


workAll = workAll.stream() // 최근 수정된 날짜 기준 정리
.sorted(Comparator.comparing(Work::getModifiedAt).reversed())
.collect(Collectors.toList());

Copy link
Member

Choose a reason for hiding this comment

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

flatMap으로 다 가져오고, stream을 적극적으로 활용하면 가독성이 좋아질거 같아요.

Suggested change
Map<User, Boolean> scrapMap = users.stream()
.collect(Collectors.toMap(UserScrapUserQueryDto::user, UserScrapUserQueryDto::scrapStatus));
// 모든 work를 가져와 최근 수정된 날짜 기준으로 정리
List<Work> workAll = users.stream()
.flatMap(dto -> dto.user().getWorks().stream())
.sorted(Comparator.comparing(Work::getModifiedAt).reversed())
.collect(Collectors.toList());

Copy link
Contributor

@GaHee99 GaHee99 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

}
return UserDiscoveryResponseDTO.of(dto.user(), userScrap, firstWork);
return UserDiscoveryResponseDTO.of(work.getUser(), userScrap, work);
Copy link
Contributor

Choose a reason for hiding this comment

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

userScrap 변수에는 boolean값이 들어있나욥?
그렇다면, 그냥 true값으로 넣는게 어떤지..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

앗 이유가 뭘까요?? userScrap 변수에 정보가 들어있으니 변수를 사용하는게 낫지 않을까용
그럼 312번 줄에도 false 넣고 314번 줄에 true를 넣어야 일관성 있지 않나용?

@GaHee99 GaHee99 merged commit 303e7e4 into develop Jun 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants