-
Notifications
You must be signed in to change notification settings - Fork 6
Add LadderGame #2
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
base: 김지우
Are you sure you want to change the base?
The head ref may contain hidden characters: "\uAE40\uC9C0\uC6B0"
Conversation
- add ConsoleInput.java - add LadderInputData.java - add User.java - add LadderHeight.java
- add a strategy for random row - add a strategy for manual row having even index lines
- add Ladder.java - add Row.java - add Line.java - add LadderGenerator.java
- show users - show ladder
- add Prize.java - add field 'prizes' in LadderInputData.java - add a method that validate whether num of users is equal to num of prizes
- add field 'ladderGameResult' in LadderGame.java
Deocksoo
left a comment
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.
리뷰가 많이 늦어 죄송합니다ㅠㅠ
코드 깔끔하게 잘 작성해주신 것 같아요 👍
아직 중간까지밖에 리뷰를 작성하지 못했는데, 어제 리펙토링 하시면서 나누신 이야기도 있을것 같아서
일단은 지금 작성한 데까지만 남겨두었습니다. 확인 부탁드립니다.
보시고 혹시 모호한 부분이 있으면 또 여쭤봐주셔요 :)
| return Arrays.asList(sc.nextLine().split(DELIMITER)); | ||
| } | ||
|
|
||
| static List<Prize> createPrizesFrom(List<String> prizeNames) { |
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.
메서드 순서도 convention! 위에서 아래로 읽을 수 있도록 배치해주시면 어떨까요?
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.
그리고 createUsersFrom 메서드와 마찬가지인데, 해당 메서드는 default 접근제어자를 가지고 있지만 현재 클래스 안에서만 사용하고 있네요. 접근제어자를 default로 하신 것은 테스트에서 사용하기 위함인가요?
| throw new IllegalArgumentException(ERROR_INPUT_NOT_INTEGER); | ||
| } | ||
|
|
||
| public static boolean isConvertibleIntoInteger(String str) { |
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.
이 부분도 접근제어자를 조금 더 고민해보아야 할거같아요.
| return new Ladder(rows); | ||
| } | ||
|
|
||
| public int startWith(int initialPosition) { |
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.
해당 메서드는 특정 위치의 플레이어가 사다리를 끝까지 내려갔을 때 어떤 위치에 있는지 반환하는 메서드인 것 같아요. 메서드 이름만 봤을 때는 메서드가 어떤 역할을 하고 있는지 잘 와닿지 않는것 같아요. 좀 더 적절한 이름은 없을까요?
| int position = ladder.startWith(user.getPosition()); | ||
| Prize prize = prizes.stream() | ||
| .filter(p -> p.hasPosition(position)) | ||
| .findAny().orElseThrow(IllegalArgumentException::new); |
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.
여기에서는 IllegalArgumentException보다는 직접 정의한 exception을 만들어 의미를 담아주면 어떨까요?
|
|
||
| public static boolean isConvertibleIntoInteger(String str) { | ||
| for (char c: str.toCharArray()) { | ||
| if (c < 48 || c > 57) { |
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.
48과 57은 무슨 숫자인가요?
|
|
||
| public static Scanner sc = new Scanner(System.in); | ||
|
|
||
| public static LadderGameInputData getLadderGameInputData() { |
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.
ConsoleInput에서 너무 많은 일을 하고있는 것 같아요.
Input에서는 순수하게 외부에서부터 문자열 input만 받도록 하고,객체 생성은 다른 곳에서 담당하는 것은 어떨까요?
Description of change