Skip to content

사다리 게임 2단계 개발 #7

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

Merged
merged 10 commits into from
Nov 18, 2018
Merged

Conversation

phs1116
Copy link

@phs1116 phs1116 commented Nov 17, 2018

No description provided.

Copy link
Contributor

@javajigi javajigi left a comment

Choose a reason for hiding this comment

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

지금 코드는 복잡도가 좀 높은 것 같은데요.
수업 중 한 것처럼 완전히 TDD로 다시 한번 구현해 보는 것은 어떨까요?


public Gamer(String name) {
Preconditions.checkArgument(name.length() <= MAX_GAMER_NAME_LENGTH, "이름이 너무 깁니다.");
this.name = name;
}

public Gamer(String name, int position) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 생성자의 name은 유효성 체크를 하지 않아도 되나?

@@ -16,23 +17,46 @@
public static final int NUMBER_TO_GET_WIDTH = 1;

private List<Gamer> gamers;
private List<LadderLayer> ladderLines;
private List<LadderLayer> ladderLayers;
private List<String> rewards;
Copy link
Contributor

Choose a reason for hiding this comment

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

"3개 이상의 인스턴스 변수를 사용하지 마라"는 원칙을 지키면서 구현해 본다.

this.gamers = gamerNameList.stream().map(Gamer::new).collect(Collectors.toList());
this.ladderLines = Stream.iterate(0, i -> i + 1).limit(height)
.map(integer -> new LadderLayer(getWidth(), supplier)).collect(Collectors.toList());
public Ladder(List<String> gamerNameList, List<String> rewardList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

변수명과 메소드 이름에 List와 같은 자료구조 이름을 사용하는 것을 추천하지 않는다.
List와 같은 복수의 데이터의 경우 복수형을 쓰는 것이 더 적합하다.

@@ -16,23 +17,46 @@
public static final int NUMBER_TO_GET_WIDTH = 1;

private List<Gamer> gamers;
Copy link
Contributor

Choose a reason for hiding this comment

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

"일급 콜렉션을 사용하라"는 원칙을 지키면서 구현해 본다.

@@ -4,14 +4,35 @@
* Created by hspark on 16/11/2018.
*/
public class LadderLine {
private int leftPosition;
private int rightPosition;
private boolean drawn;
Copy link
Contributor

Choose a reason for hiding this comment

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

"3개 이상의 인스턴스 변수를 사용하지 마라"는 원칙을 지키면서 구현해 본다.

@@ -8,13 +8,31 @@
public class Gamer {
public static final int MAX_GAMER_NAME_LENGTH = 5;
private String name;
private int position;
Copy link
Contributor

Choose a reason for hiding this comment

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

재사용성과 단순한 구현을 위해 Gamer, Result와 사다리타기 엔진을 완전히 분리해 구현해 보는 것은 어떨까?
사다리타기 엔진에서는 시작 위치와 사다리 실행 결과 위치 값만 반환하는 구조로 설계하면 어떨까?

@javajigi javajigi merged commit 3402d0b into next-step:phs1116 Nov 18, 2018
pcspapa added a commit to pcspapa/java-ladder that referenced this pull request Aug 28, 2020
parkeeseul pushed a commit that referenced this pull request Sep 3, 2020
* refactor: 리뷰내용 반영

메소드명 동사화
사다리 폭 테스트

* refactor: 모든 원시값과 문자열을 포장한다.

Boolean to Point

* feat: Line 에서의 분석기 (#7)

Closes #7

* refactor: Point 클래스 position 필드 추가에 대한 기존 클래스 변경

* refactor: 모든 원시값과 문자열을 포장한다.

현재 위치 값 index 를 Chessmen 으로 포장함.

* feat: Ladder 에서의 분서기

Closes #6

* feat: 실행 결과 입력하기

Closes #8

* feat: 결과 요청 메시지

Closes #12

* bugfix: LineAnalysis 버그

LineAnalysis 에서 상태 계산 버그.
LineAnalysis 에서 속도 개선을 위한 선택가능한 라인만 추출

* feat: 결과 요처어

1. 1인의 결과 요청
1. 모두의 결과 요청

Closes #9, #10

* refactor: 실행결과 유효성 체크

* feat: 1. 실행 결과 출력하기

Closes #12

* feat: 사다리에 결과값 출력하기

Closes #13

* feat: 일급 일급컬렉션으로 - Players

Closes #14

* feat: 일급 일급컬렉션으로 - Prizes

Closes #15

* feat: 결과를 만드는 행위를 Line, Ladder 에서 하도록 수정하자.

Closes #16

* refactor: Analysis class 삭제 및 중복 제거

* refactor: 리뷰내용 반영
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.

2 participants