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] 타임라인 선수 교체 정보 관련 기능 추가 #116

Merged
merged 17 commits into from
Feb 27, 2024

Conversation

ldk980130
Copy link
Contributor

🌍 이슈 번호

📝 구현 내용

  • 타임라인 조회 기능 구현
    • 점수 타임라인 구현
    • 교체 타임라인 구현

🍀 확인해야 할 부분

  • api 스펙 변경 (https://www.notion.so/heethehope/c4afac409bf54162bfa9038017272d32)
    • 점수와 교체 관련 타임라인을 한 번에 처리하기 위한 구조로 api 응답 스펙틀 조금 수정해봤습니다. 검토 부탁드릴게요!
  • 타임라인 관련 조회 로직이 복잡해져서 일단 application 패키지 내에 timeline 패키지를 만들어 로직을 따로 모아두었습니다.
    • 이는 임시 방편일 뿐 조회 코드가 많아짐에 따라 레이어드 아키텍처로는 감당이 안되는 시점이 오는 것 같습니다.
    • query 패키지 내의 패키지 구조를 한 번 리팩터링할 필요가 있을 것 같습니다.

Copy link
Contributor

@Jin409 Jin409 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다! 리팩토링 엄청나게 하셨네요 .. 멋집니다
제가 질문한 api 스펙 부분에 대해서만 한번 짚고 넘어가보면 좋을 것 같아요!
Query 패키지 리팩토링의 필요성에 대해서도 동의합니닷. 추후에 같이 논의해봐요

Comment on lines +18 to +20
@Query("select gt from GameTeam gt join fetch gt.leagueTeam where gt.game.id = :gameId")
List<GameTeam> findAllByGameWithTeam(@Param("gameId") final Long gameId);

Copy link
Contributor

Choose a reason for hiding this comment

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

단순한 궁금증인데, findAllByGameWithTeamId 로 네이밍하지 않고 findAllByGameWithTeam 로 오버로딩하신 이유가 궁금해요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    @Query("select gt from GameTeam gt join fetch gt.leagueTeam where gt.game = :game")
    List<GameTeam> findAllByGameWithTeam(@Param("game") final Game game);

위 메서드는 GameTeam을 Game으로 조회하면서 LeagueTeam을 같이 가져오는 메서드로 원래 있던 메서드에요. 이번 타임라인 기능을 구현하면서 GameTeam을 gameId로 조회하면서 LeagueTeam을 같이 가져오는 메서드가 필요했어요.

아예 별도로 다른 이름을 쓰는 것도 방법이겠지만 매개변수만 다르고 같은 기능을 하는 메서드이기에 오버로딩을 하는 방식을 택했습니다. 실제로 Game 엔티티로 조회하는 것과 gameId로 조회하는 것도 유사성이 많다고 생각했고요.

@@ -4,10 +4,10 @@

public record ScoreRecordResponse(
Integer point,
Copy link
Contributor

Choose a reason for hiding this comment

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

저는 개인적으로 point 가 조금 어색하다고 느껴지는 것 같아요. 왜냐하면 이전까지 이러한 득점의 경우 score 를 썼었고 현재 score_records 테이블에서도 득점 정보에 관해서 score 라는 컬럼을 쓰고 있어서인 것 같아요!

작성해주신 코드에서의 score 의 역할은 Snapshot 에 종속돼서 과거의 점수를 서술하는 용어라면, 테이블에서의 score 은 득점하는 점수 자체를 서술하는 용어라고 다가와요. 이에 대해서 어떻게 생각하시는지 궁금해요! 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[
    {
        "gameQuarter": "후반전",
        "records": [
            {
                "type": "SCORE",
                "recordedAt": 26,
                "playerName": "진승희",
                "teamName": "팀 A",
                "teamImageUrl": "...",
                "score": {
                     "point": 1
                     "snapshot": [
                         {"teamName": "...", "score": 1, "teamImageUrl": "..."},
                         {"teamNamere": "...", "score": 1, "teamImageUrl": "..."},
                     ]
                }
                "replacement": null // SCORE 타입일 땐 null
...

위 스펙에서 각 타입에 따라 null인 객체를 하나로 묶기 위해 scorereplacement라는 이름으로 각 타입에 맞는 특수한 값들을 묶어줬어요. 이 구조에서 득점을 score라고 하자니 records.score.score가 되기에 어색하다고 느껴졌어요.

score 하위의 몇점을 득점했는지에 의미를 담아 point라고 이름을 명명했는데 더 나은 네이밍이 있을까요? 아니면 최상위 객체의 이름 (score, replacment)를 다르게 표기해야 할까요?

Copy link
Contributor

@Jin409 Jin409 Feb 26, 2024

Choose a reason for hiding this comment

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

현재 테이블명을 따라서 scoreRecords, ReplacementRecord 로 하는건 어떨까요?
사실상 타입이 score 이고 replacement 인거고, 그 각자는 각각 scoreRecord, replacementRecord 이기는 하니까요!
point 는 snapshot 내부에서는 같은 맥락임에도 score 로 표기되고 있기도 하고, 저희가 사전에 정의되었거나 논의된 용어가 아니라서 나중에도, api 스펙 상에서도 혼돈을 줄 것 같다는 생각이 들어요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Jin409
리뷰 반영햇습니다~

Copy link
Contributor

Choose a reason for hiding this comment

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

수고하셨습니당~ 머지해도 좋을 것 같아요!

@ldk980130 ldk980130 merged commit 1a15a82 into main Feb 27, 2024
1 check passed
@ldk980130 ldk980130 deleted the feat/#107-timeline branch February 27, 2024 14:30
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.

[FEAT] 타임라인 선수 교체 정보 관련 기능 추가
2 participants