-
Notifications
You must be signed in to change notification settings - Fork 18
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
[사다리 미션] 신혜빈 미션 제출합니다. #20
base: shin378378
Are you sure you want to change the base?
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.
안녕하세요!
리뷰가 처음이라 이렇게 하는 것이 맞는지 잘 모르겠으나 그래도 최대한 열심히 해봤습니당 혜빈님 코드를 보면서 제가 고민했던 부분에 대해 답을 찾기도 하고, 더 나은 방식이 있다는 것도 알게 되어 의미 있는 시간이었던 것 같습니다 ㅎㅎ
아무래도 테스트 코드를 작성하려면 public 처리된 함수만 단위 테스트를 할 수 있잖아요.
랜덤 사다리 생성과 관련된 함수는 랜덤값이니까 테스트를 하지 못 하고
mockito
와 같은 mocking
라이브러리를 사용하면 테스트할 수 있다고 알고 있어요! 하지만 전 그냥 반복문을 돌렸답니다,, ㅎㅎ true
와 false
가 모두 나오면 리턴해서 테스트를 종료하는 방식으로 랜덤값을 테스트 했어요
private처리된 함수는 클래스 밖에서 사용을 할 수 없으니까 테스트를 하지 못 해서 결과적으로 이번에는 2개의 테스트 코드밖에 생성하지 못 했네요 ㅜㅠㅜ
오 이거 저도 궁금했어요 이번에 미션하다보니까 테스트 코드를 위해 접근 지정자를 수정해야 하는 일이 생기더라구요,,
https://bb-library.tistory.com/265 그래서 이 글을 읽었었는데 혜빈님도 읽어보면 좋을 것 같아요!
우선 저는 주로 제 코드랑 어떤 부분이 달랐는지를 위주로 리뷰를 남겼는데요..! 그래서 다른 부분에 대해 혜빈님과 의견을 공유하고 싶습니다!
그리고 궁금한 부분이 있는데 컨트롤러에는 주로 어떤 메서드를 넣으시는지 궁금합니다!
컨트롤러에 대한 이해가 부족해서 혜빈님은 어떤 기준을 사용하시는지 알고 싶어요 ㅎㅎ
@@ -0,0 +1,42 @@ | |||
# java-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.
헉 리드미를 작성해주셔서 한 눈에 보기가 편하네요!
String participantName = inputView.inputParticipantToWantResult(); | ||
outputView.outputParticipantResult(participantInventory, participantName); | ||
if(participantName.equals("all"))break; | ||
} |
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.
all이 나올 때까지 결과를 보여준다
이렇게 해석할 수도 있네요! 저는 그냥 한 번만 결과를 출력하는 줄 알아서 한 번만 했거든요,,ㅎㅎ
들여쓰기를 1까지 허용한다는 조건을 만족시키기 위해서 do-while문을 사용해볼 수도 있을 것 같아요!
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.
if(participantName.equals("all"))break;
저도 모르게 단순히 이 코드를 들여쓰기가 없는 코드라고 인식하고 있었네요 ㅜㅠ 짚어주셔서 감사합니다!!
현재 아래와 같이 do while문을 수정하였습니다!!
String participantName;
do{
participantName = inputView.inputParticipantToWantResult();
outputView.outputParticipantResult(participantInventory, participantName);
}while(!participantName.equals("all"));
src/main/java/model/Participant.java
Outdated
private static final int NAME_LENGTH_LIMIT=5; | ||
private int position; | ||
private String name; | ||
private String result; |
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개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.
오 저도 이거 result
를 넣고 싶었는데 조건 때문에 못 넣었어요 ㅠ
저는 세 개 중에 두 개만 넣어야 한다면 Participant
가 position
이랑 name
을 가지는 것이 나을 것 같아서 이 두 개로 클래스를 작성했습니다. 제 경우 position
을 움직이는 메서드를 따로 Participant
클래스에 만들고 이를 이용해서 마지막 result
의 인덱스만 저장을 하도록 했습니다. 결국 사용자가 입력받은 결과들은 사용자 입력을 유지한 채로 있을테니까 결과 리스트를 resultList
라고 한다면 resultList.get(participant.getPosition())
이렇게 결과를 출력하도록 했습니다.
혜빈님은 어떻게 생각하시나요??
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.
이번 기회에 저도 곰곰히 어떤 변수를 빼면 좋을까 고민을 했는 데요. 저도 상희님처럼 result결과를 빼기로 결정했습니다!!
차이점이 있다면 값을 Map<사용자이름, 사용자의 결과>
의 형태로 저장해주었어요.
resultList.get(participant.getPosition())
이렇게 리스트로 저장해서 특정 위치에 있는 결과값을 들고오는 것도 좋지만 조금 더 직관적일 순 없을까 고민하다 Map을 사용했는 데요.
특정 위치 대신 사용자이름을 키 값으로 사용해서 사용자의 결과를 뽑아내니까 직관적이라 좋은 거 같아요 :)
result값을 따로 빼라고 피드백해주셔서 감사합니다! 덕분에 조금 더 깔끔한 리팩토링에 많은 도움이 된 거 같아요!
src/main/java/model/Splitter.java
Outdated
afterSplit[i] = afterSplit[i].trim(); | ||
} | ||
return afterSplit; | ||
} |
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.printf("%" + COLUMN_LENGTH + "s", trialResult); | ||
System.out.printf(" "); | ||
} | ||
} |
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/view/OutputView.java
Outdated
if (participantName.equals(participant.getName())) { | ||
String participantResult = participant.getResult(); | ||
System.out.println(participantName + " : " + participantResult); | ||
} |
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.
indent(인덴트, 들여쓰기) depth를 2를 넘지 않도록 구현한다. 1까지만 허용한다.
이 조건을 충족시키기 위해서 스트림을 사용해보는 건 어떨까요?
저도 처음에 while
문 안에 if
로 이름 찾는 방법만 생각이 나서 고민했었는데 학습테스트를 통해 filter
사용법을 알게 돼서 고쳤어요!!
그리고 이름을 찾는 행위가 Participants
의 책임이라고 생각해서 저는 해당 메서드를 플레이어 모아놓은 클래스에 작성했는데 혜빈님은 어떻게 생각하시나요?
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.
제가 지금 우아한테크코스 프리코스를 진행 중인데요. 공동 피드백을 보면 "객체는 객체답게 사용한다."라는 부분이 있거든요?
객체는 객체답게 사용한다는 말이 getter setter만 있고 로직은 없는 객체가 있다면,
데이터를 꺼내(get) 사용하기보다는, 데이터가 가지고 있는 객체가 스스로 처리할 수 있도록 구조를 변경할 수는 없는 지를 한 번 생각해보라는 내용의 피드백이였는 데요.
상희님의 조언을 듣고 이 말이 갑자기 떠올랐어요. 사실 객체는 객체답게 쓰라는 말이 잘 이해가 안 갔는 데 이렇게 예시를 보고 나니까 get해서 쓰지말고 객체가 스스로 처리 하도록 해야한다는 게 무슨 말인지 알겠네요 :)
좋은 조언 진짜 너무 감사합니다!!
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.
그리고 stream의 filter기능을 사용하면 들여쓰기를 줄일 수 있다는 점은 또 몰랐네요!!
상희님의 조언대로 1) filter기능을 사용해 들여쓰기를 줄이고 2) 이름을 찾는 행위를 부여하는 아래와 같은 코드를 Participants에 넣어주었습니다!
public String getCertainParticipantResult(String participantName){
return participantInventory.stream()
.filter(participant -> participantName.equals(participant.getName()))
.map(Participant::getResult)
.findFirst()
.orElse(null);
}
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.
----- 제가 결과값을 Map으로 저장하면서 지금은 사용자이름을 키값으로 사용자의 결과를 불러오는 코드로 변경되었습니다.
for (int i = 0; i < points.size(); i++) { | ||
if (points.get(i) == true) System.out.print("-----|"); | ||
else if (points.get(i) == false) System.out.print(" |"); | ||
} |
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.
for
문 안에 있는 내용을 따로 메서드로 분리하면 else
를 안 써도 될 것 같은데 어떠신가용
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 outputLadderColumn(Boolean bool) {
if (bool == true) System.out.print("-----|");
if (bool == false) System.out.print(" |");
}
지금은 현재 for문 안의 내용을 이렇게 따로 메소드를 만들어 빼주었습니다!!
들여쓰기를 줄이는 데에 좋은 방법인 거 같아요 감사합니다 :)
<질문>
제가 이해를 잘 못 한 거 같은 데 else를 안 써도 될 거 같다는 말이 else if를 if로 바꾸는 게 좋을 거 같다는 말인가요?
아니면 다른 의미가 또 있는 건가요??
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.
else 예약어를 쓰지 않는다.
엇 다른 의미는 없고 이 규칙을 지키는 방향으로 수정했으면 좋겠다는 뜻이었습니다!
src/main/java/model/Ladder.java
Outdated
|
||
public void settingColumns(List<LadderRow> ladderRows) { | ||
this.ladderRows = ladderRows; | ||
} |
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
이 아닌 settingColumns
로 지으신 이유가 궁금합니다!
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.
이건 제 착오인 거 같습니다 ㅜㅠㅜㅠ
생성자가 없다는 걸 몰랐네요 짚어주셔서 감사합니다!!
현재는 생성자로 수정해두었습니다!1
|
||
public class DecideResult { | ||
private int moveColumn(List<Boolean> points, int columnPosition){ | ||
if(columnPosition!=0 && points.get(columnPosition-1)==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.
오! 이 부분에 대한 책임 부여가 저랑 조금 다른 부분인 것 같아요. 저는 조건문의 내용이 List points를 인스턴스 변수로 가진 (혜빈님 클래스 이름 기준) 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.
생각해보니까 이 상희님 말씀처럼 DecideResult를 따로 둘 필요가 없을 거 같네요!
저는 리팩토링을 통해 Ladder를 생성하고 Ladder에서 사용자의 결과값도 구할 수 있게
결과를 저장하는 메소드를 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.
전체적으로 아주 열심히 하려고 하신 부분이 너무 잘 보여서 정말 재밌게 봤습니다
고생 많으셨어요! 👍
답변 1
enum 을 잘 쓰셨는지 궁금하다
저는 enum 의 경우에는 저도 우테코때 저렇게 했었던 것 같은 기억이 나네요
그때 당시와 비교하면서 혜빈님 코드를 보면 저는 잘 썼다는 말씀을 드리고 싶은데요
저는 반대로 로직을 enum 에서는 빼는 부분이 오히려 좋을 수도 있다는 말씀을 드려보고 싶어요
우테코때 나왔던 이야기었는데요
enum 에 로직을 넣어야 하냐! 말아야 하냐! 로 크루들간에 토론이 열린 적이 있었는데요
그때 다른 크루들은 대부분 enum 에 로직이 있었으면 좋겠다, 저는 없었으면 좋겠다 파였는데요
우테코의 관점에서는 enum 에 로직을 넣는 것이 무조건 좋은 것은 맞고, 지금 미션 기준에서는 넣으려고 시도해보는 것도 좋을 것 같아요
하지만 제 개인적인 일하면서 해본 후기로는 안 넣는 것이 더 깔끔한 경우가 많다는 쪽인 것 같아요
정답은 없고, 고민해보신 부분이 너무 좋다는 말씀을 드리고 싶어요!
2번
저라면 Controller 에는 로직을 최소화 하고, Service 에 로직을 넣는 방향으로 코드를 바꿔볼 것 같아요
클래스를 분리하는 느낌인거죠
Controller 에 메소드에서 Service 의 Public method 를 호출한다 <- 이 부분은 꽤나 자연스럽게 느껴지지 않나요?
public 으로 바꾸는 것은 @VisibleForTesting
어노테이션을 붙혀서라도 테스트를 하고 싶으면 하는 편입니다
로직이 복잡하면 무조건 있는게 좋으니까요
2-2)
이 부분은 2-3쪽이랑 같이 설명드리는 방향이 더 좋을 것 같아요
Math.random()을 바로 쓰는 것이 아니라
interface LadderGenerator{
boolean generateRandom()
}
와 같은 인터페이스를 만들고, 하나는 진짜 LadderRandomGenerator 를 Main 에서 사용하고요
다른 하나는 LadderMockGenerator 라는 클래스를 테스트 코드에서만 사용하도록 할 것 같아요
Main 함수에서 Controller 를 생성할 때 저 LadderGenerator 를 받도록 하는 것을 통해서 우리가 원하는 방향으로 랜덤을 얻어낼 수 있을 것 같아요
이렇게 하면 테스트를 만들 수 있을 것 같고, 이 부분을 시도해보셔도 좋을 것 같아요
궁금한 것이 있거나 하시면 언제든 편하게 dm 주시고 여쭤봐주세요!
머지 요청 전까지 계속 봐드리겠습니다
src/main/java/Main.java
Outdated
@@ -0,0 +1,8 @@ | |||
import controller.Controller; | |||
|
|||
public class Main { |
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
과 같은 클래스 네이밍을 별로 좋아하지 않는데요
비슷한 계열로 Controller
클래스도 별로 좋아하지 않아요
그 이유는 import 를 했을 때 이름이 겹칠 클래스가 엄청 많아서인데요
그래서 약간 길더라도 LadderController
같은 네이밍을 쓰는 것을 좋아하는데요 이 부분은 어떠신지 궁금합니다!
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.
Controller
는 여러개 일 수 있으니까 네이밍이 겹칠 수 있다는 건 생각 못 해본 관점인 거 같아요!
현재 LadderController
로 네이밍 변경했습니다.
<질문>
Contoller
가 여러개 일 수 있다는 건 이해가 갑니다.
하지만 public static void main이 들어가는 Main은 1개만 존재하는 데도 이름이 겹칠 수 있다는 게 잘 이해가 가지 않습니다.
Main이 겹치는 경우는 어떤 경우인가요?
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 함수를 찾기가 힘든데요
전체 검색으로 main(으로 검색해도 엄청 많이 나와요
이때 검색할 수 있는 키워드 역할을 파일명이 하는데요
LadderApplication 같은 네이밍의 파일이 있다면 그걸로 검색하고 갈 수 있어서 편한 것 같아요
프로젝트의 규모가 커졌을 때부터 큰 의미를 갖는 네이밍인 것 같아요
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은 무조건 하나밖에 없으니까 왜 Main이 여러개가 나온다는 건 지 몰랐는 데 하나의 어플리케이션 안에 Ladder
나 Lotto
등 다양한 로직이 들어가면 main이 겹칠 수가 있군요!!
Main
을 LadderApplication
으로 변경해 주었습니다!!
src/main/java/view/InputView.java
Outdated
import java.util.Scanner; | ||
|
||
public class InputView { | ||
Scanner scanner = new Scanner(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.
다른 부분의 구현을 정말 잘 해주신 것 같아서
조금 디테일하게 챙길 수 있을 것 같을 부분을 말씀드리려고 하는데요
혹시 이 클래스가 Static method 로 이루어지지 않은 이유가 있을까요?
private static final Scanner scanner = new Scanner(System.in); 으로 바꾸면 현재 있는 것들을 전부 static 으로 바꿀 수 있을테니까요
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.
딱히 이유는 없습니다 ㅜㅠ
Controller
안에서 inputView
와 outputView
에 static
을 붙여야 한다는 개념은 생각해봤는 데 그 안의 Scanner
에도 static을 붙여줄 수 있다는 개념은 생각 못 해본 거 같아요! 짚어주셔서 감사합니다!!
현재 스캐너와 InputView
OutputView
의 모든 메서드에 static 을 붙여주었습니다!
<질문>
static을 붙이는 이유가 "이 값은 객체마다의 특징을 나타내는 값이 아니다"라는 특징을 명시해주기 위해서 인가요?
아니면 또 다른 이유가 있는 건가요?
미리 값들을 한 번에 불러온다는 게 좋은 건지 잘 모르겠습니다 ㅜㅠ
미리 불러오더라도 그 값을 안 쓰게 되면 괜한 메모리 낭비가 아닌가 하는 생각이 있기 때문입니다.
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.
보통 스태틱으로 할당되는 메모리는 신경쓸 필요가 없어요 (map 에 10만개쯤 넣는 순간부터 고민해도 충분합니다)
그래서 그 이유는 아니고요
스태틱을 쓰면 가장 간단하게는 생성자가 간단해지잖아요?
테스트에서 그 객체를 다룰 필요가 없을 때는 생성자가 간단해져서 테스트가 오히려 쉬워지기도 합니다
물론 수기 구현으로 테스트에서 모킹된 객체를 넣는다면 그때부터는 static이 아닌 쪽이 편합니다
모든 것에는 장단이 있어요!
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.
staic이라는 게 생각보다 메모리를 많이 잡아먹진 않군요 :) 몰랐던 지식인 데 알려주셔서 감사합니다!
생성자가 간단해진다는 생각은 못 해봤어요!! 생각해보지 못 한 관점이네요.
그런데 꼭 static이 아니여도 아래와 같이 선언해두면 생성자가 간단해지지 않나요?
static으로 변수를 미리 선언해주는 것과 아래와 같이 변수를 설정해주는 것의 차이가 모호하게 느껴져요...
private String name = "아무개";
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.
만약 그렇게 된다면 큰 차이는 없을 것 같아요!
Java 기준에서는 Lombok 으로 생성자를 만드는 경우가 많은데, 이때 기준으로 봤을 때 @RequiredArgsConstructor
를 쓰지 못한다는 것이 약간 아쉬울 수 있는데, 이 부분만 알고서 쓰신다면 문제는 없을 것 같아요
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.
@RequiredArgsConstructor
는 처음 들어 봤는 데 생성자를 초기화 해주는 기능이군요:)
static 변수는 생성자와 관련이 없어서 @RequiredArgsConstructor
로 처리할 수 없다는 점은 이번 기회로 잘 인지하고 있겠습니다!! 감사합니다!!
README.md
Outdated
# < controller > | ||
## 1) Controller | ||
* createPlayers() //참여자 생성기능 | ||
* createResults() //참여결과 생성기능 | ||
* createLadder() //사다리 생성기능 | ||
* createLadderResult() //사다리 결과 생성기능 | ||
* playLadderGame() //사다리 게임 실행기능 |
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.
저도 이번 문제를 푸면서 이 부분에 대한 고민이 많았습니다 ㅜㅠ
메소드는 계속 생성 수정 삭제가 되니까 README 관리가 너무 어렵더라구요.
좋은 조언감사합니다!! 앞으로는 구현한 기능에 대해 README를 작성하는 습관을 들여봐야겠어요!!
README 작성을 완료했는 데 여기서 더 자세히 적어야하는 지 덜 적어야하는 지는 감이 잘 안 잡히네요 ㅎㅎㅎ하하....
핵심 기능은 다 적어보려고 노렸했는 데 피드백 부탁드립니다!!
InputView inputView = new InputView(); | ||
Splitter splitter = new Splitter(); | ||
OutputView outputView = new OutputView(); |
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.
Input VIew 쪽에 달아드린 이 클래스가 static 으로 사용하지 않는 이유를 알고 싶어요!
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 Players createPlayers() { | ||
String playerNamesBeforeSplit = inputView.inputPlayers(); | ||
String[] playerNames = splitter.split(playerNamesBeforeSplit); |
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.
이 부분은 view 로 옮겨가면 좋을 것 같아요!
split 하는 부분이 controller에 있게 되면 점점 controller 의 복잡도가 올라갈 것 같아요
|
||
import java.util.List; | ||
|
||
public class 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.
일급 컬렉션을 지키려고 노력하신 부분이 보여서 고생 많으셨다는 생각이 드네요
이 클래스의 경우에는 method 가 오직 getter 하나 뿐인데요 이렇게 되었을 경우에 일급 컬렉션의 역할을 하는 것 대신 클래스의 숫자가 늘어나서 코드를 읽기 힘들어질 수도 있을 것 같아요!
저는 개인적으로는 여기에 player 숫자에 대한 검증과 같은 무엇인가가 없다면 이 클래스는 없어도 된다는 생각입니다!
|
||
import java.util.Map; | ||
|
||
public class PlayerResults { |
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.
바로 전에 말씀드린 Players 와는 반대로 이 클래스는 충분히 있어도 된다고 생각하는 편이긴 한데요
dto 의 경우에는 직접 Map<String, String> 형태로 넘기게 되면 이 클래스는 뭘 의미하는거지? 라는 것을 헷갈려 할 수도 있을 것 같아요
그래서 dto 에 경우에는 예외적으로 아무 메소드가 없어도 클래스로 만드는 것도 나쁘지 않다고 생각하는 편입니다!
} | ||
} | ||
|
||
private LadderPoint randomTrueOrFalse() { |
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.
이 메소드의 경우에 randomTrueOrFalse 라는 네이밍을 봤을 때는 뭔가 boolean 타입을 반환해야 할 것 같은데 어떻게 생각하시나요?
private void outputResults(List<String> trialResults) { | ||
System.out.println(); | ||
for (String trialResult : trialResults) { | ||
System.out.printf("%" + COLUMN_LENGTH + "s", trialResult); |
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.
와... 이렇게 쓸 수 있다는 것을 저도 처음봤어요 신기하네요!
java 쪽에서는 printf 를 활용한 코드를 일반적으로는 많이 사용하지 않는 것 같은데요
제가 본 방법들은 보통 StringBuilder 를 통해서
StringBuilder result=new StrintBuilder();
for(int i=0;i<COLUMN_LENGTH;i++){
result.append("-")
}
약간 이런 느낌으로 해도 좋을 것 같은데 어떠신가요?
일반적으로 많이 쓰는 형태가 아니라서 신선했으면서도 살짝 어색하게 느껴져서 달아봅니다!
if (playerResult == null) { | ||
System.out.println("존재하지 않는 사용자입니다."); | ||
} else if (playerResult != null) { | ||
System.out.println(playerName + " : " + playerResult); | ||
} |
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.
if (playerResult == null) { | |
System.out.println("존재하지 않는 사용자입니다."); | |
} else if (playerResult != null) { | |
System.out.println(playerName + " : " + playerResult); | |
} | |
if (playerResult == null) { | |
System.out.println("존재하지 않는 사용자입니다."); | |
return | |
} | |
System.out.println(playerName + " : " + playerResult); |
개인 취향은 이렇게 쓰는 편인데 어떻게 생각하시나요?
상희, 은우 리뷰어님 이번에 만나뵙게 되어 정말 반갑습니다 :)
잘 부탁드립니다!!☘️
궁금한 점:
1)
저는 이번 미션에서
enum
을LadderMove(왼쪽, 오른쪽, 밑으로)
,LadderPoint(연결o, 연결x)
이렇게 2개 두었습니다.제가 enum을 써본 경험이 많지 않아서 사실 써놓고도 잘 쓴 지 잘 모르겠어요ㅜㅠ
enum부분 잘 활용한 건 지 궁금합니다! 그리고 이번 미션에서 enum을 더 사용할 수 있을 만한 부분이 있는 지,
제가 만든 enum에 조금 더 추가해볼만한 필드나 메서드가 있는 지 궁금합니다!
2)
저는 내부 상태를 외부에서 직접적으로 변경하지 못하도록 Controller에 사용하지 않는 기능들은 전부 private처리를 하는 편입니다.
아무래도 테스트 코드를 작성하려면 public 처리된 함수만 단위 테스트를 할 수 있잖아요.
랜덤과 관련된 함수는 랜덤값이니까 테스트를 하지 못 하고 private처리된 함수는 클래스 밖에서 사용을 할 수 없으니까 테스트를 하지 못 해서 빼고 나니 결과적으로 이번에는 2개의 테스트 코드밖에 생성하지 못 했습니다.
2-1)
테스트를 위해 private함수를 public으로 변경하는 것이 나을까요? 아니면 테스트를 못 하더라도 private함수로 두는 것이 나을까요?
2-2)
리뷰어님이 보시기에 지금 코드에서도 가능할 거 같은데 제가 놓친 테스트 부분이 있다면 알려주세요 ㅜㅠ
2-3)
랜덤값 테스트를 할 수 있는 방법은 없을까요?