-
Notifications
You must be signed in to change notification settings - Fork 33
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
[사다리] 김의천 미션 제출합니다. #34
base: wzrabbit
Are you sure you want to change the base?
Conversation
- 게임의 정보는 담지 않은 채로 사다리 본연의 로직에만 집중하는 클래스 - 사다리의 시작점과 도착점이 각각 어떻게 연결되는지를 인덱스 형태로 반환 가능
- 불리언 값을 반환하는 기본 형태 - 이후 이 인터페이스를 implement해 랜덤 불리언 값을 생성하는 클래스에 활용
- 사다리의 연결 여부를 무작위로 결정
- 항상 올바른 사다리만을 반환하는 것이 보장됨 - 구현 실수로 올바른 사다리가 만들어지지 못할 경우 `LadderRow` 컬렉션과 `Ladder` 컬렉션 검증에서 예외 발생
- (참가자 이름, 실행 결과)를 짝지은 DTO - `List<LadderGamePlayerResult>` 형태로 전체 결과 데이터 전달에 사용
- 사다리의 형태, 참가자 명단, 실행 결과 명단을 다루는 DTO - 생성된 사다리 및 참가자/실행 결과를 출력하기 위한 데이터 전달에 사용
- 무작위의 사다리 생성 - 사다리 게임의 상태 및 결과 반환
- 사다리 생성 관련 기능에 대한 테스트로 기능 점검
- 생성된 사다리에 대한 검증 및 참가자와 실행 결과 연결 로직 테스트를 통한 기능 점검
- 게임 결과에 대한 기능 점검
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.
@wzrabbit 오랜만입니다.. 잘 지내세요?
저는 진짜 피곤해서 주글 것 같아요..... 요토 어떻게 우테코 출퇴근 하셨나요?
진짜 편도 1시간 30분인데 힘들어서 매일 비몽사몽이랍니다.
저의 경우 사다리의 세로선과 세로선을 연결하는 가로선을 중점으로, 연결되어 있는지/아닌지를 열 개수가 하나 적은 2차원 boolean 배열로 나타냈어요.
요토 코드를 보면서 역시 알고리즘 킹왕짱은 다르구나 싶었어요. 처음 java 미션을 했을 때와 비교하더라도 엄청 수준급으로 성장하셨는걸요?!?! 곧 백엔드도 마스터 하시는거 아닌가요ㅋㅋ
이전 미션에서 너무 많이 컨트롤러에 불필요한 멤버 변수를 둔 적이 있었는데, 이번에는 특정 단계에서만 사용되는 인스턴스들을 구분해 멤버 변수로 두지 않고 메서드 내에서만 사용하도록 개선해 보았습니다.
확실히 컨트롤러가 가벼워진게 보이기도하고 가독성 측면에서도 코드를 따라가기 편했던 것 같아요 완전 굿
제가 리뷰를 거의;;^^;; 1달 걸렸는데 진짜 미안합니다...
그리고 일단은? 다 본게 아니라 가볍게 훑기만 해서 아직 완벽하게 리뷰를 하지 않았어요.
좀만 더 기다려주면ㅠ 돌아오는 주 까지는 완벽하게 리뷰해둘게요.
너무 기다리시는 것 같아서 먼저 코멘트 남겨놔요..
이번주도 화이팅이에여!
while (true) { | ||
PlayerName playerNameForResult = InputView.inputPlayerNameForResult(); | ||
|
||
if (playerNameForResult.getName().equals("all")) { |
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.
all키워드는 자주 사용되는 것 같아서 상수로 추출해도 좋을 것 같아요
while (true) { | ||
try { | ||
return InputView.inputPlayerNames(); | ||
} catch (IllegalArgumentException e) { | ||
OutputView.printErrorMessage(e.getMessage()); | ||
} | ||
} | ||
} |
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.
while-try-catch 로직이 자주 등장하는데. 뭔가 메서드로 추상화해서 사용해도 좋을 것 같아요
|
||
public class LadderGameController { | ||
private LadderGame ladderGame; | ||
private final LadderGenerator ladderGenerator = new LadderGenerator(new LadderConnectionGenerator()); |
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.
이거 코드 생성자에서 주입하는 방식도 좋을 것 같아요
continue; | ||
} | ||
|
||
if (playerNameForResult.getName().equals("exit")) { |
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.
exit키워드도 상수로 추출해도 좋을 것 같아여
this.ladderRow = new ArrayList<>(ladderRow); | ||
} | ||
|
||
private void validateLadderLine(List<Boolean> ladderRow) { |
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.
가독성을 위해 자잘하게 함수를 나눈거죠? 좋네요 확인하기!
컨트롤러에서 input output에 대한 처리도 좀 하는걸로 보이는데 유효성 검사 때문에 상단에서 몰아서 유효한지 확인하는건가요?
} | ||
|
||
public List<LadderRow> getLadder() { | ||
return Collections.unmodifiableList(ladder); |
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.
오 고오급 문법!
ladderGameStatus.getLadder().forEach(((ladderRow) -> { | ||
String asciiLadder = " |" + ladderRow.getRow().stream() | ||
.map(ladderConnection -> { | ||
if (ladderConnection) { | ||
return "-----"; | ||
} | ||
return " "; | ||
}) | ||
.collect(Collectors.joining("|")) + "|"; | ||
System.out.println(asciiLadder); | ||
})); | ||
|
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.
엄청 마이너한 리뷰이긴 하지만 사다리 결과 출력하는 부분을 분리하면 메서드의 크기를 줄일 수 있을 것 같아요
OutputView.printGameStatus(ladderGame.getGameStatus()); | ||
} | ||
|
||
public void showGameResult() { |
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.
혹시 이쪽에 enum을 사용해보실 생각이 있으실까요? ENUM을 직접 활용한 부분이 없어서 아쉬워요
이런식으로? 사실 좀 억지로 넣는 것 같긴해도 이왕 배운거 사용해보면 좋지않을까 싶네요!
물론 여기말고 다른쪽에 활용할 부분이 있다면 거기서 enum을 사용해도 좋아요
public void showGameResult() { | |
public enum ResultCommand { | |
ALL("all"), | |
EXIT("exit"), | |
SINGLE(""); | |
private final String command; | |
ResultCommand(String command) { | |
this.command = command; | |
} | |
public static ResultCommand from(String input) { | |
if (input.equalsIgnoreCase("all")) { | |
return ALL; | |
} | |
if (input.equalsIgnoreCase("exit")) { | |
return EXIT; | |
} | |
return SINGLE; | |
} | |
} |
private final Ladder ladder; | ||
private final PlayerNames playerNames; | ||
private final PrizeNames prizeNames; | ||
private final List<Integer> ladderConnections; |
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.
마이너한 리뷰 추가하자면
private final PlayerNames playerNames;
private final PrizeNames prizeNames;
이거 두개 묶어서 또 클래스 분리하면 인스턴스 변수를 하나 줄일 수 있겠네요!
} | ||
} | ||
|
||
private PlayerName inputPlayerNameForResult() { |
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.
이친구는 왜 무한으로 자기를 호출하고 있죠..?
안녕하세요 리뷰어님 👋🏻 사다리 미션을 제출합니다. 잘 부탁드립니다!
이번 미션에서 신경써본 점 ✅
LadderApplication
이 맡도록 했습니다.InputMismatchException
을 이용해IllegalArgumentException
이 처리할 수 없는 범위 예외를 처리해 주었어요. 수를 입력해야 하는데 문자열을 입력했을 때 애플리케이션이 터져버리면 너무 슬프잖아요. 리뷰어님 통해서 배웠네요 👍🏻stream
과map
, 람다를 더 적극 사용하였습니다. for문의 사용을 가능한 한 줄여보는 연습을 하고 있습니다. stream을 사용하는 경우 타입이 어떨 지를 예측하는 게 힘들었는데 IntelliJ가 추론을 잘 해줘서 편하게 구현할 수 있었습니다.전체적으로 지금까지 배운 짤막한 지식들을 활용해 복습한다는 느낌으로 이번 미션을 수행했어요.
이번 미션에서는 특별히 enum을 사용하거나 여러 클래스에 활용되는 상수를 사용할 일은 없었지만, 상수를 사용하여 코드 품질을 개선할 수 있는 각이 보일 경우 도입할 계획입니다.
사다리 데이터를 관리한 방법 📦
미션 특성상 사다리 데이터를 다룰 수 있어야 하는데, 이건 구현하는 사람에 따라 크게 다를 거란 말이죠...? 그래서 간략하게 설명만 하고 넘어가려 합니다.
저의 경우 사다리의 세로선과 세로선을 연결하는 가로선을 중점으로, 연결되어 있는지/아닌지를 열 개수가 하나 적은 2차원 boolean 배열로 나타냈어요. 아마 그림이 대부분의 정보를 설명해 줄 거에요. 이 boolean 배열 정보를 여러 도메인에 보내 출력에 활용하기도 하고, 참가자와 연결되는 실행 결과를 따라갈 때도 활용했어요. 참가자의 시작점으로 이 boolean 배열 값에 따라 왼쪽에 길이 있으면 왼쪽으로, 오른쪽에 길이 있으면 오른쪽으로, 그리고 길과 별개로 아래로 한 칸 내려가고를 반복해 일일이 실행 결과를 찾았어요. 길 따라 일일이 이동시켜 찾았다는 말을 길게 풀어썼을 뿐이에요.
실행 결과 📷
결과 확인 부분에서는 요구하는 실행 결과와는 조금 다르지만, "all" 을 치면 전체 참가자를 반환한다는 정보를 좀 더 직관적으로 제공하고 싶었고, 현재 미션의 요구사항만으로는 프로그램을 정상 종료시킬 방법이 없어 "exit" 을 별도로 추가해 "exit" 을 입력하면 프로그램이 종료되도록 구현했어요. 당연하지만 이 경우 "all" 뿐만 아니라 "exit" 도 참가자의 닉네임이 될 수 없으므로, 이 부분도 금지된 닉네임으로 작동하도록 수정했습니다.
색다른 도메인 덕에 나름 재밌게 구현할 수 있었던 미션이었어요.
잘 부탁드려요!