-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
# Conflicts: # src/main/resources/static/docs/api.html
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.
수고하셨습니다! 리팩토링 엄청나게 하셨네요 .. 멋집니다
제가 질문한 api 스펙 부분에 대해서만 한번 짚고 넘어가보면 좋을 것 같아요!
Query 패키지 리팩토링의 필요성에 대해서도 동의합니닷. 추후에 같이 논의해봐요
@Query("select gt from GameTeam gt join fetch gt.leagueTeam where gt.game.id = :gameId") | ||
List<GameTeam> findAllByGameWithTeam(@Param("gameId") final Long gameId); | ||
|
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.
단순한 궁금증인데, findAllByGameWithTeamId 로 네이밍하지 않고 findAllByGameWithTeam 로 오버로딩하신 이유가 궁금해요
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.
@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, |
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.
저는 개인적으로 point 가 조금 어색하다고 느껴지는 것 같아요. 왜냐하면 이전까지 이러한 득점의 경우 score 를 썼었고 현재 score_records 테이블에서도 득점 정보에 관해서 score 라는 컬럼을 쓰고 있어서인 것 같아요!
작성해주신 코드에서의 score 의 역할은 Snapshot 에 종속돼서 과거의 점수를 서술하는 용어라면, 테이블에서의 score 은 득점하는 점수 자체를 서술하는 용어라고 다가와요. 이에 대해서 어떻게 생각하시는지 궁금해요! 👀
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.
[
{
"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
인 객체를 하나로 묶기 위해 score
와 replacement
라는 이름으로 각 타입에 맞는 특수한 값들을 묶어줬어요. 이 구조에서 득점을 score
라고 하자니 records.score.score
가 되기에 어색하다고 느껴졌어요.
score
하위의 몇점을 득점했는지에 의미를 담아 point
라고 이름을 명명했는데 더 나은 네이밍이 있을까요? 아니면 최상위 객체의 이름 (score
, replacment
)를 다르게 표기해야 할까요?
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.
현재 테이블명을 따라서 scoreRecords, ReplacementRecord 로 하는건 어떨까요?
사실상 타입이 score 이고 replacement 인거고, 그 각자는 각각 scoreRecord, replacementRecord 이기는 하니까요!
point 는 snapshot 내부에서는 같은 맥락임에도 score 로 표기되고 있기도 하고, 저희가 사전에 정의되었거나 논의된 용어가 아니라서 나중에도, api 스펙 상에서도 혼돈을 줄 것 같다는 생각이 들어요!
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.
@Jin409
리뷰 반영햇습니다~
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.
수고하셨습니당~ 머지해도 좋을 것 같아요!
🌍 이슈 번호
📝 구현 내용
🍀 확인해야 할 부분