[그리디] 이창희 사다리 미션 제출합니다! #45
Conversation
구현 전략은 리드미에 적어 놓았습니다.
요구사항과 구현 전략 작성
InputView를 통해 크기를 입력받고, LadderGameController 에서 Ladder클래스의 팩토리 메서드를 통해 사다리를 생성한다.
MovingDirection enum 클래스를 통해 상황별 이동 전략을 캡슐화 한다. 사다리 타는 로직을 담은 메서드 구현 Ladder -> ride()메서드 HorizontalLine -> move() 메서드
매번 MovingDirectioin을 배열로 생성했던 이전 방식에서 필드로 저장하여 사용하는 방식으로 개선
유저의 정보를 관리하는 User 클래스 추가 게임 결과를 관리하는 Prize 클래스 추가 게임의 결과를 계산하고 생성하는 LadderGameResult 클래스 추가
기존 명령형으로 작성했던 getRandomLines()메서드 선언형으로 변경하여 불변성 유지 (stream API) 그 외 getter 불변객체 반환하도록 변경
There was a problem hiding this comment.
안녕하세요~! 리뷰가 좀 늦어져서 죄송해요..ㅜㅜ 처음 리뷰를 해보려니 막막했던 것 같아요
그래도 전체적으로 코드를 깔끔하게 잘 짜주셔서 이해하기가 편했습니다!!
함수형 프로그래밍에 중점을 두셨다 해서 그 위주로 코멘트를 남기고 싶었으나, 창희님께서 이미 적절한 곳에서 적용해주신 것 같아 드릴 피드백이 잘 떠오르지 않았네여..그래도 나름대로 코멘트 달긴 했는데, 쉽지 않네요 ㅋㅋㅋㅜㅜ
+저는 enum을 연결여부 저장할 때만 사용하였는데 창희님께서는 방향정보도 enum으로 저장 후 구현하셨더라고요! 더 직관적인 것 같아 저도 참고하면 좋을 것 같다는 생각이 들었습니다 ㅎㅎ
|
|
||
| private final Map<User, Prize> gameResults; | ||
|
|
||
| public LadderGameResult(Map<User, Prize> gameResults) { |
There was a problem hiding this comment.
이 부분은 정팩메를 쓰니까 private으로 바꿔도 괜찮을 것 같습니다..!
| HorizontalLine generatedLine = LineGenerator.generateHorizontalLine(width, linkedGenerator); | ||
|
|
||
| List<Link> links = generatedLine.getPoints(); | ||
| for (int i = 0; i < links.size() - 1; i++) { |
There was a problem hiding this comment.
allsatisfy를 활용하는 건 어떻게 생각하시나요?
for문 보다는 직관적이진 않은 것 같지만, 함수형 스타일에는 더 가깝다고 생각들어요!
There was a problem hiding this comment.
조언 감사드려요~~!!
allsatisfy를 적용해서 개선해 보았는데 생각보다 복잡해 져서 stream API를 사용한 방식으로 바꾸어 봤어요 👍
allsatisfy는 사용해 본적이 없는데 덕분에 배워 갑니다
|
|
||
| public class InputValidator { | ||
|
|
||
| private static final String USERNAME_NAMES_PATTERN = "^([a-zA-Z가-힣0-9]+)(,[a-zA-Z가-힣0-9]+)*$"; |
| Link prev = Link.UNLINKED; | ||
| List<Link> link = new ArrayList<>(); | ||
|
|
||
| for (int i = 0; i < width - 1; i++) { |
There was a problem hiding this comment.
width-1 대신 MAX_LINK_INDEX 이런 식으로 상수로 바꾼다면 더 직관적일 것 같습니다!
There was a problem hiding this comment.
width 값은 generateHorizontalLine(int width, LinkGenerator generator)
이렇게 파라미터로 전달되는 값이라 상수로 바꾸기는 힘들것 같아요..! 🥲
그래도 파라미터의 변수명을 직관적으로 바꾸면은 이해가 더욱 쉬울것 같군요!
| } | ||
|
|
||
| public static LadderGameResult of(Users users, Prizes prizes, Ladder ladder) { | ||
| return new LadderGameResult(calculateResult(users, prizes, ladder)); |
There was a problem hiding this comment.
of는 객체 생성에 초점이 있다고 생각하는데.. static 메서드인 calculateResult를 이용해 계산 결과 값을 전달하면 단일 책임 원칙을 위반할 수 있을 것 같아요! 혹시 어떻게 생각하시나요?
There was a problem hiding this comment.
제 생각을 말씀 드리면,
저는 이 미션을 설계 당시 LadderGameResult가 사다리 게임의 결과를 계산하는 책임을 맡도록 구상했어요
그렇기 때문에 클래스 단위로 볼때 LadderGameResult는 사다리 게임의 결과를 계산하는 역할만 하기 떄문에 (+ 결과 조회 로직) SRP를 준수하고 있다고 생각해요!
서희님이 말씀해 주신 정팩메의 SRP 위반 여부는 메서드 단위의 책임을 뜻하는것 같아요
정팩메 안에서 결과 계산 로직이 실행되어서 생성책임 이외의 추가적인 책임을 갖는다 라는 상황 같은데
저는 각 메서드의 책임을 이렇게 생각하고 코드를 짰어요
정팩메 - 외부에 제공되는 클래스의 진입점, 결과 계산을 calculateResult에 위임하고 객체를 생성
calculateResult - 실제 계산 로직을 수행
생성자 - 불변 필드를 초기화
각각 책임이 나뉘어 있다고 생각하는데 어떻게 생각하시나요?
관점의 차이인것 같기도 하구 ㅋㅋㅋㅌ 😅
model 내부의 패키지를 ladder, user prize로 세분화 생성자 private로 변경 테스트 코드 for loop -> Stream API 변경
View 클래스의 메서드 static -> Non static으로 변경하여 컨트롤러에서 필드로 선언하여 사용하도록 변경
|
서희님 안녕하세여 우선 코멘트 남겨 주신 부분 수정했구요 추가적으로 View클래스 내 메서드들을 static -> non static으로 바꾸고 컨트롤러에서 필드로 선언해서 사용하도록 바꾸어 보았어요 |
안녕하세요 서희님~ 🙋♂️
일단은 리뷰하는 자리니 존댓말로 가겠슴다
이번에는 스터디원끼리 리뷰를 하는 자리를 갖게 되었는데요,
저도 누구 코드 리뷰하는건 처음이라 기대가 되기도 하면서도 쬐꼼 부담스러운 마음이 드네요
(나도 잘 모르는데 잘 할 수 있을까? 하는 그런..?)
서희님도 제 코드를 보면서 어떻게 리뷰를 해야 하지? 하고 막막해 하실지도 모르겠군요
솔직히 제 코드도 일주일 전에 작성한거라 다시 읽기도 쉽지 않은데, 더더욱 남의 코드를 읽고 이해하고 생각을 나눈 다는 게 쉽지 않은 일 같아요
리뷰어님들께 더욱 감사해 지네요.. ㅋㅋㅋㅋ
그래도 최대한 가독성이 좋은 코드를 작성하려고 노력해 보았으니 리드미에 적어둔 클래스 설명 의 흐름따라 이해해 주시면 될 것 같아요
아무튼 서로 몰랐던 부분은 의논하고, 생각을 나누어 보면서 발전시켜 나가봅시당
저는 이번에 미션을 하면서 가장 신경을 썼던 부분이 "함수형 프로그래밍" 을 적극적으로 적용해 보자 였어요
그 중 가장 중요한 개념을 한 문장으로 정리하면, 다음과 같다고 생각을 했어요
미션을 진행 하면서 상태의 변화를 최소화 하려고 노력 했어요
그래서 모든 필드를
final로 선언을 하였고,Stream API를 적극적으로 활용 했습니다하지만
상태 변화의 최소화를 지키려다가 오히려 너무 복잡하고 가독성이 안 좋아지는 것 같은 느낌이 들면 가감없이 포기를 했어요변수를 선언하고
for문으로 상태 변화를 반복하고 있는 Ladder의ride 메서드와LineGenerator의 로직이 이에 해당해요조금 유연하게 적용하는게 더욱 낫다고 판단을 했어요 😅
(혹시 개선할 다른 좋은 방법이 있다면 조언 부탁드려요!)
제 코드를 읽어 보시면서 이해가 안 가는 부분이나 이렇게 작성한 의도가 뭐지? 혹은 내 생각과는 다른데? 하는 점이 있다면 편하게 코멘트 남겨 주세요
잘못된 점이나 개선할 포인트 들이 보인다면 조언해 주시면 더더욱 좋구요!!
같이 의논해 보아요~~ 화이팅 🙋♂️
P.S. 편한 시간에 천천히 리뷰 부탁 드려요~~