-
Notifications
You must be signed in to change notification settings - Fork 53
[3단계 - 사다리] 이지현 미션 제출합니다 #67
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
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.
지현님! 코드 3단계 고생하셨어요 🔥
저번에 Column하고 Row 말씀하셨었는데 수정하셨네요!
간단한 코멘트 남겼어요. 다음 미션도 화이팅입니다 !!
|
||
public Column { | ||
if (value < MIN_COLUMN) { | ||
throw new IllegalArgumentException("사다리의 넓이는 " + MIN_COLUMN + " 이상이어야 합니다."); |
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.
자바 15부터 지원되는 .formatted()
메서드도 고려해보시면 좋을 것 같아요 👍🏻
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 static final int MIN_ROW = 1; | ||
|
||
public Row { | ||
if (value < MIN_ROW ) { |
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.
intelliJ에서 지원하는 단축키로 포맷팅하면 코드 컨벤션에 맞게 정렬하기 수월해요!
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.
Window : Crtl + Alt + L
|
||
import java.util.Map; | ||
|
||
public record LadderResult(Map<Integer, Integer> result) {} |
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.
오 result를 Map을 사용하셨네요 👍🏻
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.
네! 시작점과 끝점을 같이 저장하기 위해 Map을 사용해봤습니다 🙂
이렇게 하니까 결과를 한 번에 관리하기 편해서 좋더라고요!
sb.append(start) | ||
.append(" -> ") | ||
.append(end) | ||
.append(System.lineSeparator()) |
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.
오 System.lineSeparator()
사용 👍🏻
지현님 안녕하세요 이번 3단계 미션 고생 많았습니다 |
|
||
OutputView.printLadder(game.getLadder()); | ||
|
||
//사다리 게임 결과 출력 | ||
Map<Integer, Integer> resultMap = new HashMap<>(); |
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.
시작점과 도착점을 확인하기 위해 HashMap을 사용하신 건가요??
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.
넵 맞습니다! 결과를 한 번에 관리하기 위해서 HashMap을 사용했습니다!
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.
고생하셨습니다!
Width, Height, Col, Row 혼재된 개념을 잘 통일해주셨군요! 👏
몇가지 코멘트 확인 부탁드립니다~
public int play(int start) { | ||
int position = start; | ||
for (int row = 0; row < ladder.getLines().size(); row++) { | ||
position = movePosition(position, ladder.getLines().get(row)); | ||
} | ||
return position; | ||
} | ||
|
||
private int movePosition(int position, Line line) { | ||
if (canMoveLeft(position, line)) { | ||
return position - 1; | ||
} | ||
if (canMoveRight(position, line)) { | ||
return position + 1; | ||
} | ||
return position; | ||
} | ||
|
||
private boolean canMoveLeft(int position, Line line) { | ||
return position > 0 && line.isLinkedAt(position - 1); | ||
} | ||
|
||
private boolean canMoveRight(int position, Line line) { | ||
return position < line.getLinks().size() && line.isLinkedAt(position); | ||
} |
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.
다음 요구사항을 만족시켜볼까요?
Java Enum을 적용한다.
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 static final int MIN_ROW = 1; | ||
|
||
public Row { | ||
if (value < MIN_ROW ) { |
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.
Window : Crtl + Alt + L
|
||
public static void printResult(LadderResult result) { | ||
StringBuilder sb = new StringBuilder(); | ||
sb.append("\n"); |
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.
여기에서는 System.lineSeparator() 를 사용하지 않은 이유가 있을까요?
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.
사실 \n과 System.lineSeparator()가 실제로 같은 동작을 하는지 궁금해서 일부러 두 가지를 섞어서 사용해봤는데, 그 후에 수정을 깜빡했네요!
실제 코드에서는 말씀해주신 대로 일관성 있게 하나로 통일해서 사용하는 게 더 좋을 것 같아요. 이 부분은 바로 수정하도록 하겠습니다!
|
||
OutputView.printLadder(game.getLadder()); | ||
|
||
//사다리 게임 결과 출력 | ||
Map<Integer, Integer> resultMap = new HashMap<>(); |
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.
제 생각에는 Map<Int,Int>는 의미있는 값을 가지고 있긴 어렵다고 생각이 드네요 🤔
물론 변수명으로 표현할 수는 있겠지만 key, value 각각에 의미를 부여하기에는 어려워보이기도 해요
map.get(something)
을 해서 나온 결과값은 어떤 의미를 부여해야할지 계속 고민을 해야할 것 같기도 해요
이럴거면 의미를 명확히 가지고있는 LadderResult에게 주체를 갖게하는건 어떨까요?
한줄요약: Map을 생성하는것이 아닌 LadderResult를 생성하여 값을 추가해주는건 어떻게생각하시나요?
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.
Map으로 묶는 것이 아니라, LadderResult에 int start, int end처럼 각각의 값을 변수로 만들어서 List 형태로 사용하라는 의미일까요?
저는 추후에 사람이름과 결과(상)을 매칭할 때 Map<Person, Prize> 같은 구조로 가져갈 계획이었습니다.
“의미있는 값을 가지고 있기 어렵다”는 말이 조금 애매하게 느껴지는데, 여기서 말씀하신 “의미있는 값”이 어떤 의미인지 조금 더 설명해주실 수 있을까요?
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.
제네릭설정도 좋은 방법인거같아요!
Map이라는 자료구조도 일급컬렉션처럼 래핑해보면 좋겠다는 의미였습니다 ㅎㅎ
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.
아, 제가 리뷰의 의도를 잘못 파악했었네요.
그런데 현재 Map을 바로 쓰지 않고 일급 컬렉션으로 감싸서 사용하고 있습니다.
Map<Integer, Integer> resultMap = new HashMap<>();
for (int start = 0; start < columns.value(); start++) {
resultMap.put(start, game.play(start));
}
LadderResult result = new LadderResult(resultMap);
혹시 리뷰 의도는 Map 자체를 매개변수로 받아서 생성하는 것 보다, 생성하는 책임(로직)도 객체 내부로 가져가라는 의미인지 궁금합니다!
예를 들어
public static LadderResult of(LadderGame game, Column columns) {
Map<Integer, Integer> resultMap = new HashMap<>();
for (int start = 0; start < columns.value(); start++) {
resultMap.put(start, game.play(start));
}
return new LadderResult(resultMap);
}
이런 식으로 생성부터 LadderResult에서 책임지도록 구현하는 걸 말씀하신 걸까요?
현재 다른 일급 컬렉션처럼 외부에서 생성된 컬렉션을 매개변수로 넣어 생성하고 있었는데, 이 경우에는 내부 의미가 약해져서 그런거겠죠?
안녕하세요 주노!
이번 리뷰도 잘 부탁드립니다 😊
고민했던 부분
궁금한 점
추가로 테스트하는 부분 고정된 사다리로 할려고 하는데 꼬였는지 "디버그용 바이트코드(또는 컴파일된 클래스)와 에디터에 열려 있는 소스 파일의 라인 번호가 맞지 않는다"라는 경고가 나오네요. (이 부분은 따로 수정해서 올리도록 하겠습니다 ㅠ )
rebuild랑 cache도 지워봤는데 해결이 안 되어서 밀어버렸는데 혹시 왜 이런 문제가 발생하는지 아실까요?