[그리디] 염지환 사다리 미션 제출합니다 #48
Conversation
There was a problem hiding this comment.
안녕하세요 지환님!
리뷰가 조금 늦어져서 죄송합니다 🙇♀️
이번에 처음으로 리뷰를 해보게 됐는데, 남들은 나랑 어떻게 다르게 짰을까 구경하는 게 생각보다 정말 재미있더라고요.
같은 요구사항을 보고도 각자 전혀 다른 방식으로 접근하고 구현하는 걸 보면서, '아 이런 식으로도 풀 수 있구나' 하고 많이 배우게 되었어요.
특히 Player가 각자의 위치를 가지고 실제 사다리 게임처럼 내려가는 방식으로 구현된 점이 정말 인상 깊었습니다.
덕분에 전체 흐름도 한눈에 들어와 이해하기 쉬웠던 것 같아요!
한 가지 조심스럽게 말씀드리고 싶은 부분은, 테스트를 위해 사다리를 생성한 뒤 set을 통해 상태를 변경하는 방식이 혹시 의도치 않은 외부 변경을 허용하게 되지는 않을까 하는 점이에요.
사다리의 연결 여부를 외부에서 직접 설정하기보다는, 연결 정보를 담은 객체를 주입받아 사다리를 구성하면 조금 더 안전하고 일관된 방식이 되지 않을까 하는 생각이 들었습니다 😊
저도 아직 많이 배우는 중이지만, 지환님께서 말씀해주신 것처럼 서로 의견을 주고받으며 함께 고민해보면 더 좋은 방향으로 발전할 수 있을 것 같아요!
혹시 제가 놓친 부분이나 궁금한 점 있으시면 편하게 이야기해 주세요 :)
| private void movePlayersDown(Ladder ladder) { | ||
| players.getPlayers().forEach(player -> player.downLadder(ladder)); | ||
| } | ||
|
|
There was a problem hiding this comment.
이 코드는 사다리 게임의 핵심 로직에 해당하는 부분이라 controller에서 직접 다루기보다는 도메인 객체나 게임 흐름을 제어하는 별도 클래스로 책임을 분리하는 것이 더 적절해 보이는데 어떻게 생각하시나요?
There was a problem hiding this comment.
저도 이 부분을 LadderGame과 같은 별도의 객체로 놔두어야 하나 생각을 했었어요! 그런데 그렇게 하면 너무 domain의 객체 수가 많아져서 최대한 컨트롤러에서 구현해 보았는데 지금 보니 이 메서드는 일급 컬렉션인 Players에 넘겨줘도 되는 코드 인 것 같네요 그래서 플레이어들이 사다리를 내려가도록 하는 이 메서드는 Players로 옮겼습니다!
| // "all"이 입력된 경우, 모든 결과를 출력 | ||
| if ("all".equals(name)) { | ||
| printAllResults(players, results); | ||
| } else { | ||
| printSingleResult(name, players, results); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
else 예약어를 쓰지 않는다.
else 예약어를 쓰지 말라고 하니 switch/case로 구현하는 경우가 있는데 switch/case도 허용하지 않는다.
힌트: if문에서 값을 반환하는 방식으로 구현하면 else 예약어를 사용하지 않아도 된다.
프로그램 요구사항에 else 예악어를 쓰지말라고 되어있어요!
There was a problem hiding this comment.
정신없이 하다보니 else를 무심코 사용해 버렸네요ㅜㅜ else를 사용하지 않는 코드들로 다시 변경하겠습니다
| private static String getPointFormat(Boolean point) { | ||
| String result; | ||
| if (point) { | ||
| result = LADDER_POINT; | ||
| } else { | ||
| result = NO_LADDER_POINT; | ||
| } | ||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분에도 else 문이 사용되고 있네요 😊
전체적으로 else 사용을 지양하라는 요구사항이 있었던 만큼,
다른 부분들도 함께 확인하셔서 요구사항에 맞게 수정해주시면 좋을 것 같아요!
| public static void printResult(String name, Players players, Results results) { | ||
| System.out.println("실행 결과"); | ||
|
|
||
| // "all"이 입력된 경우, 모든 결과를 출력 | ||
| if ("all".equals(name)) { | ||
| printAllResults(players, results); | ||
| } else { | ||
| printSingleResult(name, players, results); | ||
| } | ||
| } | ||
|
|
||
| private static void printSingleResult(String name, Players players, Results results) { | ||
| Player player = players.findByName(new Name(name)); | ||
| Result result = results.findByPosition(player.getPosition()); | ||
| System.out.println(player.getName().getName() + " : " + result.getResult()); | ||
| } | ||
|
|
||
| private static void printAllResults(Players players, Results results) { | ||
| for (Player player : players.getPlayers()) { | ||
| Result result = results.findByPosition(player.getPosition()); | ||
| System.out.println(player.getName().getName() + " : " + result.getResult()); | ||
| } | ||
| } | ||
|
|
||
| private static String getPointFormat(Boolean point) { | ||
| String result; | ||
| if (point) { | ||
| result = LADDER_POINT; | ||
| } else { | ||
| result = NO_LADDER_POINT; | ||
| } | ||
| return result; | ||
| } | ||
| } |
There was a problem hiding this comment.
저는 이 코드가 단순히 값을 출력하는 수준을 넘어, 도메인 탐색 로직까지 포함하고 있어 View의 역할을 다소 넘어서고 있다고 느껴졌어요.
View는 보통 전달받은 데이터를 사용자에게 "어떻게 보여줄 것인가"에만 집중하는 계층이기 때문에 저는 해당 로직의 일부를 다른 곳에서 수행하는것이 적절해 보이는데 혹시 View에서 이 로직을 수행하게 된 특별한 이유가 있을까요?
There was a problem hiding this comment.
석준님께서 말씀하시는 로직이 정확히 어떤 부분인지 궁금합니다!
저는 평소에 출력하는 view를 만들 때 값을 가져오거나 보기 좋게 하는 간단한 가공정도는 해도 된다고 생각하는 편이에요
그래서 해당 코드에서도 출력할 값을 찾는 로직과, 출력의 형식을 정하는 로직 정도가있다고 생각하는데 너무 과하다 싶은 로직이 있다면 말씀해 주시면 정정해보도록 하겠습니다!
src/main/java/domain/PointState.java
Outdated
|
|
||
| public static PointState randomState() { | ||
| if (random.nextBoolean()) { | ||
| return MOVABLE; | ||
| } else { | ||
| return NOT_MOVABLE; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
PointState.randomState()는 현재 테스트에서만 사용되는 메서드로 보이는데요, 테스트를 위해 도메인 모델에 기능을 추가하는 건 조금 조심스러울 수 있어요.
테스트는 결국 구현을 검증하기 위한 도구인데, 이를 위해 도메인 모델에 영향을 주게 되면 비즈니스 로직과 설계 의도 사이에 괴리가 생길 수 있고 나중에는 그 코드가 실제 로직에서 의도치 않게 사용될 가능성도 생기기 때문이에요.
그래서 저는 테스트만을 위한 메서드나 코드는 가능하면 도메인 밖에서 해결하려고 하고, 정말 불가피한 경우라면 public보다는 default 같은 제한적인 접근제어자를 사용해서 외부 노출을 최소화하는 편이에요.
There was a problem hiding this comment.
확실히 테스트에서만 사용되고 있네요! 마지막에 코드를 많이 고치면서 놓친 부분이 있었나 봅니다. 해당 로직을 삭제하는 것 보다는 Line에서 사다리를 생성할 때 그대로 이 메서드를 사용하면 domain의 핵심 역할을 부여할 수 있기에 Line에서 랜덤으로 사다리 이동 가능 여부를 생성하는 코드를 삭제하고 PointState의 메서드를 사용하도록 변경했습니다!
|
|
||
| public class LadderController { | ||
|
|
||
| Players players; | ||
| Results results; | ||
|
|
There was a problem hiding this comment.
players와 results는 상태를 가지는 객체인데 현재 외부에 그대로 노출되어 있어 의도치 않은 접근이나 변경 위험이 있어보여요!
There was a problem hiding this comment.
player와 result를 private final 로 변경하고 이 둘을 입력받아 초기화 하는 생성자를 만들어서 해결하였습니다!
src/main/java/domain/Line.java
Outdated
| public class Line { | ||
|
|
||
| private final List<Point> points; | ||
|
|
||
| public Line(int width) { | ||
| points = new ArrayList<>(); | ||
| Random random = new Random(); | ||
|
|
||
| points.add(new Point(generateRandomState(random))); | ||
| for (int i = 1; i < width; i++) { | ||
| PointState newState = generateNextState(i, random); | ||
| points.add(new Point(newState)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
해당 생성자 메서드에 비즈니스 로직이 포함되어 있어 역할이 과중해 보입니다. 생성자는 객체의 생성과 초기화에 집중하는 것이 바람직하다고 생각합니다.
또한 Random 인스턴스를 매번 생성하기보다는 static 필드로 분리하면 불필요한 객체 생성을 줄일 수 있을 텐데, static을 사용하지 않은 이유가 있을까요?
There was a problem hiding this comment.
해당 내용은 PointState에서 주셨던 리뷰를 바탕으로 PointState에서 랜덤한 결과값을 받아오도록 변경하였습니다.
또한 생성자 메서드에 비즈니스 로직이 포함되어 있는 것이 역할이 과중해 보인다고 해 주셨는데. 아마 저 코드를 작성할 당시에는 Line의 Point를 생성하는 것 역시 Line을 초기화 하는 행위에 해당한다고 생각했던 것 같습니다
해당 로직을 분리하여 새 메서드로 만들어 보려고 하는데 해당 메서드를 생성자 안에서 호출하여 내부적으로 객체를 초기화 하는 것이 좋을지 혹은 Ladder를 생성하는 메서드, 즉 외부에서 호출하는 게 좋을 지 석준님의 의견이 궁금합니다!
안녕하세요 석준이형! 아무래도 코드 리뷰는 리뷰다 보니 존댓말로 작성하겠습니다!
상호간 리뷰하는 미션이라 평소보다 더 리뷰하기 좋도록 코드를 작성하는데 신경을 많이 썼지만
또 많이 시간에 쫒기기도 해서 잘 짰는지 긴장이 되네요...
늘 많이 물어보고 배우는 입장이기에 이번에도 잘 부탁합니다!
일단 이번은 평소와는 다르게 최대한 mvc 패턴과 객체지향 원칙을 지켜가며 기능 구현을 빠르게 완성한 다음에
추가적으로 요구사항을 만족시키는 방법으로 짜게 됐어요.
예전에는 처음부터 모든 요구사항을 만족시키려고 시간을 많이 쏟았는데 확실히 이렇게 고쳐가는 것이
조금 더 시간도 단축되고 오히려 모든 기능에 신경써서 코드를 작성하다 보니 객체를 나눌 때도 훨씬 수월했던 것 같아요
그리고 이번에 가장 신경 쓴 부분은 enum과 함수형을 어디에 활용할 수 있는가? 였어요
사실 기능을 먼저 구현하다 보니 둘을 어디에 써야할지 잘 모르겠어서 일단 제가 생각했을 때 적용할 수 있는 부분에
적용해 보았습니다
예를 들면 point의 이동 가능 여부를 enum 객체로 만들어서 이동 가능 여부와 출력에 사용할 string 값을 같이 매핑한 점 정도가 있겠네요
그리고 출력에서 사용했던, Player의 name이나 Result의 result 값을 이용해서 객체를 찾아 반환하는 함수에는 함수형 프로그램을
적용해 보았습니다.
그리고 Players 나 Results 를 초기화해서 생성하는 of 메서드의 경우에서 함수형을 사용해 보았습니다
많이 신경 썼다고 생각했지만 많이 부족할 수도 있으니 천천히 보시고 편안하게 전부 리뷰해 주시면 감사하겠습니다.
정답을 알려주는 것 뿐만 아니라 석준이형은 해당 코드를 짤 때 어떻게 짰는데 둘 중 어느게 더 나을지 토론하는 것도
좋은 방향인 것 같아요 아무래도 같은 미션을 한 사람들 끼리 서로 리뷰를 해 주는 상황에서만 가능한 일이니까요
앞으로 1주일동안 리뷰 잘 부탁드립니다!