-
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] 방 게임 진행 시간 변경 #332
[FEAT] 방 게임 진행 시간 변경 #332
Conversation
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.
gooooooooooooood boy
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.
직접 나서주셔서 감사합니다 커찬!
시간 부분만 수정 부탁드리겠습니다.!
import org.junit.jupiter.api.Nested; | ||
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.ValueSource; | ||
|
||
class RoomSettingTest { | ||
|
||
private static final List<Integer> ALLOWED_TIME_LIMIT = List.of(5_000, 10_000, 15_000, 30_000); |
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.
10 15 30 60 아닌가요~?!
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.
빠르게 반영하겠습니다
- 매번 허용되는 시간이 추가될 때마다 에러 메시지가 변경되어야 하므로 에러 메시지 형식을 변경함
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.
커찬 수고했어요~ 커멘트 남겼는데 납득이 되시면 반응 부탁드릴게요! 사소한 의견밖에 없어서 Approve 드립니다~
@@ -24,7 +24,7 @@ public enum ClientErrorCode { | |||
|
|||
// RoomSetting | |||
// todo s로 변경 | |||
INVALID_TIME_LIMIT("시간 제한은 %dms / %dms / %dms 만 가능합니다. requested timeLimit: %d"), | |||
INVALID_TIME_LIMIT("시간 제한은 %s 만 가능합니다. requested timeLimit: %d"), |
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.
질문) s로 변경한 이유가 궁금합니다!
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.
이거 매번 시간 제한 선택지가 늘어날 때마다 메시지가 바뀌는 게 마음에 안들었습니다.
그래서 List를 toString으로 바꿔서 에러 메시지로 넣도록 했습니다~
@ParameterizedTest | ||
@ValueSource(ints = {2, 11}) | ||
void 라운드는_3이상_10이하_여야한다(int notValidTotalRound) { | ||
void 라운드는_3미만_10초과인_경우_예외를_던진다(int notValidTotalRound) { | ||
// when & then | ||
assertThatThrownBy(() -> new RoomSetting(notValidTotalRound, 5000, Category.IF)) | ||
assertThatThrownBy(() -> new RoomSetting(notValidTotalRound, 15_000, Category.IF)) | ||
.isExactlyInstanceOf(InvalidRangeTotalRoundException.class) | ||
.hasMessage("총 라운드는 %d 이상, %d 이하만 가능합니다. requested totalRound: %d" | ||
.formatted(3, 10, notValidTotalRound)); | ||
} |
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
Closed #331
As-Is
To-Be
Check List
Test Screenshot
(Optional) Additional Description