-
Notifications
You must be signed in to change notification settings - Fork 3
[feat] GameSetting 변경 기능 추가 #107
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
Conversation
| public void handlePlayerReady(Long roomId, String sessionId) { | ||
| Player player = | ||
| roomRepository | ||
| .findPlayerInRoomBySessionId(roomId, sessionId) |
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.
이렇게 player를 가져오는 코드가 있는줄은 몰랐네요!
단순하게 room.PlayerSessionMap.get(sessionId) 를 쓰지 않고 이렇게 가져오는 이유가 있을까요?
NPE 때문일까요?
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.
저건 작성한지 좀 된것같은데, 세션아이디 재연결 로직이 없어서 저렇게 짠것 같습니다. 재연결 로직에서 맵에서 바로 세션을 바로 삭제하지 않으니까 Room안에서 플레이어는 null일 가능성이 없을것 같네요!
소켓의 세션아이디가 널이 아니어도 룸에 플레이어로 등록되지 않은 경우도 존재할 것 같네요! null 체크하나 추가해서 수정했습니다!
수정했습니다!
| if (room.isPlaying()) { | ||
| throw new CustomException(RoomErrorCode.GAME_ALREADY_PLAYING); | ||
| } | ||
| if (!Objects.equals(player.getId(), room.getHost().getId())) { |
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.
오홍 이것도 NPE 를 방지하고자 Objects.equals를 쓰신건가요?
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.
Host에 대해서는 널 체크를 하지 않아도 될것 같습니다! isHost로 변경했습니다!
| int questionSize = quiz.getQuestions().size(); | ||
| room.getGameSetting().changeQuiz(quiz); | ||
| // 퀴즈의 문제 갯수로 변경 | ||
| room.getGameSetting().changeRound(questionSize, questionSize); |
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.
room.getGameSetting().changeQuiz()
room.getGameSetting().changeRound() 를
Room 객체 안에 하나의 메소드로 만들면 깔끔할 것 같다는 의견입니다!
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.
gameSetting없이 Room으로 호출가능하도록 변경했습니다!
| public void afterChange(Room room, MessageSender messageSender) { | ||
| room.resetAllPlayerReadyStates(); | ||
|
|
||
| String destination = "/sub/room/" + room.getId(); |
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.
WebsocketUtils 에서 getDestination(roomId)로 대체할 수 있을 것 같습니다!
| return Arrays.stream(values()) | ||
| .filter(t -> t.value == value) | ||
| .findFirst() | ||
| .orElseThrow(() -> new CustomException(GameErrorCode.GAME_SETTING_CONFLICT)); |
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.
에러 내용을 조금 세분화해서 유효하지 않은 타이머 설정입니다.
같은 타이머 에러라는걸 명시하면 어떨까요?.?
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.
게임 내에서 일반 사용자가 유효하지 않은 타이머를 보낼일은 없다고 생각해서, 비정상적인 요청에 대해서 만들어놓은 방어적 예외라 예외 내용을 구체화하지는 않았습니다!
sehee123
left a comment
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.
수고하셨습니다! 👍
|
|
||
| @Override | ||
| public boolean change(Room room, QuizService quizService) { | ||
| if (room.getGameSetting().getTimeLimit() == timeLimit) { |
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.
Room 내부 메서드로 위치시키면 좋을 것 같습니다 !
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.
room과 gameSetting의 관계를 생각을 안하고 만들었네요 수정했습니다!
인사이트 감사합니다!
| if (room.getGameSetting().getTimeLimit() == timeLimit) { | ||
| return false; // 동일하면 무시 | ||
| } | ||
| room.getGameSetting().changeTimeLimit(TimeLimit.from(timeLimit)); |
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.
위의 리뷰 내용과 동일합니다 !
|
|
||
| public void resetAllPlayerReadyStates() { | ||
| for (Player player : playerSessionMap.values()) { | ||
| if (Objects.equals(player.getId(), getHost().getId())) continue; |
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.
isHost() 메서드가 아닌 Objects.equal()를 통해 직접 비교한 이유가 궁금합니다 ! 만약, 이유가 null-safe라면 isHost()를 null-safe하게 바꿔 사용하는 것이 가독성이나 일관성 측면에서 좋을 것 같습니다 !
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.
Host에 대해서는 널 체크를 하지 않아도 될것 같습니다! isHost로 변경했습니다!
backend/src/main/java/io/f1/backend/domain/game/dto/request/QuizChangeRequest.java
Outdated
Show resolved
Hide resolved
| Room room = findRoom(roomId); | ||
|
|
||
| return playerSessionMap.values().stream().allMatch(Player::isReady); | ||
| Player player = room.getPlayerSessionMap().get(sessionId); |
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.
이 부분도 Room의 메서드로 분리할 수 있을 것 같습니다 !
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.
Room클래스 안에, getPlayerBySessionId 만들어서 교체했습니다!
jiwon1217
left a comment
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.
고생하셨습니다 ! OCP를 고려하여 설계한 부분이 인상깊네요
silver-eunjoo
left a comment
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.
확인했습니다 !! 인터페이스로 구현하신 방식이 굉장하십니다 ! 배워갑니다..! 고생하셨습니다 :)
* ✨ feat: 게임 설정 변경 인터페이스 및 구현체 추가 - GameSettingChanger 인터페이스 도입 * chore: Java 스타일 수정 * ✨ feat: 게임 설정 변경 기능 추가 * chore: Java 스타일 수정 * 🚚 move: 소켓 요청 DTO request 디렉토리로 이동 * 🚚 move: 소켓 요청 DTO 경로 변경 * chore: Java 스타일 수정 * ♻️ refactor: 게임 세팅 요청 후처리 분리 * chore: Java 스타일 수정 * ♻️ refactor: 코드 리뷰 반영 * chore: Java 스타일 수정 * 🗑️ remove: 불필요 코드 삭제 * chore: Java 스타일 수정 * ♻️ refactor: isHost 원상복구 * ♻️ refactor: QuizChangeRequest, RoundChangeRequest 타입 수정 * 🔧 chore: RoomUpdatedEventListener, RoomDeletedEventListener 빈 등록 * chore: Java 스타일 수정 * chore: Java 스타일 수정 * ♻️ refactor: player 조회 메서드 추가 --------- Co-authored-by: github-actions <>
* ✨ feat : 게임 종료 로직 구현 * chore: Java 스타일 수정 * ✨ feat: 게임 종료 후 disconnected 참여자 정리 * 🔧 chore : 깃 충돌 해결 * chore: Java 스타일 수정 * ♻️ refactor : 게임 시작 시, 실시간 랭킹도 함께 SEND * ♻️ refactor : 서비스 간의 순환참조 해결 * chore: Java 스타일 수정 * ♻️ refactor : 게임 종료 시 로직 변경 * chore: Java 스타일 수정 * 🔧 chore: 게임 종료 시 모든 메세지 브로드캐스팅하도록 변경 * chore: Java 스타일 수정 * 🐛 fix: index 예외, Lazy 예외 버그 수정 * chore: Java 스타일 수정 * ✨ [feat] 유저 닉네임으로 정보 조회 (#101) * ✨ feat: 특정 닉네임이 포함된 유저 정보 조회 기능 * 🐛 fix: 닉네임 중복 확인 시 대소문자 구분하지 않음 * chore: Java 스타일 수정 --------- Co-authored-by: github-actions <> * ♻️ refactor: 세션 타임아웃 값을 환경변수로 대치 (#105) * 🔧 chore : 깃 충돌 해결 * 🔧 chore : 깃 충돌 해결 * ♻️ refactor: 랭킹 조회 Redis 적용 (#111) * ✨ feat: GameSetting 변경 기능 추가 (#107) * ✨ feat: 게임 설정 변경 인터페이스 및 구현체 추가 - GameSettingChanger 인터페이스 도입 * chore: Java 스타일 수정 * ✨ feat: 게임 설정 변경 기능 추가 * chore: Java 스타일 수정 * 🚚 move: 소켓 요청 DTO request 디렉토리로 이동 * 🚚 move: 소켓 요청 DTO 경로 변경 * chore: Java 스타일 수정 * ♻️ refactor: 게임 세팅 요청 후처리 분리 * chore: Java 스타일 수정 * ♻️ refactor: 코드 리뷰 반영 * chore: Java 스타일 수정 * 🗑️ remove: 불필요 코드 삭제 * chore: Java 스타일 수정 * ♻️ refactor: isHost 원상복구 * ♻️ refactor: QuizChangeRequest, RoundChangeRequest 타입 수정 * 🔧 chore: RoomUpdatedEventListener, RoomDeletedEventListener 빈 등록 * chore: Java 스타일 수정 * chore: Java 스타일 수정 * ♻️ refactor: player 조회 메서드 추가 --------- Co-authored-by: github-actions <> * 🐛 fix: 라운드 변경 시 question개수 조회에 fetch join 적용 (#116) * 🐛 fix: round 변경 시 question 조회에 fetch join 적용 * ♻️ refactor: questionCount만 찾는 쿼리로 변경 및 불필요한 퀴즈 아이디 조회 삭제 * chore: Java 스타일 수정 --------- Co-authored-by: github-actions <> * chore: Java 스타일 수정 * chore: Java 스타일 수정 * 🔧 chore : 테스트 코드 통과 * chore: Java 스타일 수정 --------- Co-authored-by: github-actions <> Co-authored-by: Jiwon Kwak <[email protected]> Co-authored-by: dlsrks1021 <[email protected]> Co-authored-by: kanghyun <[email protected]>
🛰️ Issue Number
Closes #100
🪐 작업 내용
게임 설정 변경 기능 구현
GAME_SETTING을 브로드캐스팅하고,PLAYER_LIST를 추가로 브로드캐스팅합니다.GameSettingChanger 인터페이스 도입
(퀴즈 요청은 다른 요청과 다르게, 플레이어의 준비상태를 전부 false로 만들고,
PLAYER_LIST를 브로드캐스팅해야합니다.)메서드 이동
빈 등록
RoomUpdatedEventListener, RoomDeletedEventListener 빈 등록 했습니다.
📚 Reference
✅ Check List