-
Notifications
You must be signed in to change notification settings - Fork 3
[feat] 방 입/퇴장 동시성 구현 및 로직 변경 #47
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
| } | ||
|
|
||
| /* 방장 변경 */ | ||
| if (room.getHost().getId().equals(removePlayer.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.
[L5-참고의견]
Room 객체 내부에서 해당 조건을 확인하는 메서드를 구현하면 비즈니스 로직의 getter 중첩을 없앨 수 있을 것 같습니다 !
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.
덕분에 로직이 깔끔해지고,책임이 명확해졌습니다. 감사합니다. 😇
|
|
||
| PlayerListResponse playerListResponse = toPlayerListResponse(room); | ||
| /* 방 삭제 */ | ||
| if (playerSessionMap.size() == 1 && playerSessionMap.containsKey(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.
[L4-변경제안]
주석을 다신 단위로 메서드를 분리하면 exitRoom 메서드에 포함된 메서드 이름과 파라미터만 보고 전체 동작을 파악하기 쉬울 것 같습니다 !
| SystemNoticeResponse systemNoticeResponse = | ||
| ofPlayerEvent(removedPlayer, RoomEventType.EXIT); | ||
| if (removePlayer == null) { | ||
| room.getUserIdSessionMap().remove(getCurrentUserId()); |
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.
[L5-참고의견]
이 부분도 Room 객체 내부에서 구현할 수 있을 것 같습니다 !
backend/src/test/java/io/f1/backend/domain/game/app/RoomServiceTests.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/io/f1/backend/domain/game/app/RoomServiceTests.java
Outdated
Show resolved
Hide resolved
backend/src/test/java/io/f1/backend/domain/game/app/RoomServiceTests.java
Show resolved
Hide resolved
backend/src/test/java/io/f1/backend/domain/game/app/RoomServiceTests.java
Show resolved
Hide resolved
# Conflicts: # backend/src/main/java/io/f1/backend/domain/game/app/RoomService.java # backend/src/test/java/io/f1/backend/domain/game/app/RoomServiceTests.java
LimKangHyun
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.
RoomService가 슬슬 무거워지는 것 같습니다. 클래스를 어느정도 분리할 방법을 찾으면 좋을것 같습니다!
고생하셨습니다!
| @PostMapping("/validation") | ||
| @PostMapping("/enterRoom") | ||
| @ResponseStatus(HttpStatus.NO_CONTENT) | ||
| public void validateRoom(@RequestBody @Valid RoomValidationRequest request) { |
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.
[L4-변경제안]
입장과 validation 검사를 한 번에 해주는 건데, 뭔가 컨트롤러는 메서드 네이밍이 validateRoom으로 남아있고, 다른 엔드포인트나 서비스 네이밍은 enterRoom이라 컨트롤러 네이밍도 enterRoom이면 좋겠다는 의견입니다 !
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.
헉. 메소드 네임은 안바꾸고 경로만 바꿨네요 .. 메서드 네임 변경해서 puh했습니다. 감사합니다!
# Conflicts: # backend/src/test/java/io/f1/backend/domain/game/app/RoomServiceTests.java
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.
테스트까지!! 꼼꼼하게 작업해주시고, 많이 고민해주셔서 감사합니다 :) 고생하셨습니다 !
🛰️ Issue Number
🪐 작업 내용
synchronized적용입장 동시성 적용 전 테스트 실패
입장 동시성 적용 후 테스트 성공
퇴장 동시성 적용 전 테스트 실패
퇴장 동시성 적용 후 테스트 성공
논의사항
📚 Reference
✅ Check List
소감
구현보다 테스트가 너무나 힘들었네요 ^_ㅠ