[그리디] 황혜림 사다리 미션 제출합니다. #46
Conversation
There was a problem hiding this comment.
안녕하세요 혜림님!
이렇게 리뷰를 하는것이 좀 어색하군요..ㅋㅋㅋㅌ
코드 잘 읽어 보았어요
클래스별로 책임이 아주 잘 나뉘어져 있어서 가독성도 좋고 구조적으로도 정말 견고하게 잘 작성하신것 같아요 (저도 한 수 배워 갑니다 ㅎㅎ)
혜림님 코드가 특히나 좋았던 점은 네이밍이 깔끔해서 이해하는데에 시간이 오래 걸리지 않았던것 같아요
그리고 두번째는 model 패키지 내에 역할 별로 또 패키지를 나누어서 잘 정리를 해 주셔서 더욱 편했던 것 같아요
이미 잘 작성해 주셔서 딱히 어떤 리뷰를 달아야 할까 잘 떠오르진 않았지마는 어찌저찌 리뷰를 해 보았네요 ㅎㅎ
P.S. 정답이 있는 분야는 아니라 제 생각과 다르시다면 언제든지 코멘트 달아 주세요
서로 논의해 보면서 공부하는 시간을 가져 보아요
| private Goals createGoals(Players players) { | ||
| try { | ||
| List<String> rawGoals = inputView.inputGoals(players); | ||
| List<Goal> goalList = rawGoals.stream() | ||
| .map(Goal::new) | ||
| .collect(Collectors.toList()); | ||
| return new Goals(goalList, players.size()); | ||
| } catch (IllegalArgumentException e) { | ||
| System.out.println(INPUT_EXCEPTION_MESSAGE); | ||
| return createGoals(players); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분의 잘못된 입력시 회복 로직은 재귀를 이용해서 잘 작성해 주셨어요
하지만 에러 메세지가 조금 불분명한데요,
지금의 방식은 잘못된 입력 시 모든 에러 메세지를 다음의 사진과 같이
올바른 입력 형식이 아닙니다. 다시 입력해주세요. 로 보여주고 있습니다.
좀 더 명확하게 사용자에게 문제 상황을 인지시키려면 이를 어떻게 개선하면 좋을까요?
+) 아마 빠뜨리신것 같은데 System.out.println()도 View로 분리를 해주시면 좋을것 같아요!
| List<Player> players = new ArrayList<>(); | ||
| for (int i = 0; i < rawNames.size(); i++) { | ||
| players.add(new Player(new PlayerName(rawNames.get(i)), | ||
| new Position(i))); | ||
| } | ||
| return new Players(players); |
There was a problem hiding this comment.
Players 를 생성하는 일련의 과정이라고 이해가 되는데요,
지금의 방식은 이 로직이 위치한 컨트롤러에서 도메인의 내부 구조를 전부 알아야 가능한 생성 로직입니다
즉, Players → Player → PlayerName/Position의 "생성 흐름이 컨트롤러에 드러나 있어, 캡슐화가 깨진 상태" 라고 볼 수 있을 것 같아요
이처럼 생성 과정의 세부 로직이 외부(컨트롤러)에 노출되어 있으면 도메인의 생성 방식이 변경되었을 때, 외부(컨트롤러) 코드가 영향을 받을 가능성이 높고, 응집도가 낮은 구조가 됩니다
이를 어떻게 개선하면 좀 더 좋은 코드가 될까요?
| public Map<Player, Goal> play(Players players, Goals goals) { | ||
| Map<Player, Goal> results = new HashMap<>(); | ||
| List<Player> playerList = players.getPlayers(); | ||
| List<Goal> goalList = goals.getGoals(); | ||
| for (int start = 0; start < playerList.size(); start++) { | ||
| Position end = ladder.getGoalsPosition(new Position(start)); | ||
| results.put(playerList.get(start), goalList.get(end.getValue())); | ||
| } | ||
| return results; | ||
| } |
There was a problem hiding this comment.
이 코드도 내부 컬렉션의 외부 노출을 없애고 책임을 분리를 한다면 더욱 함수형 프로그래밍스럽게 리팩토링 할 수 있을 것 같아요
이 말이 조금 안 와닿을 수 있을것 같은데요,
나누어서 설명을 드릴게요
1. 내부 컬렉션의 외부 노출을 없애고 책임을 분리를 한다?
현재의 코드는 players 에서 getter로 List 를 가져와 for 반복문의 정수형 인덱스로 해당하는 Player를 찾아오고 있어요
➡️ 이렇게 getter로 리스트를 가져와 로직에 사용한다면 내부 컬렉션의 구조가 외부에 노출되게 됩니다
(Goals도 비슷한 상황이에요)
하지만 이를 해결할수 있는 쉬운 방법이 있습니다!
이전에 스터디에서 다루었던 디미터의 법칙 기억 나시나요?
디미터의 법칙이 떠오르신다면 쉽게 개선할 점을 알 수 찾을 수 있을 거에요~
2. 함수형 프로그래밍스럽게 리팩토링?
현재의 for 반복문을 통한 결과 집계방식은 새로운 Position을 할당받고 다시 Map으로 추가하는 로직을 담고 있어요
이로인해 루프마다 객체의 상태변화가 일어나게 되고, 불변성이 지켜지지 못하고 있는 상황이에요
그로인해 부수효과(side effect)가 발생할수 있어요
이는 함수형 프로그래밍에서 가장 피하는 상황이죠
그렇다면 이를 for 반복문을 쓰지 않고 함수형 프로그래밍스럽게 리팩토링 해 보는건 어떨까요?
| public void tryMoveAt(Position position) { | ||
| if (isRightPassable(position)) { | ||
| position.moveToRight(); | ||
| return; | ||
| } | ||
| if (isLeftPassable(position)) { | ||
| position.moveToLeft(); | ||
| } | ||
| } | ||
|
|
||
| private boolean isRightPassable(Position position) { | ||
| if (position.getValue() == points.size()) { // 오른쪽 끝, 이동 불가 | ||
| return false; | ||
| } | ||
| return points.get(position.getValue()).isConnected(); | ||
| } | ||
|
|
||
| private boolean isLeftPassable(Position position) { | ||
| if (position.getValue() == Position.MINIMUM_POSITION) { // 왼쪽 끝, 이동 불가 | ||
| return false; | ||
| } | ||
| return points.get(position.getValue() - 1).isConnected(); | ||
| } |
There was a problem hiding this comment.
여기 코드는 사다리를 타고 이동 가능하면 이동하는 로직으로 보여요.
지금 구조도 이해하기 쉽고, 정말 깔끔하게 잘 작성하신 것 같아요! 👏
수정이 꼭 필요한 상황은 아닌것 같지만, 같이 공부하는 입장에서 제가 코드를 짰던 다른 방식을 한 가지 소개드릴게요
저는 Enum 클래스를 활용해서 사다리의 "오른쪽", "왼쪽", "제자리" 와 같은 이동 전략을 분리해보았어요.
저저번인가 스터디에서 Enum 클래스에는 함수형 인터페이스를 통해 값에 함수(로직)를 담을 수 있다는 것을 공부했었죠?
저는 이걸 한번 적용시켜 보았어요
그 개념을 바탕으로 오른쪽, 왼쪽, 제자리 각각이
- 이동 방향 값과
- 이동 가능 여부를 판단하는 전략 함수를 갖도록 만들었어요.
또한 그 전략을 바탕으로 새로운 포지션 값을 반환하는 메서드도 Enum 안에 넣어서,
→ 이동 판단 + 이동 책임을 한 곳에서 처리하게 했습니다
이렇게 하게 되면 기존과 비교해서 장점이 몇가지 있어요
-
새로운 이동 전략이 생겨도 기존 도메인 코드를 수정할 필요 없이,
Enum 클래스에 값만 추가하면 되기 때문에 → 확장성에 유리해요. -
전략(함수)을 값으로 다루고 있고, 외부 상태를 바꾸지 않아서 → 함수형 프로그래밍의 핵심 개념도 만족합니다.
(즉, 함수를 값으로 다루고, 불변성, 부수 효과(side effect) 없음과 같은 특성을 살릴 수 있어요.)
하지만 혜림님 코드와 같이 간단하게 도메인 내 메서드로 처리가 가능한데 분리를 해 버리면
오히려 처음 코드를 읽는 사람이 직관적으로 이해하기가 어려울 수 있을것 같아요 장단점이 있는것 같아요 ㅎㅎ
참고해 보시고 한 번 적용해 보는 것도 좋을것 같고 기존 방식을 유지 하셔도 상관 없을 것 같아요
단지 제가 어떻게 생각했나를 공유해보고 싶었어요 정답이 있는 부분은 아니니까요 😄
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(name, position); | ||
| } |
There was a problem hiding this comment.
이렇게 hashCode() 도 재정의 해 주신 부분 좋네요!! 👏
그렇다면 객체의 동등성을 판단하기 위해 equals()를 재정의를 할때
왜 hashCode()를 같이 재정의를 해야 하는 걸까요??
| private final InputView inputView; | ||
| private final OutputView outputView; | ||
| private final LadderGenerator generator; | ||
|
|
||
| public LadderController(InputView inputView, OutputView outputView, LadderGenerator generator) { | ||
| this.inputView = inputView; | ||
| this.outputView = outputView; | ||
| this.generator = generator; | ||
| } |
There was a problem hiding this comment.
이렇게 In/OutVIew를 필드로 선언해 주신 부분 좋은것 같네여 👍
저 같은 경우는 View 클래스의 메서드 들을 static으로 선언해서 컨트롤러에서 바로 사용하였는데,
이렇게 필드로 갖고 의존성을 주입을 받으니까 더욱 명확하게 컨트롤러는 View클래스에 의존하고 있구나 라는 것을 알 수 있는것 같아요
안녕하세요 창희님😊 이번 미션 잘 부탁드립니다~!
저도 누군가를 리뷰하는 게 거의 처음이라 뭔가 쉽지 않을 것 같단 생각이 드는데, 처음이시라면 비슷한 생각이 드실 것 같아요. 어떤 피드백이든 도움이 많이 될 것 같아서 편하게 말씀해주시면 감사하겠습니다!!
1-5단계를 같이 하는 건 이번이 처음이었는데, 점점 조건이 늘다 보니 확실히 신경 쓸 게 많고 오래 걸리더라구요...! 그래서 우선 로직을 간단히 완성시키고 하나씩 조건들에 맞게 수정해 나갔습니다.
이번에 사다리 검증 부분과 입력 부분에 신경을 많이 썼던 것 같아요. 서치를 많이 해보면서 복구하는 방식으로 예외 처리도 바꿔보고 또 사다리 만들 때 검증하는 코드 부분에서도 계속 수정하면서 시간을 많이 쏟았습니다..ㅎㅎ 조건이나 규칙 중에 놓친 부분이 있는지, 구조에 더 좋은 방향성이 있는지, 혹은 창희님은 어떤 식으로 진행하셨는지 등 모든 피드백 환영합니다🙇♀️
프로젝트 구조
controller
model
util
view
test