-
Notifications
You must be signed in to change notification settings - Fork 252
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
[2단계 - 사다리 게임] 레디(최동근) 미션 제출합니다. #357
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.
안녕하세요 레디,
뭔가 이번에 많은 것을 학습하시면서 미션에 녹여보신 것 같네요ㅎㅎ👍🏻
새로운 개념을 적용해보는 것은 좋은 시도인 것 같습니다!
새롭게 적용하다보니 기존 코드와 상반되는 스타일이 생기신 것 같아요.
잘 고민해보시고 일관성있게 작성해주시면 좋을 것 같습니다😁
관련해서 피드백 남겼으니 확인 부탁드릴게요!
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.
리뷰 요청 보내려니까 model과 view를 더 빡세게 분리해보고 싶은 생각이 드네요...ㅎㅎ
request change 보내주시면 다시 열심히 분리해보겠습니다!
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.
안녕하세요 레디,
리팩터링을 통해 코드가 많이 개선됐네요👍🏻
2단계 요구사항에 모두 충족해서 머지해도 될 것 같은데,
더 욕심을 내보고 싶으신 것 같아서 몇가지 커멘트 남겼습니다.
Ladder ladder = Ladder.of(height, players.size()); | ||
|
||
outputView.printResult(players.getNames(), ladder.getFormattedLines()); | ||
outputView.printLadderResult(players.getNames(), ladder.getFormattedLines(), prizes.getPrizeNames()); |
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개씩 넣어서 결과를 출력하고 있어요,
개선해볼 수 있을까요?
} | ||
|
||
private void printResult(final LadderResult ladderResult, final Players players) { | ||
Set<Player> playerNames = new HashSet<>(players.getPlayerNames()); |
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.
Set으로 처리하는 이유가 있나요?
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.
프로그램 종료 조건을 등록된 player의 결과를 전부 요구할 경우로 하였습니다!
그래서 Set에 player를 담고 입력된 player를 제거한 후 이 set이 빌때까지 계속 결과를 보고 싶은 사람을 입력 받았습니다. 입력된 player를 제거할 때(remove) List보다 Set으로 처리하는 것이 더 효율적이라고 판단하여 Set으로 처리하였습니다
다시 보니까 LinkedList로 처리해주어도 괜찮았을 거 같네요:)
src/main/java/model/Ladder.java
Outdated
public LadderResult findResult(final Players players, final Prizes prizes) { | ||
final int ladderSize = players.size() - 1; | ||
List<String> prizeResult = new ArrayList<>(); | ||
List<Prize> result = prizes.getPrizeValues(); | ||
|
||
for (int index = 0; index < players.size(); index++) { | ||
prizeResult.add(result.get(findLadderBottomIndex(index, ladderSize)).value()); | ||
} | ||
return LadderResult.of(players.getNames(), prizeResult); | ||
} |
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.
지금 Ladder한테 Players, Prizes 값을 던져주고 결과를 찾으라고 하고 있습니다.
그러면 애초에 Ladder가 둘다 알고 있었으면 안됐을까요?
Controller에서는 그냥 Ladder한테 결과만 요청할 수 있지 않을까요?
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.
해당 방법으로 구현해보았습니다!
변경하면서 새로운 고민점이 생겼는데, Ladder가 둘다 알고 있는 방법을 사용하기 위해 필드에 저장을 하는 식으로 구현을 하였고, 필드가 수정되다 보니까 생성자를 건들이게 되고, 생성자를 건들이면서 테스트코드까지 건들이게 되더라구요
그래서 기존에 Ladder 테스트에 있던 테스트들에도 Players와 Prizes를 주입해주어야 해 간단한 테스트였던 것들이 크기가 꽤 커졌습니다.
기존 테스트들은 되도록 수정하지 않으면서 기능을 수정하는 것이 좋은 구조라고 생각되는데, 이렇게 무언가 수정이 있을 때 테스트 코드를 건들이지 않고 잘 수정하는 방법이 있을까요?
구현 시작전부터 완벽한 설계를 하고 구현을 해야하는 것일까요?
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.
Ladder의 역할이 기존에는 사다리 내용만 알고 있었습니다.
하지만 Players, Prizes가 추가되면서 사다리 구성에 필요한 추가 정보들이 생겼으니, 테스트에 변경이 생기는 일은 불가피한 것 같습니다. 기존에 테스트가 존재했었기 때문에 prod 코드에 수정이 일어나서 기존 로직들이 변경된다는 것을 잘 인지할 수 있지 않았을까요?
객체에 필드가 추가되고, 생성자가 변경되는 것은 큰 변화인데 오히려 테스트 코드가 수정없이 돌아가는 상황이 더 위험해보여서 오히려 테스트의 장점을 잘 경험해보셨다고 생각합니다.
느끼신 불편했던 점은 테스트 코드의 트레이드오프라고 생각되네요. 테스트가 있으면 귀찮은 작업들이 존재하는 것이 사실입니다, 그런데 저는 귀찮은 비용이 가치 있다고 생각하는 것 같아요, 조금 덜 귀찮게 하기 위해서 Fixture를 이용하면 그나마 변경점을 덜 가져가실 수 있을 것입니다.
구현 시작전부터 완벽한 설계를 하고 구현을 해야하는 것일까요?
우선 완벽한 설계가 존재하는지를 고민해봐야할 것 같은데, 사다리 게임만 보더라도 레디가 구현한 것 처럼 Ladder에게 역할을 부여할 수도 있고 LadderGame이라는 객체를 만들어서 Ladder, Players, Prizes를 넣어서 사용하는 방식이 있을 수도 있겠죠. 둘 다 저는 좋은 설계라고 생각합니다, 장단점이 있는 것이죠.
서비스 규모가 커지면 커질수록 미리 설계를 하고 구현하는 것은 어려워질 것 입니다. 그리고 소프트웨어 엔지니어링 업계는 다른 엔지니어링 업계와는 다르게 유연하게 대처할 수 있습니다. 그렇기 때문에 TDD 같은 방식을 시도해서 리팩터링을 지속적으로 하고 테스트코드로 안정성을 유지하면서 개발하는 것이죠. 그렇다고 설계를 안하는 것은 아닙니다, 대강 ERD 정도는 그리고 시작을 하는데 언제든지 변경될 수 있다는 가정을 깔고 개발을 해야합니다. 그래서 객체 지향적으로 개발을 하게 된 것 같아요.
앞으로 미션을 하시면서 리팩터링을 하면서 코드가 바뀌고 테스트코드가 변경되는 일이 계속 발생할거에요. 그런 과정을 겪으면서 테스트를 얼만큼 만들것인지? 어떻게 만들것인지에 대한 기준이 잡히실 겁니다. 계속 이런 고민을 해보세요ㅎㅎ
public boolean isCmdAllResult(String cmd) { | ||
return CMD_ALL_RESULT.equals(cmd); | ||
} |
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.
커맨드 입력에 대한 확인도 LadderResult의 책임인가요?
안녕하세요 레디!
정답은 없습니다. 도메인 관계와 크기에 따라서 많이 달라질 것 같아요. 더 나아가서 문맥과도 관련이 있습니다. Player의 사이즈만 필요하다면 사이즈만 전달해주는게 가독성이 더 좋은 코드가 될 수 있습니다. 그런데 피드백 드린 것처럼 만약 Ladder 가 Players를 들고 있다면 어떨까요? 본인의 LadderSize를 계산하는 로직을 안에서 사용할 수 있지 않을까요? List 와 Players를 자꾸 전달해서 사용해야 한다면 적절하게 일급컬렉션으로 묶었는지 고민해보고 연관관계를 한번 고민해보시면 좋을 것 같습니다.
네, 컨트롤러가 프로그램 제어의 역할을 맡고 있기 때문에 적절한 위치에서 하고 있다고 보여집니다. |
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 void printResult(final LadderResult ladderResult, final Players players) { | ||
Set<Player> playerNames = new HashSet<>(players.getPlayerNames()); |
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.
프로그램 종료 조건을 등록된 player의 결과를 전부 요구할 경우로 하였습니다!
그래서 Set에 player를 담고 입력된 player를 제거한 후 이 set이 빌때까지 계속 결과를 보고 싶은 사람을 입력 받았습니다. 입력된 player를 제거할 때(remove) List보다 Set으로 처리하는 것이 더 효율적이라고 판단하여 Set으로 처리하였습니다
다시 보니까 LinkedList로 처리해주어도 괜찮았을 거 같네요:)
src/main/java/model/Ladder.java
Outdated
public LadderResult findResult(final Players players, final Prizes prizes) { | ||
final int ladderSize = players.size() - 1; | ||
List<String> prizeResult = new ArrayList<>(); | ||
List<Prize> result = prizes.getPrizeValues(); | ||
|
||
for (int index = 0; index < players.size(); index++) { | ||
prizeResult.add(result.get(findLadderBottomIndex(index, ladderSize)).value()); | ||
} | ||
return LadderResult.of(players.getNames(), prizeResult); | ||
} |
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.
해당 방법으로 구현해보았습니다!
변경하면서 새로운 고민점이 생겼는데, Ladder가 둘다 알고 있는 방법을 사용하기 위해 필드에 저장을 하는 식으로 구현을 하였고, 필드가 수정되다 보니까 생성자를 건들이게 되고, 생성자를 건들이면서 테스트코드까지 건들이게 되더라구요
그래서 기존에 Ladder 테스트에 있던 테스트들에도 Players와 Prizes를 주입해주어야 해 간단한 테스트였던 것들이 크기가 꽤 커졌습니다.
기존 테스트들은 되도록 수정하지 않으면서 기능을 수정하는 것이 좋은 구조라고 생각되는데, 이렇게 무언가 수정이 있을 때 테스트 코드를 건들이지 않고 잘 수정하는 방법이 있을까요?
구현 시작전부터 완벽한 설계를 하고 구현을 해야하는 것일까요?
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.
안녕하세요 레디!
리팩터링을 진행하니 클래스의 역할이 더 명확해진 것 같습니다.
레이어간 분리도 잘 진행해주신 것 같네요.
테스트 코드에 대한 고민들도 정말 필요한 고민이었다고 생각합니다.
관련해서 답변 달아드렸는데 궁금한 점이 있으면 DM 주세요.
사다리 미션 고생 많으셨습니다!
남은 미션들도 잘 수행하시고 많이 성장하시길 바랍니다👍🏻
안녕하세요 아서👋
2단계 미션 제출합니다!
TDD를 하면서 요구 사항에 핏한 코드를 작성할 수 있음을 배우고 또 리팩토링 과정에선 테스트 코드의 장점을 깨달으면서 코딩을 하고 있네요 😄
2단계 진행하면서 고민하였던 부분들을 남겨보아요
예외시 재입력
예외시 재입력 기능을 구현하려고 하였으나 요구 사항이 아닌 것을 구현하기 위해 이정도의 노력을 들일만한 가치가 있을까라는 의문점이 생겨 구현하다가 다시 제거하고 리팩터링에 집중하였습니다.
기운이 남는다면 다시 도전해보겠습니다😀
파라미터의 계층(?)
Players 나 Result 와 같이 일급 컬렉션 객체를 파라미터로 넘겨줄 때 일급 컬렉션 자체로 넘겨주는 것이 좋을지 아니면 getter로 꺼내와 List 형태로 넘겨주는 것이 좋을지 고민을 하다가 전자를 택하였는데, 과연 적절한 선택이였는지 궁금합니다.
변경 시점 커밋
일급 컬렉션을 활용하자는 취지에서 이런 방식을 사용하였는데, 어차피 사용하는 부분에서 꺼내게 될텐데 미리 꺼내서 넘겨주는 것이 간편할 수도 있겠다고 생각도 듭니다.
결과를 전부 보여줄때까지 종료하지 않음
예제의 입출력문을 보면 처음에 결과를 보고 싶은 사람으로 pobi를 입력하고, 그 다음으로는 all 을 입력하고 프로그램이 종료되었습니다. 실세계의 사다리 타기를 생각해보면 결국 모든 사람의 결과를 출력하고 종료되어야 한다고 생각하여 개별로 전부 요청하거나 all을 입력하면 종료하도록 기능 구현을 하였습니다. (명확한 요구 사항이 없어서 꽤 고민을 하였지만)
커밋 내용
이런 기능을 컨트롤러에서 처리해주는 것이 과연 맞을까요?
감사합니다🙌