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

feature: PR #11 코드 리뷰 반영 #29

Merged
merged 9 commits into from
Nov 6, 2024
Merged

feature: PR #11 코드 리뷰 반영 #29

merged 9 commits into from
Nov 6, 2024

Conversation

Lee-Dahyeon
Copy link
Collaborator

⚠️ 연관된 이슈 번호 #28

📋 작업 내용

  • 코드 리뷰 반영하여 수정하였습니다.
    • 어노테이션 추가
    • 메소드명 수정
    • 예외 처리(orElseThrow)

📷 스크린샷(선택)

💬 리뷰 요구사항

리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요.
e.g. 메서드 XXX의 이름을 더 잘 짓고 싶은데 혹시 좋은 명칭이 있을까요?

- findByVisitorMatch->findParticipantIdByWorkspaceId
- existsById->isClickedById
- Optional에 대한 orElseThrow처리
Copy link
Contributor

@LJH098 LJH098 left a comment

Choose a reason for hiding this comment

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

제가 놓쳤었는데
public interface ParticipantRepository extends JpaRepository<User, Long> {

이 부분 User가 아니라 Participant가 되야 할 것 같습니다.

Comment on lines 17 to 18
@Query(value = "SELECT COUNT(p) >0 FROM Participant p WHERE p.id=:id AND p.participantMatch=true")
Boolean isClickedById(long id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Boolean existsByIdAndAndParticipantMatchTrue(long id);
위처럼 JPQL을 사용하지 않고 Data JPA를 사용 할 수도 있을 것 같아요

그리고 isClicekd는 너무 추상적인 단어 인 것 같아요.

GetWorkspacesResponse.of(
workspace.getId(), workspace.getCreator().getUid()))
.filter(workspace -> workspace.getCreatedAt().isBefore(dateFilter)) //게임방 조회: 유지 시간은 24h
.map(workspace ->GetWorkspacesResponse.of(workspace.getId(), workspace.getCreator().getUid()))
.toList();
Copy link
Contributor

Choose a reason for hiding this comment

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

#27 PR이 아직 머지가 되지 않았는데 해당 PR의 commit이 그대로 있는 것 보아하니
feature-#28 브랜치를 develop에서 브랜치를 생성하는 것이 아니라 feature-#26에서 생성한 것 같은데 맞을까요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

현재는 머지가 된 상태입니다!

@LJH098 LJH098 force-pushed the develop branch 3 times, most recently from e657b04 to c2e417d Compare November 4, 2024 13:19
@Lee-Dahyeon Lee-Dahyeon merged commit fac7abd into develop Nov 6, 2024
1 of 2 checks 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