[사다리게임] 미션 피드백 부탁드립니다. by mywnajsldkf · Pull Request #5 · sproutt/sproutt2-ladder

mywnajsldkf · 2022-07-26T09:04:37Z

이전 PR에서 불필요한 파일이 함께 올라가 문제가 되었습니다. rebase 사용해서 수정을 해보긴 했는데 어렵네요... (이것도 정리해보겠습니다.)
한번에 구현하는데 어려움이 있어 먼저 사다리 생성부터 피드백 요청드립니다.

사다리 생성 방법

participant(참여자 수)를 width로, height(사다리 높이)를 height로 두고 width * height 크기의 사다리를 만든다.
width 크기의 point로 구성된 line을 height번 반복하여 생성한다.
- point는 이전 point의 상태를 확인하고 생성된다.
생성된 line은 height만큼 쌓아올린다.

+) 원래 생각했던 방법
사다리를 바둑판으로 보고 line을 세로로 두고 가로로 이동하면서 point를 지정하려고 했습니다. 저는 계산이 무척 복잡해지던데(부분집합이 계속 생기고 이전 line과 비교하면서...) 이런식으로 접근하신분도 계신가요?

어려운 점

함수 반복을 줄이는 방법

private ArrayList<String> toStringParticipantList(List<Participant> participants) {
        ArrayList<String> stringParticipantList = new ArrayList<>();
        for (Participant participant : participants) {
            stringParticipantList.add(participant.getName());
            stringParticipantList.add("  ");
        }
        return stringParticipantList;
    }

    private ArrayList<String> toStringWinningList(List<Winning> winnings) {
        ArrayList<String> stringWinningList = new ArrayList<>();
        for (Winning winning : winnings) {
            stringWinningList.add(winning.getWinningInput());
            stringWinningList.add("  ");
        }
        return stringWinningList;
    }

동작하는 내용은 같은데 반환값이 달라 두번 작성하는 경우가 있었습니다. 반복을 줄일 수 있는 방법이 있을까요? -> 상속을 이용해보았습니다.

패키지 분리

작성한 코드를 mvc 패턴을 적용하여 패키지 분리를 하였습니다.
적절하게 분리가 되었는지 피드백 주시면 감사하겠습니다~

- Participant 객체 없음

- 이름이 5글자 이상일 경우, Exception 발생 O - 이름이 5글자 이하인 경우, Exception 발생 X

- Winning 객체 없음

- Point 객체는 사다리의 선과 선의 교점을 나타낸다.

- Line 객체는 Point가 한줄 모였을 때를 나타낸다.

- Ladder 객체는 참가자와 높이 수만큼 Point의 집합니다.

- participant 크기만큼 초기화한 line을 생성 - 생성한 line을 height만큼 Ladder ArrayList에 추가

- Q: parseToParticipant와 parseToWinning의 반환값만 다르고 같은 일을 한다. 중복되는 부분을 줄일 수 있는 방법이 있을까?

- 사다리를 구성하는 한줄(참가자 크기 사이즈)을 생성

- Line getLadder(int i) 메서드 삭제: Ladder를 구성하는 line 반환 / List<Line> getLines() 메서드 생성: Ladder를 구성하는 line의 모음을 반환

- Info 클래스를 상속하여 사용

- asList() 사용하여 변환 테스트

hyukjin-lee · 2022-08-02T11:30:18Z

src/main/java/controller/GenerateStringLadder.java

