Skip to content
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

[사다리 게임] 베디 미션 제출합니다 #25

Merged
merged 59 commits into from
May 20, 2019

Conversation

dpudpu
Copy link

@dpudpu dpudpu commented May 17, 2019

지적 많이 해주시면 감사하겠습니다.

starkim06 added 30 commits May 14, 2019 14:19
split(string numbers) 메소드가 문자열을 받아서 구분자를 기준으로 문자열을 분리해서 리턴.
만약 문자열 앞에 커스텀 구분자가 있으면 커스텀 구분자로 분리
양수를 Positive.class 를 통하여 포장, StringCalculator.class 의 calculate 메소드를 통하여 입력받은 문자열을 숫자로 변환하여 더해서 반환해준다.
Player.class 생성 및 이름 길이에 따른 유효성 검사
입력받은 문자열을 쉼표(,)를 기준으로 구분후 List Player를 생성해 준다.
validateName 메소드를 추출해서 이름의 유효성 검사
Player들을 관리하는 일급 컬렉션
사이즈 체크를 위한 validate 메소드 추가
사이즈가 0인지, subLine이 겹치는(연속 true) 유효성 검사
각각의 Line에 대해서 subLine의 List를 random하게 생성
- 높이와 사용자 수에 따른 유효성 검사
- SubLineRandomGenerator로 부터 생성된 subLine의 List를 가지는 Line의 List를 생성
- subLines의 수가 countOfPlayer보다 많이 생성되는 경우
- true가 연달아 삽입되는 경우
Ladder.class 와 GamePlayers.class 를 가지고 게임을 관리
console game 진행 추가
- GameResult.class: 실행결과 관리
- GameResultGenerator.class : 입력에따라 GameResult.class 를 생성
List<Line> 를 생성하기 위해 LineGenerator.class 사용
starkim06 and others added 5 commits May 17, 2019 16:31
결과를 보고 싶은 사람은?에서 없는 사람 입력할 경우
없는 사용자입니다. 출력
Player의 name에 all 이 들어올 경우 예외로 처리
결과 입력시, 없는 사용자일 경우 상수로 추출
Set을 이용한 허용될 수 없는 이름검증
Copy link
Contributor

@pobiconan pobiconan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

전체적인 객체 설계와 그에 따른 테스트 💯
메소드 분리를 작은 단위까지 하는 것은 좋으나 너무 과하다 생각하는 부분도 있네요.
피드백 몇 개 남겼어요

@@ -0,0 +1,20 @@
package calculator;

