-
Notifications
You must be signed in to change notification settings - Fork 252
[1단계 - 사다리 생성] 배키(백은희) 미션 제출합니다 #278
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
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.
안녕하세요 배키~! 리뷰어 썬입니다 ☀️
2번째 미션 진행하시느라 수고 많으셨습니다!
TDD를 잘 활용해서 구현하신 것 확인했습니다 💯
다른 질문들에 대한 답변은 코멘트에 남겨두어서 해당 부분에 대한 답변만 여기에 남길게요~!
TDD 방식
맞습니다. TDD는 처음부터 완벽한 도메인 설계를 기대하고 하는 것이 아닌, 가장 작은 기능/요구사항에만 초점을 맞춰서 지속적으로 코드를 변경해나가는 것이기 때문에 충분히 마주할 수 있는 문제라고 생각해요!
다만, 절대적으로 TDD의 방식을 따르기보다는 어느정도의 도메인을 산출하고나서 해도 된다고 생각합니다. 결국 TDD도 방법론이기 때문에 모두가 똑같이 사용하기보다는 자신의 입맛에 맞게 어떤 부분은 타협하고, 어떤 부분은 아닐 수도 있을 것 같아요.
다만! 지금은 학습을 하는 과정이기 때문에 최대한 미션 요구사항에 나와있는대로 또는 강의에서 배운대로 하는 것을 추천드립니다. 아직은 객체지향적인 도메인 설계가 미숙하기 때문에 코드 한 줄 쳐보기도 전에 어지러워질 수 있거든요 🤯 우선은 TDD에 익숙해지시고 그 후에 고민해봅시다! 리팩토링도 TDD로 부탁드려요 👀
(익숙해지면 도메인 설계가 한층 더 쉬워질 수도...!?)
+) 미션 요구사항에 모든 원시값 포장하기, enum 적용하기가 있는데, 어떤걸 더 포장해볼 지, 어떤 곳에서 enum을 사용해볼 지 고민해봅시다 😎
마지막으로 제가 드린 리뷰가 해당 코드뿐만 아닌 다른 곳에서도 적용되어야 할 부분이 있는 것 같아, 전반적으로 확인해주세요 !
|
|
||
| - [온라인 코드 리뷰 과정](https://github.com/woowacourse/woowacourse-docs/blob/master/maincourse/README.md) | ||
|
|
||
| ## 기능 요구사항 |
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 final BufferedReader reader; | ||
|
|
||
| public InputView() { | ||
| this.reader = new BufferedReader(new InputStreamReader(System.in)); |
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.
Scanner를 사용하지 않고 BufferedReader를 사용하신 이유가 궁금합니다~!
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.
BufferedReader는 Scanner보다 아래와 같은 장점이 있어서 BufferedReader를 사용했습니다.
Scanner는 사용자에게 입력을 받은 즉시 프로그램에 문자를 넘겨줍니다. 반면에BufferedReader는 문자가 개행문자가 나올 때까지 버퍼에 담아두었다가 프로그램에 넘기기 때문에 더 빠른 처리를 할 수 있습니다.Scanner는UncheckedException이 발생해서 해당 클래스를 사용할 때 예외가 발생할지 컴파일 타임에서 예측할 수 없지만BufferedReader는CheckedException을 발생시키기 때문에 예외가 발생할 수 있음을 예상할 수 있습니다.Scanner는 Thread Safe하지 않고BufferedReader는 Thread Safe하기 때문에 멀티스레드 상황에서도 안전합니다.(사실 이 프로그램에서는 필요한 장점인지 모르겠습니다.)
제가 찾은 (Scanner와 비교했을 때)BufferedReader의 단점은 아래와 같습니다.
Scanner는 특정 타입으로 반환해 주는 반면,BufferedReader는String으로만 받을 수 있습니다. 때문에 처리를 위한 추가적인 코드가 필요합니다.
(또 다른 단점이 있을지 궁금합니다!)
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.
추가로, 위와 같은 이유 때문에 System.out.print()도 WriterReader로 수정할 예정입니다!
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.
Scanner는 UncheckedException이 발생해서 해당 클래스를 사용할 때 예외가 발생할지 컴파일 타임에서 예측할 수 없지만 BufferedReader는 CheckedException을 발생시키기 때문에 예외가 발생할 수 있음을 예상할 수 있습니다.
헛 그렇다면 Unchecked Exception과 Checked Exception에 대한 추가적인 조사도 해주실 수 있을까요~?
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.
자바에서 Exception 상속 구조를 보면 아래와 같습니다.

여기서 RuntimeException을 상속받은 예외를 UncheckedException(비검사 예외)이라고 하고 RuntimeException을 거치지 않고 바로 Exception을 상속받은 예외를 CheckedException(검사예외)이라고 합니다.
검사 예외는 간단히 말하자면 컴파일 시점에서 예외를 대비할 수 있도록 하는 예외입니다. 즉, 강제로 예외를 처리할 수 있도록 돕습니다. 대표적인 예로는 IOException이 존재하는데, 아래 코드와 같이 작성하면 컴파일 오류가 뜹니다. IOException이라는 예외를 던졌지만, 그에 대해서 처리하지 않아 발생하는 오류입니다.
public class ThrowExceptionClass {
public void throwIOException() { // 컴파일 에러 발생!!
throw new IOException();
}
}이를 해결하기 위한 방법으로는 2가지가 있습니다.
public class ThrowExceptionClass {
// 1. 호출하는 쪽에서 처리하도록 위임
public void throwIOException() throws IOException {
throw new IOException();
}
// 2. 해당 메서드에서 처리
public void throwIOException() {
try {
throw new IOException();
} catch(IOException e) {
System.out.println("에러 발생");
}
}
}반면에 비검사 예외는 예외에 대비하는 코드를 작성하지 않아도 컴파일 오류가 발생하지 않습니다. 비검사 예외의 대표적인 예로는 IllegalArgumentException이 있습니다.
public class ThrowIllegalArgumentException {
public void throwIllegalArgumentException() { // 컴파일 에러가 발생하지 않음
throw new IllegalArgumentException();
}
}프로그램에서 예외 처리가 매우 중요한 것인데, 개발자가 실수로 예외를 처리하지 못하는 경우도 있습니다. 이를 해결하고자 검사 예외가 존재하는 것으로 알고 있습니다!
src/main/java/model/Ladder.java
Outdated
| private static final String NOT_POSITIVE_HEIGHT = "최대 사다리의 높이는 양수가 되어야 합니다"; | ||
| private final int maximumHeight; |
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 static final String NOT_POSITIVE_HEIGHT = "최대 사다리의 높이는 양수가 되어야 합니다"; | |
| private final int maximumHeight; | |
| private static final String NOT_POSITIVE_HEIGHT = "최대 사다리의 높이는 양수가 되어야 합니다"; | |
| private final int maximumHeight; |
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.
앗 그렇군요..! 확실히 눈에 더 잘 띄는 것 같습니다. 감사합니다 👍 내용 반영하겠습니다!
src/main/java/model/Ladder.java
Outdated
| private LadderRow buildRow(int participantsSize) { | ||
| List<Boolean> lineStatus = new ArrayList<>(); | ||
| lineStatus.add(new Random().nextBoolean()); | ||
| for (int i = lineStatus.size(); i < participantsSize - 1; i++) { |
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.
시작을 lineStatus.size()해야 할까요? 😢
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.
이렇게 사용한, i=0으로 초기화한다면, lineStatus를 비교할 때, 이전 인덱스의 값과 비교하는 과정에서 i의 값이 -1이 될 수 있기 때문에 1로 초기화하려 했습니다. 그런데 이 함수에서 lineStatus.size()가 항상 1이고 1의 의미를 더 표현할 수 있다고 생각하여 이렇게 진행했습니다!
그런데 리뷰어님 말씀을 듣고 생각해보니, 아래와 같은 이유로 1로 바꾸는 것이 좋다는 판단이 섰습니다.
- lineSize의 크기를 한눈에 짐작하기 어렵다.
- i를
lineSize.size()로 초기화하면 값의 변동이 있을 수 있다고 예측할 수 있다.(상수 값을 초기화하고 싶은 의도를 전달하지 못 한다.)
리뷰 반영하여 다시 수정했습니다.
추가로, 리뷰어님도 이와 같이 생각하셔서 저에게 물어보신 걸까요?
src/main/java/model/Ladder.java
Outdated
| } | ||
|
|
||
| private void validateHeightIsPositive(int maximumHeight) { | ||
| if (maximumHeight < 0) { |
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.
네 리뷰 감사합니다!
사실 이전에는 모든 매직 넘버를 상수화했는데, 이번 코드에서 0을 상수화하지 않은 것은 다음과 같습니다.
0이라는 그 값 자체로 충분히 의미가 담겨 있다고 생각한다. (메서드와 코드를 보면 충분히 0의 의미를 이해할 수 있기 때문에, 상수로 의미로 담지 않아도 괜찮다고 생각했다.)- 코드를 전체적으로 볼 때 충분히 이해할 수 있는 코드라고 생각했다.
- 유지보수 관점에서 0은 아직 한번 밖에 쓰이지 않았기 때문에 상수로 뺄 필요가 없다.
상수화하면 다음과 같은 장점이 있습니다.
- 상수로 해당 값을 선언하면 메모리에 캐싱해 두기 때문에 validateHeightIsPositive의 값을 부를 때마다. 값을 로드할 필요가 없어집니다. 이는 최적화에 도움을 줍니다.
- 해당 클래스에
0의 값이 어떤 의미인지 보여줍니다. - 유지보수 관점에서 같은 의미로 쓰이는 매직 넘버를 통합할 수 있다.
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.
재 생각에는 위와 같은 이유로 작성했는데 0도 상수화를 해야 할까요?
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.
좋습니다~! 명확한 기준이 있다면 상관없죠.
저도 0에 특별한 의미가 없다면 0 그 자체가 더 직관적이라고 생각합니다.
근데 사다리 타기 게임에서 최소 높이가 0이어도 되나요?! 0은 양수인가요?
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.
앗.. 사실 위에 댓글을 썼을 때, 뭔가 잘못되었다는 걸 깨달았어요...😅
- 사다리 게임은 최소 높이가
0이 되면 안 된다. validateHeightIsPositive()라는 함수는 메서드명에 양수인지 검증한다고 적혀있지만, 사실 양수가 아닌0은 검증되지 않는다.
두 점을 고려해 봤을 때, validateHeightIsPositive()라는 변수명은 유지하되, 0또한 예외가 발생하도록 수정해야 할 것 같습니다!
|
|
||
|
|
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.
앗.. No newline at end of file을 잘 지켜야 했는데 오히려 더 늘렸네요. 앞으로는 신경쓰겠습니다. 꼼꼼하게 봐주셔서 감사합니다!
src/test/java/model/LadderTest.java
Outdated
| @DisplayName("한 행에는 연속된 선이 존재하면 안된다.") | ||
| @RepeatedTest(100) | ||
| void makeNotContinuousLineInRows() { | ||
| int participantsSize = 5; | ||
| Ladder ladder = new Ladder(1); | ||
| ladder.build(participantsSize); | ||
| LadderRow row = ladder.getRow(0); | ||
| List<Boolean> lines = row.getLineStatus(); | ||
| for (int i = 1; i < participantsSize - 1; i++) { | ||
| Assertions.assertThat(lines.get(i) && lines.get(i - 1)).isFalse(); | ||
| } | ||
| } |
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.
올바른 테스트 코드인가?
100번 실행해서 테스트가 통과한다면, 200번 실행했을 때도 항상 같은 결과라고 단언할 수 있을까요? 😢
테스트 코드에 if나 for 문을 이용한 로직이 들어가야한다면, 구현한 코드를 의심해볼 필요가 있습니다.
테스트하기 어렵다면 도메인의 역할과 책임이 적절히 분리되지 않았다는 것일 확률이 높거든요.
만약 그것도 아니라면 현재 내가 작성한게 정말 단위 테스트가 맞는지에 대해 살펴볼 수 있을 것 같아요!
(단위 테스트가 무엇인지 한번 살펴보고 알려주세요 😄)
Ladder에 Random이 있어야 할까?
랜덤 값에 대한 테스트 이전에 Ladder에 랜덤값이 정말 들어가야할까?를 고민해봅시다. 현재는 도메인의 모든 과정을 포함하고 있거든요! 정말 Ladder라는 곳에 random이 필요한 지, 객체의 책임을 더 분리해볼 지를 생각해보고 2차 리뷰때 알려주세요!
(엇, PR에서 질문 주신부분이군요 👀)
new Random().nextBoolean()이라는 함수가 Ladder 클래스에서 랜덤 값을 생성하는 것이 올바른가?"입니다. Rows의 상태를 변경하는 것은 list의 내부에서 이루어지는 것이 더 나을 것 같다는 생각이 들기도 합니다.
사실 저는 올바른 객체명만 사용한다면 상관 없다고 생각합니다.
단순히 배키가 어떤 생각으로 현재처럼 또는 리팩토링을 통해 변경된 구조를 선택하게 됐는지 잘 설명할 수 있다면 상관 없습니다!
랜덤 값에 대한 테스트
마지막으로 배키가 단순히 랜덤 값에 대한 테스트를 하고 싶으신거라면 프로덕션에서만 사용될 코드와 테스트 코드에서만 사용될 코드를 분리해보는건 어떨까요?
질문) 자동차 경주에서는 어떻게 테스트하셨나요? 🤔
hint) 전략 패턴
+) 테스트명이 적절한가에 대해 고민해봅시다! 연속된 선이 존재하면 안된다라면 저는 ---|--- 상태인 ladderRow가 예외를 던지는 테스트를 예상할 것 같아요.
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.
올바른 테스트인가?
100번 실행해서 테스트가 통과한다면, 200번 실행했을 때도 항상 같은 결과라고 단언할 수 있을까요? 😢
사실 해당 테스트를 작성할 때도 RepeatTest가 걸렸습니다..! 그래서 해당 부분은 일단 삭제하고 연속되는 true 값을 반환하지 않도록 돕는 LadderRowElementGenerator를 작성하고 해당 로직을 테스트했습니다. 자연스럽게 for문과 if문을 이용하지 않게 되었습니다. 그 이후 LadderRowGenerator클래스에서 LadderRowElementGenerator를 사용하여 Boolean 리스트를 생성하도록 수정했습니다.
혹시 여기서 LadderRowGenerator가 연속된 true를 반환하지 않는 리스트를 반환한다는 것을 테스트할 필요가 있나요? 사실 저는 테스트해서 눈으로 직접 보고 싶은데요..🥲 단순한 반복문을 통해서 값을 초기화하는 로직이며 테스트하는 방법도 모르기에 그냥 두었습니다. 좋은 테스트 방법이 있을지 궁금합니다.
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에 Random이 있어야 할까?
랜덤 값에 대한 테스트 이전에 Ladder에 랜덤값이 정말 들어가야할까?를 고민해봅시다. 현재는 도메인의 모든 과정을 포함하고 있거든요! 정말 Ladder라는 곳에 random이 필요한 지, 객체의 책임을 더 분리해볼 지를 생각해보고 2차 리뷰때 알려주세요!
객체의 책임과 역할에 대해서 생각하지 못하고 구현했는데, 정말로 실제로 보니 Ladder가 거의 모든 책임을 가지고 있더라고요..!😅 물론 Boolean의 랜덤 값 리스트를 생성하고 그 값을 연속된 true 값이 없어야 한다.라는 역할도 Ladder에 있었습니다. 이를 해결하고자 Boolean의 랜덤 값 리스트를 생성하는 LadderRowtGenerator를 생성하여 따로 분리했고 연속된 true 값이 없도록 돕는 로직을LadderRowElementGenerator를 작성하여 각각의 역할을 Ladder에서 다른 클래스로 이동시켰습니다.
그러다 보니, Ladder는 그냥 (정제된?) 랜덤 값을 받아서 Ladder를 초기화만 하는 역할을 하게 되었습니다... 마찬가지로 LadderRow또한 초기화만 하는 역할을 합니다. 이런 클래스가 존재하는 것이 맞는 걸까요?
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.
랜덤 값에 대한 테스트
마지막으로 배키가 단순히 랜덤 값에 대한 테스트를 하고 싶으신거라면 프로덕션에서만 사용될 코드와 테스트 코드에서만 사용될 코드를 분리해보는건 어떨까요?
제가 초반에 질문을 잘못 드린 것 같습니다..! 랜덤 값에 대한 테스트보다는, 랜덤 값이 생성되는 로직 때문에 다른 함수의 테스트를 못하는 것이 걸렸던 것입니다. 예를 들어, 사다리 생성의 경우 Boolean 리스트를 생성한 이후 '연속된 true값이 존재한는가?'를 확인하고 싶은데 랜덤으로 생성하다보니 그것을 확인하기 어려웠습니다😭
질문) 자동차 경주에서는 어떻게 테스트하셨나요? 🤔
자동차 경주에서 테스트할 때는 공동 우승자를 찾는 부분에서 랜덤값을 제어할 필요가 있었습니다. 그때는 RandomGenerator를 사용하여 랜덤 값을 생성하는 역할을 Car와 분리했습니다. 때문에 테스트 코드에서 마음대로 차를 이동시킨 후, 공동 우승자를 테스트할 수 있었습니다.
hint) 전략 패턴
리뷰어님 말씀을 듣고 전략 패턴에 대해서 공부해 봤습니다! 테스트 코드의 랜덤 값을 제어하는 방식으로 전략 패턴을 어떻게 사용해야 할지 모르겠습니다. 전략 패턴은 클래스의 공통적인 특성을 각각 인터페이스로 추상화해, 각각의 구체 클래스를 만든 뒤, 속성을 끼워 넣는 방식입니다. 그러나 사다리 생성에 필요한 속성이 따로 없는 것 같습니다.. 조금 더 힌트를 주실 수 있을까요? 😀
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.
+) 테스트명이 적절한가에 대해 고민해봅시다! 연속된 선이 존재하면 안된다라면 저는 ---|--- 상태인 ladderRow가 예외를 던지는 테스트를 예상할 것 같아요.
앗 그렇군요.. 그럼 연속된 선을 생성하지 않는다. 정도가 좋은 것 같네요..!
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.
(단위 테스트가 무엇인지 한번 살펴보고 알려주세요 😄)
단위 테스트는 개별적인 코드 단위(대부분 메서드 레벨)이 의도한 대로 작동하는지 확인하는 행위입니다.
단위 테스트를 사용하는 이유는 간단히 말하자면 원하는 부분에 대한 실행 결과만 확인할 수 있고 안전성, 빠르게 코드의 문제점을 파악할 수 있다는 점이 있습니다.
좋은 단위 테스트를 만족하기 위해서는 FIRST 원칙을 따라야 합니다.
Fast : 단위 테스트는 빨라야 합니다.
Independent : 테스트는 독립적으로 작동해야 합니다. 이전 테스트에 영향을 받으면 실패 원인을 찾기 어렵기 때문입니다. 예를 들어 첫 번째 테스트에서 static 값을 변경했다면 이는 두 번째 테스트에서 영향을 받을 수 있습니다.
Repeatable : 계속 반복해도, 어떤 상황에서든 예상한 대로 테스트가 나와야 합니다. 예를 들어, 랜덤 값이 생성되는 로직은 어떤 값이 나올지 모르기 때문에 랜덤 값에 따라 테스트 코드가 실패할 수 있고 성공할 수 있습니다.
Self-Validating : 출력이나 로그를 사용하는 것이 아닌, 테스트 자체적으로 결과가 나와야 합니다.
Timely : 적시에 테스트를 철저하게 작성해야 합니다.
| static Stream<Arguments> inputName() { | ||
| return Stream.of( | ||
| Arguments.of("abcdef", "참가자의 이름은 최대 5글자까지 부여할 수 있다.", "참여자의 이름은 최대 5글자입니다."), | ||
| Arguments.of(null, "참가자의 이름은 null일 수 없다.", "참가자의 이름은 null 이거나 공백일 수 없습니다."), | ||
| Arguments.of(" ", "참가자의 이름은 공백일 수 없다", "참가자의 이름은 null 이거나 공백일 수 없습니다."), | ||
| Arguments.of("", "참가자의 이름은 빈 문자일 수 없다", "참가자의 이름은 null 이거나 공백일 수 없습니다.") | ||
| ); | ||
| } | ||
|
|
||
| @DisplayName("참여자의 이름이 잘못 입력된 경우 예외가 발생한다.") | ||
| @ParameterizedTest(name = "{1}") | ||
| @MethodSource("inputName") |
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.
@MethodSource는 편리한 기능을 제공하지만 가독성이 좋지 않습니다 😢
저는 최대한 사용을 지양하는 편인데요!
각각의 검증을 따로 테스트하면 실행된 테스트를 봤을때도 한 눈에 어떤 테스트가 수행됐는지 보일 것 같아요 :)
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.
리뷰어님 의견을 듣고 고치는 것이 나을 것으로 판단하여 해당 내용을 수정했습니다.
저는 처음에 아래와 같은 이유로 MethodSource를 사용했습니다!
- 만약
MethodSource를 사용하지 않으면 중복되는 테스트 코드가 발생합니다. @ParameterizedTest(name= )에서 name에 세부 메시지를 인자로 넘길 수 있어 아래와 같이 확인이 쉽습니다.
MethodSource를 사용하지 않으면 서로 다른 메시지를 한 번에 검증할 수 없습니다.
그러나 MethodSource를 사용했음에도 불구하고 아래와 같은 문제점이 발생합니다.
- 위의 사진처럼 인텔리제이에서는 간단하게 정리되지만 '테스트 코드 자체'로는 가독성이 떨어지는 문제가 발생합니다.
- 테스트의 중복이 생깁니다.
혹시 MethodSource를 사용하지 않고 제가 제일 처음에 언급했던 문제점을 해결할 방법이 있을까요?
테스트 픽스처로 해결해 봤는데 괜찮은 방식인지 확인 부탁드립니다 😃
사실 제가 한 것이 테스트 픽스처인지 잘 모르겠습니다
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.
그래서 말씀드린 부분이 Test을 나누는 것입니다! 테스트도 코드이기 때문에 각각을 분리하면
하나의 함수는 하나의 일만 해야 한다를 더 잘 지킬 수 있지 않을까요~?
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.
MethodSource를 사용하면 하나의 함수는 하나의 일만 해야한다에 어긋날 수 있네요..!
MethodSource를 피하는 방식이 가독성 측면이나, 리뷰어님이 말씀하신 측면을 고려했을 때 더 좋은 방식인 것 같아요. 내용 반영했습니다. 해당 내용에 대해서 다시 한번 피드백 부탁드립니다! 4df5fda
| .hasMessage("참가자가 1명 이하인 경우는 존재할 수 없습니다."); | ||
| } | ||
|
|
||
| } No newline at end of file |
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.
POSIX new line에 대해 아시나요~?
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.
파일 끝에는 개행을 꼭 추가해야 한다는 코드 컨벤션 입니다.
이 컨벤션은 UNIX의 표준으로는 파일의 마지막에 개행이 있어야 하는데, 만약 작성 작성하지 않으면 잠재적인 문제가 될 수 있습니다.
개행이 있어야 해당 파일이 끝났다는 것을 알릴 수 있습니다.
gcc는 C 컴파일러로, POSIX에 근거해서 동작합니다. 때문에 파일이 끝났더라고 개행 문자가 없으면 한 줄이 끝나지 않은 것으로 인식하기 때문에 해당 파일이 끝났다는 표시를 해줘야 합니다.
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.
추가로 질문이 있습니다..! 자바는 gcc 컴파일러를 이용하나요?! 찾아봤는데 정보가 없어 물어봅니다😅
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.
넵 답변 감사합니다!! JVM 부분은 한 번 공부해 보겠습니다👍
src/main/java/view/OutputView.java
Outdated
| } | ||
|
|
||
| private void printRow(LadderRow ladderRow) { | ||
| System.out.print(LadderComponent.EMPTY_LINE); |
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.
...실행을 한번 해서 확인해 봤어야 했는데 잘 된다고 확신하고 있었네요 😳
해당 내용은 반영하여 수정했습니다! 감사합니다 :)
Ladder 클래스의 height에 대한 검증은 높이의 역할이다.
size()값을 예측할 수 없는 이유로 위와 같이 수정
모든 getter에 get 키워드를 앞에 붙인다.
코드의 유지보수를 위해 상수 숫자 값을 포매팅 한다.
가독성을 위해서 MethodSource를 삭제하고 중복되는 부분은 함수로 추출(테스트 픽스처)
|
안녕하세요 썬! 꼼꼼한 리뷰 정말 감사드립니다🥹 리뷰해 주신 덕분에 제출한 코드에 대해서 진지하게 고민할 수 있는 계기가 되었어요 :)
시도는 했으나, 정신 차려보면 구현만 하고 있네요ㅜ 특히 랜덤값 관련 로직을 작성할 때면 TDD를 하지 못하는 것 같아요. 아직 머릿속에 어떤 방식으로 랜덤 값 로직을 피해서 테스트해야 할지 몰라서 그런 것도 있는 것 같습니다.
원시값 포장에 대하여 같은 이유로 참가자의 이름인 name도 포장했습니다. name은 5자 이하라는 조건을 검증하는 로직을 분리했습니다. Name은 Height와 다르게 같은 값을 가져도 서로 다른 것을 의미할 수 있기 때문에 record가 아닌 클래스로 작성했습니다.(record는 equals()를 자동 생성하는 것으로 알고 있어서 record를 사용하지 않았습니다.) enum 사용에 대하여 추가로, 리뷰 내용을 반영하면서 궁금했던 점을 같이 올립니다!Q1) 검증 로직만 가진 클래스는 도메인이라고 할 수 있는가?제가 불변 클래스로 분리한 Height 클래스는 단순히 높이가 양수임을 검증하는 로직입니다. 단순히 Height 객체를 생성할 때, 객체의 상태인 value만 Q2) 값 자체만을 변수로 가진 객체는 객체의 역할을 제대로 하는 것일까?1번 질문과 연관될 수 있는 질문이라 생각하는데요..! 제가 생각하는 Height는 높이 값 자체라고 생각합니다. 같은 값을 가진 Height는 동일한 객체라고 생각하고 만든 클래스입니다. 즉, 아래 두 객체인 Height height1 = new Height(0)
Height height2 = new Height(0)그런데 이런 값만 가지는 객체는 도메인이라고 볼 수 있을까요? 아직 초반만 읽었지만 요즘 객체지향의 오해와 사실이라는 책을 읽고 있는데요, 객체는 변수가 아닌 행동에 중점을 맞춰야 한다고 강조하더라고요. 그래서 그런지 값만 가지고 있고 값을 생성할 때 검증만 하는 있는 객체도 의미가 있는지 조금 헷갈립니다... Q3) 전략패턴과 랜덤 테스트랜덤 테스트에 영향을 받지 않도록 작성하기 위해서 전략 패턴을 이용하는 방법을 찾아보았습니다. 랜덤 값 생성 전략에 대한 인터페이스를 생성한 뒤, 실제 랜덤 값을 반환하는 클래스(RandomGenerator)와 고정된 값을 반환하는 클래스(FixedGenerator)를구현합니다. 그 이후 프로덕트 코드에서는 실제 랜덤 값을 반환하는 구현체(RandomGenerator)를 사용하고 테스트 코드에서는 고정된 값을 반환하는 구현체(FixedGenerator)를 사용합니다. 여기서 2가지 의문점이 있습니다.
|
비어있는 칸을 출력할 때 EMPTY_LINE이 아닌, " "로 출력하도록 수정
이름에 관한 검증 로직은 Name으로 이동시키고 Participant가 Name을 가지도록 함
| public static void main(String[] args) throws Exception { | ||
| try (InputView inputView = new InputView()) { | ||
| new LadderController(inputView, new OutputView()).play(); | ||
| try (InputView inputView = new InputView(); | ||
| OutputView outputView = new OutputView()) { | ||
| new LadderController(inputView, outputView).play(); | ||
| } |
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.
checked와 unchecked 예외에 대해 여쭤본 것은 checked를 사용할 경우
매번 throws ~를 붙여줘야하기도 하고 어디선가 이렇게 무조건 잡아줘야 하기 때문이었습니다.
(현재 input, output, controller 에 붙어있네요)
속도면에서는 이정도 규모를 위한 입력이라면 차이가 얼마 없을 것 같구요!
+) 또한 지금 main()에서도 throws를 표시하고 있어서 실제 IOException이 발생할 경우 안잡아주는데 의도하신걸까요?
현재 그렇게 중요한 부분은 아닌 것 같지만, 알고 쓰기를 바라는 마음에서 남겨두었습니다 :)
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.
또한 지금 main()에서도 throws를 표시하고 있어서 실제 IOException이 발생할 경우 안잡아주는데 의도하신걸까요?
예외 발생시 예외를 처리하는 방법 중에서 throws를 표시하는 것이 간단하다고 생각하여 그렇게 작성했는데 그러면 예외를 처리해줄 수 있는 곳이 없네요😅
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.
checked와 unchecked 예외에 대해 여쭤본 것은 checked를 사용할 경우
매번 throws ~를 붙여줘야하기도 하고 어디선가 이렇게 무조건 잡아줘야 하기 때문이었습니다.
(현재 input, output, controller 에 붙어있네요)
속도면에서는 이정도 규모를 위한 입력이라면 차이가 얼마 없을 것 같구요!
네 저도 BufferedReader만 썼을 때는 throws를 사용하는 것이 그렇게 복잡하다고 못 느꼈는데, BufferedWriter를 쓰니 확실히 가독성에도 문제가 생기고 예외를 어디선가 처리해 줘야 하는, 추가적인 코드가 생성되는 것 같습니다. 리뷰어님께서 말씀하신 것처럼 속도면에서도 빠르긴 하겠지만 보는 입장에서는 차이가 없고요.
때문에 BufferedReader와 BufferedWriter 모두 없애는 방향으로 결정했습니다.
| @@ -1,32 +1,14 @@ | |||
| package model; | |||
|
|
|||
| public class Participant { | |||
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.
이름만 가지고 있는 클래스를 따로 만드신 이유가 있을까요?
Participants 내에 List names로도 충분하지 않을까요?
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.
네 그렇네요..! Participant의 name이라는 String 값을 Name이라는 클래스로 분리하면서 검증에 대한 내용은 Name에서 사용 가능하고 자연스럽게 Participant가 Name만을 사용하는 클래스가 된 것 같습니다.
Participant는 Name을 사용할 뿐이지 어떠한 역할도 하지 않기 때문에 해당 클래스는 삭제하겠습니다.
|
|
||
| import java.util.List; | ||
|
|
||
| @FunctionalInterface |
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.
함수형 인터페이스 👍
| public class LadderRowElementGenerator implements BooleanGenerator{ | ||
|
|
||
| @Override | ||
| public boolean updateFalseIfTrue(boolean isTrue) { | ||
| if (isTrue) { | ||
| return false; | ||
| } | ||
| return new Random().nextBoolean(); | ||
| } | ||
| } |
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.
BooleanGenerator와 BooleansGenerator를 굳이 두개로 나눌 필요가 없어보입니다.
BooleanGenerator가 인터페이스의 역할을 제대로 수행하고 있는 건지에 대해 고민해봅시다.
아래를 제가 생각했던 랜덤 테스트 전략인데요. 프로덕션에서는 랜덤을 사용하고 테스트 코드는 검증을 위해 매번 true 또는 false를 반환하도록 할 수 있을 것 같아요. 이때 Ladder에서 연속된 true가 없다는 validation 이 있다면 테스트 코드에서는 매번 true를 반환하는 로직으로 인해 테스트를 해볼 수 있겠네요.
public interface BooleanGenerator {
boolean generate();
}public class RandomBooleanGenerator {
public boolean generate() {
return new Random().nextBoolean();
}
}public class FixedBooleanGenerator {
public boolean generate() {
return true;
}
}굳이 제가 제시해드린 방법이 아닌 배키가 만드신 BooleansGenerator로도 충분히 가능할 것 같으니 꼭 따라할 필요는 없을 것 같아요.
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.
👀
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.
또 이런 실수를..😅 한번 더 전체적으로 검토하겠습니다. 감사합니다!
| return lineStatus; | ||
| } | ||
| } | ||
| public record LadderRow(List<Boolean> isLines) { } |
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.
LadderRow 객체는 역할이 없고 검증하는 로직 또한 없습니다. LadderRow는 지금 상태로는 필요한 객체가 아닙니다.
그러나 LadderRow라는 값에는 연속되는 true가 존재하면 안되기 때문에 이름 검증해야 하는 과정이 있어야합니다.
이를 검증하는 로직을 넣어서 다시 수정했습니다..!
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.
추가로 궁금한 점이 있습니다!
사실 LadderRow 생성자를 호출할 때 매개변수로 넣는 List 타입의 isLines는 이미 연속된 true 값이 존재하지 않는다라는 조건에 충족합니다. 그런데 LadderRow에서 또 검증을 해야 할까요?
제 생각에는 이미 연속된 값이 존재하지 않는 리스트를 반환하기 때문에 검증이 필요한가라는 의문이 들어서요..! 리뷰어님은 어떻게 생각하시는 지 궁금합니다!
|
|
||
|
|
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.
👀
| @@ -0,0 +1,12 @@ | |||
| package model; | |||
|
|
|||
| public record Height(int value) { | |||
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.
해당 글을 살펴보시죠~!
Height 객체가 데이터의 유효성을 검사하는 기능을 제공한다면, 이는 도메인에서 사다리의 높이라는 핵심 기능 중 하나를 표현하고 있다고 생각합니다.
| @Test | ||
| void ladderHeight() { | ||
| Height height = new Height(5); | ||
| Ladder ladder = new Ladder(height, () -> List.of(true, false, true)); |
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.
여기서 전략 패턴이 사용됐다고 볼 수는 있겠네요. 다만, 연속된 true로 인해 올바르게 생성되지 않았다는 테스트가 있었다면 더 좋을 것 같아요.
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.
네 LadderRow 클래스에 추가했습니다. 감사합니다!
| @@ -0,0 +1,25 @@ | |||
| package view; | |||
|
|
|||
| public enum LadderComponent { | |||
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.
저도 동일한 상황에서 사용합니다 😊
이펙티브 자바에 34. int 상수 대신 열거 타입을 사용하라을 추천드립니다 !
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단계때 구현때 참고해주세요~
질문에 대한 답변도 대부분 리뷰 내에 있습니다 !
추가로 정답을 좇기보다는 본인의 개발 철학을 만들어나가는게 중요하다고 생각합니다.
제가 드리는 피드백을 정답으로 보지 않고 의견정도로 받아들여 주시면 더 좋을 것 같아요.
때로는 공식 가이드나 조언보다도 개인의 경험과 직관이 더 중요할 수 있으니
모든 부분에 대한 제 의견을 구하기보다는 본인의 판단에 조금 더 확신을 가져주세요 🙂
장단점을 파악하고 합리적인 이유를 근거로 스스로 결정해나가는 것 또한 우아한테크코스에서 배우기 좋을 것 같아 조심스럽게 말씀드립니다 👀
1단계 너무 수고하셨고 2단계에서 다시 뵙겠습니다 😊


안녕하세요. 썬 😄
리뷰요청을 드리면서 궁금했던 점이 있어서 올립니다!
Ladder 클래스는 아래와 같이 사다리 한 줄(행)을 나타내는 LadderRows라는 클래스를 리스트만큼 가지고 있습니다.
사다리의 생성을 위해서 Ladder 클래스에서 Random.nextBoolean()함수를 아래와 같이 사용했습니다.
여기서 궁금한 점은 리뷰어님이 보시기에 "
new Random().nextBoolean()이라는 함수가 Ladder 클래스에서 랜덤 값을 생성하는 것이 올바른가?"입니다. Rows의 상태를 변경하는 것은 list의 내부에서 이루어지는 것이 더 나을 것 같다는 생각이 들기도 합니다.랜덤 값을 조작하기 힘들어서 아래와 같이 RepeatedTest 방식을 사용했습니다.
아래와 같은 방식 외에도 랜덤 테스트를 잘 분리할 수 있는 방법이 없을까요?
이번 미션에서 저희는 tdd의 방식을 최대한 따라가려 했습니다. 하다 보니 고민이 들었던 것이 처음 코드를 구현할 때 도메인 설계가 선행되어야 하는가? 입니다. 제일 첫 테스트 코드를 작성하기 전에 클래스에 대한 설계를 진행하나요?
이번에 tdd를 진행하면서 생각보다 잘 안 되었습니다. 무엇이 문제인가를 생각해 보니, 요구사항을 이해했지만, 요구사항에 따라서 머릿속에 클래스 설계가 안되어서인 것 같습니다. 그런데 tdd는 바로 테스트 코드를 작성하라고 해서 도메인에 대한 설계 없이 진행했습니다. 이것이 올바른 방식인지 궁금합니다!
읽어주셔서 감사합니다!