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.

Sign up for GitHub

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

Jump to bottom

[FEAT] 게임 진행 타임라인 내부 로직 구현 #237

Merged
merged 15 commits into from
Sep 28, 2024
Merged

[FEAT] 게임 진행 타임라인 내부 로직 구현 #237

merged 15 commits into from
Sep 28, 2024

Conversation

ldk980130
Copy link
Contributor

🌍 이슈 번호

📝 구현 내용

  • 게임 진행 타임라인 생성 시 경기의 쿼터 및 상태 변경 로직 구현
  • 게임 진행 타임라인 삭제 시 롤백 로직 구현

🍀 확인해야 할 부분

  • GameProgressTimelineTest 테스트를 보면서 빠진 엣지 케이스가 없는지 확인해주세요
  • 롤백 로직을 위해 GameProgressTimeline에 아래 두 필드가 추가되었습니다.
    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "previous_quarter_id")
    private Quarter previousQuarter;

    private LocalDateTime previousQuarterChangedAt;

@ldk980130 ldk980130 self-assigned this Sep 22, 2024
Jin409
Jin409 reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@Jin409 Jin409 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

수고하셨어요~ ㅠ.ㅠ 굉장히 복잡하네요 당 채우고 답장해주세요 ㅋㅋ

src/main/java/com/sports/server/command/sport/domain/Quarter.java Outdated
@@ -18,4 +18,7 @@ public class Quarter extends BaseEntity<Quarter> {
@ManyToOne(fetch = FetchType.LAZY)
@JoinColumn(name = "sports_id")
private Sport sport;

@Column(name = "_order")
Copy link
Contributor

Choose a reason for hiding this comment

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

_ 는 예약어라서 붙여둔 건가요?

src/main/java/com/sports/server/command/sport/domain/Sport.java
public Quarter getEndQuarter() {
return quarters.stream()
.max(Comparator.comparing(Quarter::getOrder))
.orElseThrow(() -> new IllegalArgumentException("최종 쿼터가 존재하지 않습니다."));
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 예외가 던져지는 상황은 Quarter 가 하나도 없을 때 아닌가요?!
그냥 쿼터가 존재하지 않는다고 해도 되지 않을런지... getAfterStartQuarter() 도 마찬가지로!!

src/test/java/com/sports/server/command/timeline/domain/GameProgressTimelineTest.java
// when then
assertThatThrownBy(() -> new GameProgressTimeline(
game,
전반전,
Copy link
Contributor

Choose a reason for hiding this comment

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

요거 케이스가 너무 다양하지 않을까요?
후반전 -> 경기전으로 가는 것으로 쿼터 순서를 오름차순으로 생성할 수 있는지 검증하는 것은 조금 불안쓰하네요 😵‍💫

src/test/java/com/sports/server/command/timeline/domain/GameProgressTimelineTest.java Outdated
@Test
void 전반전_시작_타임라인을_생성한다() {
// given
경기_시작_타임라인을_생성한다();
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 테스트가 다른 메서드를 의존하도록 짠 이유가 있을까?!
내가 생각했을 때 우려되는 점은

  1. 다른 메서드를 의존하기 때문에 하나의 메서드가 실패하면 전부 실패
  • 만약 전반전 종료 타임라인 로직만 잘못 되더라도 그 이후에는 다 실패하게 됨
  1. 중복적인 메서드의 호출때문에 테스트가 느려지지 않을까

이렇게 두가지야

정확성과 테스트의 속도를 높이기 위해서 경기전 상태인 게임, 전반전 상태인 게임, 전반전 종료 상태인 게임, 후반전 상태인 게임.. 등등 각각의 픽스처를 만드는게 차라리 낫지 않을까?!

src/test/java/com/sports/server/command/timeline/domain/GameProgressTimelineTest.java
GameProgressType.GAME_START
);

timeline.apply();
Copy link
Contributor

Choose a reason for hiding this comment

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

이것도 ㅜㅜ 다른 메서드에 넘 의존적이지 않으려나요?
타임라인 로직 자체가 너무 복잡해서 하나만 어긋나도 다같이 깨져버리면 어디가 문제인지 못찾을 것 같다는 걱정이 드네유..

src/test/java/com/sports/server/command/timeline/domain/GameProgressTimelineTest.java

// then
assertAll(
() -> assertThat(game.getGameQuarter()).isEqualTo(전반전.getName()),
Copy link
Contributor

Choose a reason for hiding this comment

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

경기 시작 타임라인과 전반전 시작 타임라인의 차이가 있을까?
결과도 1. 쿼터가 전반전이다 2. 경기의 상태가 PLAYING 이다 로 동일한데 이걸 굳이 분류해야 하는 이유가 있으려나?!
경기 시작 타임라인은 생성하지 말고, 전반전 시작 = 경기 시작으로 가는 게 낫지 않을까?
똑같은 결과를 위해 사용자가 중복적인 요청을 보내야 한다면 낭비이기두 하고 UX 적으로도 귀찮지 않을까 하는 생각!
GAME_START 는 내부적으로만 사용하는 게 어떨까?

src/test/java/com/sports/server/command/timeline/domain/GameProgressTimelineTest.java
50,
GameProgressType.GAME_END
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

두 가지 추가적인 논의를 하고 싶은데!

  1. 사용자가 전반전 -> 바로 승부차기로 가버린다면?
    -> 이 케이스를 일부러 막지 않은 것인지는 모르겠지만.. 지금 로직 상으로는 이것도 가능할 것 같고 테스트에도 없는 것 같아 보여서!

  2. 전반전 시작만 등록하고 종료는 등록하지 않은 상태에서 후반전 시작을 등록한다면?
    -> 자동으로 Timeline 데이터를 추가해서 저장해줄 것인지 아니면 무시하고 갈 것인지.. 논의가 필요하려나.. 싶음..!

@Jin409 Jin409 merged commit e0b8cd2 into main Sep 28, 2024
1 check passed
@Jin409 Jin409 deleted the feat/#229-progress-timeline branch September 28, 2024 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jin409 Jin409 Jin409 left review comments

Assignees

@ldk980130 ldk980130

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEAT] 게임 상태 변경 타임라인 추가 시 게임 쿼터 자동 변환 로직 구현
3 participants
@ldk980130 @Jin409 @Zena0128