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

Feat/taggame #43

Merged
merged 16 commits into from
Sep 7, 2024
Merged

Feat/taggame #43

merged 16 commits into from
Sep 7, 2024

Conversation

david-parkk
Copy link
Member

✏️ 작업 개요

  • 술래잡기 매칭 기능 추가

⛳ 작업 분류

🔨 작업 상세 내용

  1. 매칭 기능을 추가했습니다.(수박게임가 동일한 로직)

💡 생각해볼 문제

  • global request 관련 이슈에 정리하였습니다.
  • 검증된 방법이 아니기 때문에 분명이 단점이 있습니다. 솔직하게 의견을 알려주세요(버릴 의지 충분히 있음)

Copy link

github-actions bot commented Sep 6, 2024

Test Results

18 tests   18 ✅  7s ⏱️
 7 suites   0 💤
 7 files     0 ❌

Results for commit 62753a6.

♻️ This comment has been updated with latest results.

Copy link
Member

@kamothi kamothi left a comment

Choose a reason for hiding this comment

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

잘 봤습니다. 공통으로 처리하여 유연하게 가져갈려고 고민하시는게 느껴지네요. 제네릭으로 묶어서 처리하는게 좋아는 보입니다. 이건 저도 한 번 제 미니게임에도 적용해보면서 직접 느껴봐야할거 같습니다.


@Getter
@NoArgsConstructor
public class TagGameUser {
Copy link
Member

Choose a reason for hiding this comment

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

데이터 형식은 map과 동일한가요?

Copy link
Member 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.

👌

@AllArgsConstructor
@NoArgsConstructor
@ToString
public class TagGameRequest<T> {
Copy link
Member

Choose a reason for hiding this comment

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

public class TagGameRequest<T> {

    private TagGameStatus tagGameStatus;

    private T request;
}

지금은 TagGame으로 status와 클래스가 지정되어 있는데 이것 또한 유연하게 공통 제네릭을 만들자는 의미죠?

Copy link
Member Author

Choose a reason for hiding this comment

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

질문을 제대로 이해했는지 모르겠는데 TagGameStatus 를 GameRequestType으로 변경시켜 로직에 대한 enum 값을 추가할 생각이였습니다, 예를 들어

  1. MAP_SAVE (맵 이동)
  2. TAG_GAME_MATCHING (술래잡기 매칭)
  3. MINI_GAME_RUNNIG(수박게임 실행)

Copy link
Member

Choose a reason for hiding this comment

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

제대로 이해하신거 같습니다. 현재 코드에서는 제네릭 이름이나 status가 미니게임으로 특정되어 있어서 물어본거였습니다!


}
else{
if(player1.getValue().isOpen()){
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
Member Author

Choose a reason for hiding this comment

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

원래 자바에서 queue를 LinkedList 구현체로 사용하는 것으로 알고 있는데, Deque를 구현하셨더라고요.
LinkedList의 경우 인덱스로 접근할 수 있어서 코드 상으로는 더 좋을 수 있는데 queue는 head가 delete되어도 나머지 요소들이 앞으로 이동하는 오버헤드가 발생안해서 저렇게 구현하는게 일단 맞다고 생각합니다.
말씀하신 것과 같이 리펙토링이 필요한 부분이라고 저도 생각합니다

Copy link
Member

Choose a reason for hiding this comment

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

좋습니다. 그냥 코드 자체를 조금 더 쉽게 작성할 수 있을거 같은데 안떠올라서 꺼낸 이야기였습니다.✌

@kamothi
Copy link
Member

kamothi commented Sep 7, 2024

테스트 작성 후 머지해주시면 될거 같습니다.

@david-parkk david-parkk merged commit b711ca1 into main Sep 7, 2024
3 checks passed
@david-parkk
Copy link
Member Author

/쿠타버스 배포

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.

2 participants