public class Positive {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호. 이 연습도 했군요. 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

의식적인 연습으로 TDD 영상 잘 봤습니다 👍

public class LadderConsoleApp {
public static void main(String[] args) {
LadderGameController ladderGameController = new LadderGameController();
ladderGameController.run();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 Controller를 별도로 추가하지 않고 main() 메소드를 controller처럼 사용해도 되지 않을까?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 반영했습니다.

}

private String inputNames() {
return InputConsoleView.inputNames();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 이런 부분까지 메소드로 분리할 필요가 있을까?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정했습니다. 👍

import java.util.List;
import java.util.Set;

public final class GamePlayers {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일급 콜렉션 적용 👍

}

private void validateDuplication(List<Player> players) {
Set<Player> set = new HashSet<>(players);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

중복 체크를 위해 Set을 사용한다면 인스턴스 변수를 'List players' 대신 'Set players`로 구현하는 것은 어떨까?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

List를 한 이유가 Player는 필드로 이름만 가지고 있어서 List의 index로 position을 찾기 위해서 한거였는데요.
Player에 position 필드를 추가하고 Set으로 변경 했습니다.

import java.util.HashMap;
import java.util.Map;

public class GameResult {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GameResult에 Ladder를 전달하기 보다 Map<Integer, Integer> result = ladder.play()와 같은 실행 결과만을 GameResult에 전달하는 것은 어떨까?

또는 GameResult result = ladder.play(gamePlayers, playerRewards)와 같이 구현하는 것은 어떨까?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 다시 생각해보니 이렇게 하는게 좋을거 같아서 반영했습니다.


private boolean isRight(int index) {
return index < subLines.size() && subLines.get(index);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수업 중에 다룬 Point 또는 Direction과 같은 객체의 추가를 통해 isLeft, isRight 구현부 조건절을 좀 더 단순하게 구현해 보는 것은 어떨까?

List<Line> lines = Arrays.asList(new Line(line1.size(), () -> line1), new Line(line2.size(), () -> line2));
ladder = new Ladder(lines.size(), gamePlayers.size(), () -> lines);

gameResult = new GameResult(ladder, gamePlayers, playerRewards);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GameResult 테스트를 위해 데이터 초기화 과정이 너무 복잡하다.
여기서 힌트를 얻어 GameResult의 구조를 좀 더 개선해 볼 방법이 없는지 고민해 보면 좋겠다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 반영 했습니다. 감사합니다.

dpudpu added 20 commits May 18, 2019 17:58
파라미터 받던 것을 안받는 것으로 수정.
subLine(가로줄)을 Direction으로 변경해주면서
기존의 SubLineGenerator와 LineGenerator는 삭제하고 List<Direction>를 생성해주는 DirectionsGenerator를 생성해주고
DirectionsGenerator를 생성해주는 팩토리를 구현.
팩토리 패턴으
Player에 position을 추가해줘서 List의 인덱스로 position을 찾을 필요가 없어져서
중복 제거 차원에서 Set으로 변경
GamePlayer와 PlayerRewards로 게임의 결과를 구해주는 Ladder.play()
기존에는 List<Player>를 반환 해서 그 값으로 외부에서 Players 인스턴스를 생성했지만,
더 복잡한거 같아서 한번에 Players 인스턴스 생성 후 반환하게 변경
일급컬렉션의 컬렉션은 불변이 보장되어야 하는데 그대로 return 해주면 같은 주소를 참조한다.
그래서 외부에서 값 변경시 일급컬렉션의 컬렉션도 같이 변경 되서 변경
@dpudpu
Copy link
Author

dpudpu commented May 19, 2019

Direction을 추가하니까 연쇄적으로 변경되는 코드들이 정말 많아서 고생했습니다. '차라리 엎어버리고 새로 하고 싶다'라는 충동도 느끼고 이래서 클래스 사이의 결합도가 낮아야 한다는 것을 뼈저리게 느낄 수 있었습니다.

그리고 기존에 있던 Generator를 조금 더 수정해서 팩토리 패턴으로 구현했는데요. 구현하고 나서 객체지향 강의를 보니까 팩토리 패턴을 사용하면 유연해지지만 이른 추상화는 추천하지 않는다고 하더라고요.

  • 아직 존재하지 않는 기능에 대한 이른 추상화는 주의 (잘못된 추상화 가능성, 복잡도만 증가)
  • 실제 변경, 확장이 발생할 때 추상화 시도

사실 지금 굳이 할 필요는 없었는데 나중에 확장도 생각하면서 설계를 하면 좀 더 유연하게 하지 않을까 하는 마음으로 구현했습니다. 포비님은 어떻게 생각하시나요? 현재는 팩토리패턴을 사용할 필요가 없으니 미리 하지 말고 나중에 기능이 추가될 때 구현하는 게 더 나을까요?

@pobiconan
Copy link
Contributor

@dpudpu
팩토리 패턴까지 적용하지 않더라도 사다리를 생성하는 Factory 객체가 있는 것은 좋지 않을까 생각해요.
글에 말한 것처럼 저도 섣부른 추상화는 좋지 않다고 생각해요. 단, 그 시점을 찾는 것이 쉽지 않더라고요.

Copy link
Contributor

@pobiconan pobiconan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백 반영 잘 했네요. 💯
이번 연습을 통해 객체 설계의 맛을 느낄 수 있었기를 기대해 봅니다.

@pobiconan pobiconan merged commit aa58382 into woowacourse:dpudpu May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants