2주차 미션/ 서버 4조 조규빈 #41
Conversation
| import ladder.creator.*; | ||
|
|
||
| public class LadderGameFactory { | ||
| public static LadderGame createBasicLadderGame(GreaterThanOne numberOfRow, GreaterThanOne numberOfPerson) { |
There was a problem hiding this comment.
GreaterThanOne 인자 두개를 받는 메서드나 클래스가 많은데,
두 인자를 변수로 갖는 사다리 사이즈에 대한 객체가 있으면 관리가 더욱 용이할 것 같아요
|
|
||
| public class LadderPrinter { | ||
| private final Row[] rows; | ||
| private StringBuilder sb; |
There was a problem hiding this comment.
sb 변수는 printLadder 메서드 내에서만 사용하고 있는 것 같은데, 해당 메서드 내에서도 setLength(0) 를 통해 초기화하고 있네요!
클래스의 멤버 변수로 선언하지 않아도 괜찮을 것 같아요!
| public void printLadder(Position position, int currentRow, boolean isAfter) { | ||
| sb.setLength(0); // StringBuilder 초기화 | ||
| for (int i = 0; i < rows.length; i++) { | ||
| sb.append(addPositionMarker(rows[i].rowToString(), position, i == currentRow)); | ||
| sb.append("\n"); | ||
| } | ||
|
|
||
| System.out.println(isAfter ? "After" : "Before"); | ||
| System.out.print(sb.toString()); | ||
| System.out.println(); | ||
| } | ||
|
|
||
| private String addPositionMarker(String rowString, Position position, boolean isCurrentRow) { | ||
| String[] parts = rowString.split(" "); | ||
| StringBuilder rowSb = new StringBuilder(); | ||
|
|
||
| for (int i = 0; i < parts.length; i++) { | ||
| if (i == position.getValue() && isCurrentRow) { | ||
| rowSb.append(parts[i].equals("-1") ? "-1*" : parts[i] + "*"); | ||
| } else { | ||
| rowSb.append(parts[i]); | ||
| } | ||
| if (i < parts.length - 1) { | ||
| rowSb.append(" "); | ||
| } | ||
| } | ||
| return rowSb.toString(); | ||
| } |
There was a problem hiding this comment.
LadderPrinter 에서 node 의 정보를 확인하고 그리기보다는
StringBuilder 를 인자로 넘겨주고, 직접 Row -> Node 레벨로 들어가 해당 클래스 내에서 사다리 방향이나 현재 위치를 그려주면 훨씬 객체지향적인 코드가 될 것 같아요.
그렇게 되면 자연스럽게 "-1" 과 같은 하드코딩을 피하고 Direction 객체를 재사용할 수 있을 것 같네요!
| printer.printLadder(position, i, false); //Before 출력 | ||
| rows[i].nextPosition(position); | ||
| printer.printLadder(position, i, true); //After 출력 |
There was a problem hiding this comment.
주석처리보다는 Before, After 상수를 갖는 enum 클래스를 만들어보는건 어떤가요??
| private final Row[] rows; | ||
|
|
||
| public BasicLadderCreator(GreaterThanOne numberOfRow, GreaterThanOne numberOfPerson) { | ||
| rows = new Row[numberOfRow.getNumber()]; |
|
|
||
| public class AutoLadderCreator implements LadderCreator { | ||
| private final BasicLadderCreator basicCreator; //조합 사용을 위한 private class | ||
| private int count; |
There was a problem hiding this comment.
해당 변수명 자체로도 어떤 역할을 하는지 알 수 있도록 네이밍하면 더욱 유지보수에 용이합니다!
지금은 count 의 변수가 직관성이 살짝 떨어지는 감이 있는데, 클래스의 멤버 변수로 선언하지 말고 createLadder 메서드 내에서 직관적인 변수명으로 초기화 해주시면 좋을 것 같아요!
| public class AutoLadderCreator implements LadderCreator { | ||
| private final BasicLadderCreator basicCreator; //조합 사용을 위한 private class | ||
| private int count; | ||
| private final Random random = new Random(); |
There was a problem hiding this comment.
random 변수도 createLadder 메서드 내에서만 사용하고, 클래스 바깥에서 알 필요가 없으므로 클래스 멤버 변수로 선언하지 않아도 좋을 것 같아요.
| private final GreaterThanOne numberOfRow; | ||
| private final GreaterThanOne numberOfPerson; |
There was a problem hiding this comment.
위에 언급했듯이 두개의 객체를 묶는 사다리 사이즈 정보 객체를 생성하면 좋을 것 같아요
| int x = random.nextInt(numberOfRow.getNumber()); | ||
| int y = random.nextInt(numberOfPerson.getNumber() - 1); | ||
| drawLine(Position.from(x), Position.from(y)); |
There was a problem hiding this comment.
랜덤 포지션의 상한값은 잘 지정해주었지만 음수값이 발생할 수 있네요.
random.nextInt 의 범위값을 양수로 지정해주면 Position.from 에서 음수 예외가 발생할 상황을 없애서 시간을 단축하고, 더욱 안전한 코드가 됩니당.
2주차 과제입니다! 수정할 부분이나 부족한 부분 있으면 말해주세요!