+        generatedStringLadder.add(toStringPlayerList(players));
+
+        for (Line value : lines) {
+            ArrayList<String> generatedLine = new ArrayList<>();


모든 타입 선언은 구체클래스가 아닌 인터페이스로 통일되면 좋을 것 같습니다.
지난번에 공유드린 이펙티브 자바 아이템을 참고하시면 좋을 것 같아요 ~

hyukjin-lee · 2022-08-02T11:30:57Z

src/main/java/controller/GenerateStringLadder.java

+
+        for (Line value : lines) {
+            ArrayList<String> generatedLine = new ArrayList<>();
+            Line line = value;


value 라는 변수명보다 좀 더 구체적인 명칭은 없을까요~?

@hyukjin-lee lineValue는 어떤가요??

hyukjin-lee · 2022-08-02T11:32:42Z

src/main/java/controller/GenerateStringLadder.java

+                if (!(line.getLine().get(j)).getConnection()) {
+                    generatedLine.add(EMPTY);
+                }
+                if (line.getLine().get(j).getConnection()) {


26번째 줄에서도 공통으로 호출되고 있는 부분인데 변수로 빼면 어떨까요?
그리고 getConnection 보다는 이어져있는지 판별하는 메서드명이 어울리지 않을까 싶습니다
ex isConnected()

hyukjin-lee · 2022-08-02T11:34:30Z

src/main/java/controller/LadderGame.java

+import java.util.List;
+
+public class LadderGame {
+    public Ladder generateLadder(int height, int participant) {


LadderGame 은 상태를 가지지 않는 클래스인 것 같아보이네요. 입력받은 파라미터들로 메서드 내부의 로직으로만 반환값을 만들 수 있다면 클래스로 따로 빼는게 좋을지? 아니면 다른 책임으로 옮겨갈 여지가 있을지 고민해보시는 것도 좋을 것 같습니다

@hyukjin-lee 오호! 피드백 감사합니다~~

hyukjin-lee · 2022-08-02T11:36:10Z

src/main/java/model/Height.java

+import exception.HeightIsUnderMiniHeightException;
+
+public class Height {
+    private static final int MINI_HEIGHT = 1;


MINIMUM 이라고 풀네임으로 적어주셔도 좋을 것 같습니다

hyukjin-lee · 2022-08-02T11:36:45Z

src/main/java/model/Info.java

@@ -0,0 +1,17 @@
+package model;
+
+public class Info {


Info 라는 클래스는 매우 추상적인 클래스로 보이는데요, 따로 만드신 이유가 있을까요?

@hyukjin-lee
Info 클래스는 Player와 Winning 클래스에서 상속해서 사용하는데요~

Player와 Winning의 정보를 입력받아 List 형식으로 만들어 반환하는 기능을 구현할 때, Player와 Winning을 래핑해서 사용하다보니 메서드의 동작은 같지만 반환값이 각각 List<Player>, List<Winning>으로 다른 두개의 메서드가 작성되었습니다.

중복을 줄일 수 있는 방법이 무엇일지 고민하다 상속이 떠올랐고 상속할 클래스로 Info를 만들었습니다.

hyukjin-lee · 2022-08-02T11:39:20Z

src/main/java/model/Ladder.java

+
+import java.util.List;
+
+public class Ladder {


요구사항으로인해 일급컬렉션을 만들고 값들을 다 래핑하신 것 같은데, 실제로 그것의 장점이 반영되지 못한 것 같습니다.
Ladder 클래스에 위임될만한 책임이 없을지 고민해보셔도 좋을 것 같아요. 지금은 생성자로 생성하고 getter 로 호출하는 것 뿐이어서 일급컬렉션의 의미가 퇴색되지 않나해서요 ~ :)

@hyukjin-lee
'상태와 행위를 한 곳에서 관리할 수 있다'는 일급 컬렉션의 장점을 좀더 살려보라는 것으로 이해해도 될까요?

hyukjin-lee · 2022-08-02T11:40:45Z

src/main/java/model/Line.java

+
+public class Line {
+    private static final int NON_CONNECTION = 0;
+    private static final int CONNECTION = 1;


boolean 값으로 표현하면 어떨까요? 숫자로 표현하신 이유가 있을지 궁금합니다 ~

private boolean isVisited() { Random random = new Random(); int randomNumber = random.nextInt(BOUND_NUMBER);

방문 여부를 숫자를 사용하여 랜덤으로 지정하려고 표현하였습니다~

hyukjin-lee · 2022-08-02T11:42:31Z

src/main/java/model/Line.java

+    }
+
+    private boolean isVisited() {
+        Random random = new Random();


랜덤값이 다뤄지는 경우에는 테스트를 어떻게 하는게 좋을까요?

@hyukjin-lee
제 생각은 값이 랜덤한가를 테스트하는 것은 어렵다고 생각합니다. 또 테스트 코드를 작성하는 의도가 자바의 라이브러리를 테스트하는 것이 아닌 제가 작성한 코드를 테스트하는 것이라고 생각합니다.
그래서 랜덤으로 나온 값을 호출하는 것을 테스트하는 것이 필요하다고 생각합니다.
여기서는 random으로 생성된 randomNumber가 제가 생각하는 범위(BOUND_NUMBER)안의 숫자이고, 타입(int)이 올바른지 파악하고 싶습니다.

hyukjin-lee · 2022-08-02T11:42:57Z

src/main/java/model/Player.java

+
+import exception.PlayerNameIsOverMaxNameSizeException;
+
+public class Player extends Info {


info 를 만들어서 상속시킨 이유가 궁금합니다 ~
그리고 상속과 조합의 차이점에 대해서 공부해보셔도 좋을 것 같아요 :)

@hyukjin-lee
Player와 Winning에 해당하는 값을 입력받아 가공할 때 둘 사이에 String 타입의 파라미터로만 이루어졌다는 공통점이 있어, 동일한 로직이 타입만 다르게 중복되었습니다.

중복을 줄일 수 있는 방법이 무엇일지 고민하다 상속이 떠올랐고 상속할 클래스로 Info를 만들었습니다.

피드백을 반영하고 리서치하다보니 상속이 적합하지 않다는 생각이 들었습니다.
상속은 '포유류(동물을 상속)는 동물이다.' 처럼 확실한 is-a 관계여야하는데 'Player는 정보다' 정보가 매우 추상적이고 자손인 Player와 Winning 사이에도 확실한 연관관계가 없기 때문입니다.

따라서 이를 해결할 수 있는 방법을 고민해보았습니다.

Player와 Winning을 공통으로 하는 클래스를 생성해서 이를 각각 Player와 Winning으로 재정의하자.
Player와 Winning의 공통된 부분을 Info라고 한다면 Info Player;, Info Winning; 처럼 입니다. 코멘트를 반영하다보니 LadderGame에서 전체적인 흐름을 관리하게 되었고 LadderGame을 생성할 때 값을 검증해도 되겠다고 생각했습니다.

하지만 이 경우에 Player와 Winning에 추후에 기능이 추가되면 코드를 전체적으로 수정해야하므로 이 방법은 적절하지 않다고 생각했습니다.

인터페이스를 사용한다.
이 경우에는 인터페이스 사용이 상속으로 구현한 것과 차이가 없다고 생각해서 적절하지 않다고 생각했습니다.

조합 사용
말씀하신 조합을 공부하게 되었고, StringInfo를 변수로 가지도록 코드를 수정했습니다.

+) 다만, WinningLotto에서 BonusBall이나 Lotto 클래스를 조합하여 사용할 경우 관계가 확실한테 제 경우에는 Player와 StringInfo, Winning과 StringInfo의 관계가 추상적이라고 생각합니다. 문제는 변수명이라고 생각하는데 마땅히 떠오르지 않네요😅

hyukjin-lee · 2022-08-02T11:43:37Z

src/main/java/model/Winning.java

+public class Winning extends Info {
+    private static final int MINIMUM_NUMBER_SIZE = 0;
+    private static final String LOSING_TICKET = "꽝";
+    private static final String numberPattern = "^[0-9]+$";


상수는 대문자라는 컨벤션이 어겨진 것 같습니다

hyukjin-lee · 2022-08-02T11:44:48Z

src/test/java/model/HeightTest.java

+        int testHeight = -1;
+
+        // when & then
+        assertThatThrownBy(() -> new Height(testHeight))


assertThrows 도 직관적이어서 추천드립니다 ~

@hyukjin-lee 감사합니다! 덕분에 다른 테스트에서 사용해보았습니다~

hyukjin-lee · 2022-08-02T11:45:39Z

src/test/java/model/HeightTest.java

+
+public class HeightTest {
+    @Test
+    public void should_throwException_when_height_is_under_1() {


JUnit 5 는 테스트에 접근제어자를 붙이지 않아도 됩니다 ~
그리고 @DisplayName 으로 테스트의 목적을 표현해보면 어떨까요? (~인 경우 ~할 때 ~ 한다)

hyukjin-lee · 2022-08-02T11:46:05Z

src/test/java/model/WinningTest.java

+import static org.assertj.core.api.Assertions.*;
+
+public class WinningTest {
+    @ParameterizedTest


parameterizedTest 를 사용해보셨네요 bb

hyukjin-lee · 2022-08-02T11:46:33Z

build.gradle

 dependencies {
-    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.7.0'
-    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
+    implementation 'org.testng:testng:7.1.0'


이것은 어떤 용도의 라이브러리일까요?

@hyukjin-lee
정확한 이유는 모르겠으나 사용하고 있지 않은 것을 보니 잘못들어간 것 같습니다.
덕분에 찾아보니 JUnit과 NUnit에서 나온 자바 테스트 프레임워크라고 합니다.
Junit과 차이를 보자면

Annotations

JUnit 5는 테스트 클래스를 초기화하거나, 테스트 후 실행시킬 때 @BeforeAll과 @afterall을 사용해야 합니다.
TestNG는 그럴 필요 없이 사용할 수 있습니다. 사용할 수 있는 어노테이션은 다음과 같습니다.
@BeforeSuite, @AfterSuite, @BeforeTest, @AfterTest, @BeforeGroup, @aftergroup, @BeforeMethod, @AfterMethod
selenium을 사용한 자동 테스트 앱을 만드는데도 사용된다고 하네요. (참고)

Parameterized testing

모두 Parameterized 테스트가 가능합니다. TestNG는 testng.xml이나 @dataProvider 어노테이션을 사용해서 할 수 있습니다.

결론은 JUnit이 더 많이 사용되고, 기본적으로 IDE와 함께 제공됩니다. 하지만 TestNG는 다양한 테스트를 할 수 있는 추가 옵션들이 있으니 용도에 맞게 선택해서 사용하면 될 것 같습니다.

+) IntelliJ IDEA에서 TestNG를 사용 방법에 대한 아티클이 있습니다.
테스트 결과를 HTMl 파일로 직관적으로 확인할 수 있는게 매력적인 것 같네요~

Ref

https://en.wikipedia.org/wiki/TestNG

hyukjin-lee · 2022-08-02T11:46:49Z

build.gradle

-    testRuntimeOnly 'org.junit.jupiter:junit-jupiter-engine:5.7.0'
+    implementation 'org.testng:testng:7.1.0'
+    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.2'
+    testImplementation 'org.junit.jupiter:junit-jupiter-params:5.8.2'


params 라는걸 처음봐서 그러는데 어떤 용도인가요?

@hyukjin-lee 여러 argument를 이용해 테스트를 여러번 돌릴 수 있는 테스트를 할 수 있는 기능인 Junit5의 Parameterized Test를 사용하기 위한 설정입니다!
https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests-setup

hyukjin-lee · 2022-08-02T11:47:08Z

build.gradle

+    implementation 'org.testng:testng:7.1.0'
+    testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.2'
+    testImplementation 'org.junit.jupiter:junit-jupiter-params:5.8.2'
+    testCompile 'org.assertj:assertj-core:3.11.1'


testCompile 과 testImplementation 의 차이점은 뭘까요?

@hyukjin-lee
compile과 implementation의 차이를 살펴보겠습니다.

compile: 모듈을 수정할 때, 직접 혹은 간접 의존하고 있는 모듈을 모두 재빌드한다.

implementation: 모듈을 수정할 때, 직접 의존하고 있는 모듈만 재빌드합니다.

찾아보니 compile은 이제 deprecated되었고, api로 대체되었네요...!
둘 차이를 봤을 때, api를 사용하면

Module의 Library의 인터페이스까지 노출됩니다.

클린아키텍쳐 측면에서도 각 레이어별로 분리가 되어야하는데 api를 사용하면 모듈 계층을 건너뛴 참조가 발생할 수 있어 레이어를 나눈 이유가 없습니다.

이런 이유로 Implementation으로 수정했습니다.

참고:

https://kotlinworld.com/317

https://stackoverflow.com/questions/44413952/gradle-implementation-vs-api-configuration

saint6839 · 2022-08-03T00:41:04Z

src/main/java/controller/LadderGame.java

+        // DESC: left, right 갈 곳이 없다 -> Y축 이동한다.
+        if (!direction.isLeftFlag() && !direction.isRightFlag()) {
+            playerPosition.moveYAxis(playerPosition.getY() + 1);
+            playerPoint = ladder.getLines().get(playerPosition.getY()).getPoints().get(playerPosition.getX());


개인적으로는 체이닝 메서드에 대한 개행을 사용하면 조금 더 보기 좋은 것 같더라구요! 혹시나 필요하시면 아래 순서 따라 들어가서 설정해보시면 좋을 것 같습니다!
Settings -> Editor -> Code Style -> Java
Wrapping and Braces 탭에서 ChainedMethod calls 부분 설정 변경

@saint6839 오~ 감사합니다!!
+) 중요한 것은 아닌데 메서드 체이닝(Method chaining)이라는 이름으로 더 부르는 것 같습니다! 혹시 몰라 공유합니다.

saint6839 · 2022-08-03T01:01:09Z

src/test/java/model/PlayerTest.java

+
+public class PlayerTest {
+    @Test
+    public void should_throwException_when_participantName_over_5() {


camel case와 snake case가 중복 사용된 것 같습니다!

- 불필요한 dependency 삭제 및 버전 수정 - 변수명 수정: MINI -> MINIMUM - numberPattern -> NUMBER_PATTERN - 메서드명 수정

- WinningSize와 PlayerSize 크기 비교

mywnajsldkf added 30 commits July 4, 2022 15:23

docs : 기능 요구사항, 프로그래밍 요구사항, 요구사항 명세 작성

2093c75

test : 실패 케이스 생성

3afdd12

- Participant 객체 없음

feat : Participant 객체 생성, 참여자 이름 글자수 검증 기능 구현

4045488

test : 이름 검증 테스트 추가

87bed0b

- 이름이 5글자 이상일 경우, Exception 발생 O - 이름이 5글자 이하인 경우, Exception 발생 X

test : 실패 케이스 작성

fa3dd28

- Winning 객체 없음

docs : README.md 업데이트

b1ad674

refactor

ac7d3ff

feat : Winning 객체 생성, Winning 입력값 검증 기능

b714b99

test : winning 객체 생성 테스트

385e3ba

docs : update

44b2496

chores : import 패키지 변경

5807893

feat : StringParsing 기능 추가

8191cf8

feat : StringParsing 테스트 코드 작성

3813e85

test : 실패 케이스 생성(Ladder 객체 없음)

0573051

feat : Point 객체 생성

222e4cb

- Point 객체는 사다리의 선과 선의 교점을 나타낸다.

feat : Line 객체 생성

a5ec7a3

- Line 객체는 Point가 한줄 모였을 때를 나타낸다.

feat : Ladder 객체 생성

0a961da

- Ladder 객체는 참가자와 높이 수만큼 Point의 집합니다.

feat : Ladder 초기화 기능 추가

228eae6

- participant 크기만큼 초기화한 line을 생성 - 생성한 line을 height만큼 Ladder ArrayList에 추가

feat : Point 객체 생성 / edit : .gitignore

a96df5b

feat : 당첨 요소 확인

92f1e10

feat : parseToWinning 메서드 추가(당첨 리스트의 ',',공백을 제거함)

05094bb

- Q: parseToParticipant와 parseToWinning의 반환값만 다르고 같은 일을 한다. 중복되는 부분을 줄일 수 있는 방법이 있을까?

feat : getName() 메서드 추가

c9bb2fd

feat : Line 객체 생성

eb9e185

- 사다리를 구성하는 한줄(참가자 크기 사이즈)을 생성

Refactor : Ladder 객체 수정

b395e53

- Line getLadder(int i) 메서드 삭제: Ladder를 구성하는 line 반환 / List<Line> getLines() 메서드 생성: Ladder를 구성하는 line의 모음을 반환

feat : String Ladder 객체 생성

6589342

Refactor : 패키지 분리

1e87f60

수정사항 반영

6240d89

refactor : Winning과 Player 중복을 줄이기 위한 Info 생성

44147b9

- Info 클래스를 상속하여 사용

test : string -> list 변환 테스트

262a366

- asList() 사용하여 변환 테스트

feat : Participant 객체 생성, 참여자 이름 글자수 검증 기능 구현

7bd47ad

hyukjin-lee reviewed Aug 2, 2022

View reviewed changes

mywnajsldkf force-pushed the jeongin-sample branch from 49a9378 to b106b3b Compare August 2, 2022 13:13

Local history로 잘못 삭제한 파일 복구

2c6bfa4

saint6839 reviewed Aug 3, 2022

View reviewed changes

mywnajsldkf added 9 commits August 4, 2022 16:13

chores :

4057d47

- 불필요한 dependency 삭제 및 버전 수정 - 변수명 수정: MINI -> MINIMUM - numberPattern -> NUMBER_PATTERN - 메서드명 수정

refactor: 접근제어자 생략, @DisplayName 어노테이션으로 설명 추가

5e38003

test : LadderGame 생성 테스트 추가

f66a66e

- WinningSize와 PlayerSize 크기 비교

refactor : 상속을 조합으로 수정

1ba98bc

feat : StringLadder 출력 기능 추가

f3ecde4

refactor : Ladder과 Ladder 및 StringLadder를 생성하도록 수정

c151d01

feat : 게임 실행 결과 출력

2082955

chores : info -> stringInfo로 수정, 불필요한 파일 삭제

5944810

chores

052ea01


		import exception.PlayerNameIsOverMaxNameSizeException;

		public class Player extends Info {

Conversation

mywnajsldkf commented Jul 26, 2022

사다리 생성 방법

어려운 점

함수 반복을 줄이는 방법

패키지 분리

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mywnajsldkf Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mywnajsldkf Aug 8, 2022 •

edited

Loading