-
Notifications
You must be signed in to change notification settings - Fork 56
[BCSD Lab] 이동훈 4차시 미션제출합니다. #77
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
base: dh2906
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.
마지막 미션까지 수고하셨습니다! 정말 시간 빠르네요...
너무 잘 작성해주셔서 코멘트 드릴게 별로 없습니다...!
그동안 리뷰 해주셔서 정말 감사했습니다! 많이 배우고 갑니다~!
src/test/java/model/PlayerTest.java
Outdated
.extracting("message") | ||
.isEqualTo(ErrorMessage.NAME_LENGTH.getMessage()); | ||
} | ||
} |
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, Line, Player 테스트에도 없어요!)
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.
엇 원래는 Intellij에서 자동으로 해주던데 뭔가 설정이 잘못 되었나 보네요....
src/main/java/view/OutputView.java
Outdated
System.out.println(); | ||
|
||
int index = players.indexOf(playerName); | ||
int position = ladder.move(index); | ||
String prize = prizes.getPrizes().get(position).getPrize(); | ||
|
||
System.out.println("실행결과"); |
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.println(); | |
int index = players.indexOf(playerName); | |
int position = ladder.move(index); | |
String prize = prizes.getPrizes().get(position).getPrize(); | |
System.out.println("실행결과"); | |
int index = players.indexOf(playerName); | |
int position = ladder.move(index); | |
String prize = prizes.getPrizes().get(position).getPrize(); | |
System.out.println("\n실행결과"); |
InputView처럼 이런식으로 하면 통일성이 있을 것 같아요!
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 SizeValidator { | ||
|
||
private static final int ZERO = 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.
👍
import model.Height; | ||
import model.Ladder; | ||
import model.LadderGenerator; | ||
import model.Players; | ||
import model.Prizes; | ||
import model.Width; |
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.
모델의 거의 모든 파일을 import 하고 있으니 이런식으로 바꿔도 좋을 것 같아요!
import model.Height; | |
import model.Ladder; | |
import model.LadderGenerator; | |
import model.Players; | |
import model.Prizes; | |
import model.Width; | |
import model.*; |
public class Ladder { | ||
|
||
private final Height height; | ||
private final List<Line> lines; |
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을 총괄하는 Lines 일급 컬렉션을 만들어도 좋을 것 같아요!
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.
좋은 의견 감사합니다!
this.outputView = new OutputView(); | ||
this.inputView = new InputView(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.
View 같은 경우는 굳이 필드로 정의하지 않고 클래스명으로 불러서 사용할 때도 많더라구요! 동훈님은 어떻게 생각하시나요?
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는 객체 생성 시 스트림을 받아야 하는데, 테스트 코드 작성 시 직접 입력 스트림을 주입하여 키보드로 입력받을 필요가 없다는 장점이 있어서 위와 같이 사용했습니다!
private void printResultByViewerName(Ladder ladder, Players players, Prizes prizes) { | ||
String resultViewerName = inputView.inputResultViewerName(); | ||
|
||
if (resultViewerName.equalsIgnoreCase("all")) { |
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.
이 부분도 상수화 할 수 있을 것 같습니다!
NOT_INTEGER_FORMAT("int 타입 정수가 아닌 값이 입력되었습니다."), | ||
NEGATIVE_NUMBER("음수가 입력되었습니다."), | ||
EMPTY_NAME("이름이 비어있습니다."), | ||
NAME_LENGTH("이름이 5글자를 넘었습니다."), | ||
PLAYER_NAMES_EMPTY("참여자가 존재하지 않습니다"), | ||
PRIZE_EMPTY("당첨 결과가 비어있습니다."), | ||
PRIZES_EMPTY("당첨 결과 리스트가 비어있습니다."), | ||
PLAYER_NOT_FOUND("해당 플레이어를 찾을 수 없습니다."), | ||
NULL_INPUT_STRING("입력 문자열로 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.
👍
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.
짧은 시간동안 고생하셨습니다
몇가지 질문 대답해주세요!
이제 코인 개발합시다!!!
this.inputView = new InputView(new Scanner(System.in)); | ||
} | ||
|
||
public void run() { |
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.
run()
메서드가 확실히 깔끔해진것 같네요
메서드 분리 잘하신것 같아요 👍
private final int height; | ||
|
||
public Height(int height) { | ||
SizeValidator.validateNotNegative(height); |
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.
Height
와 Width
에서 음수 값인지 검증하는 로직이 중복되는 일이 발생하여 분리했습니다.
제 생각으로는 중복 로직의 최소화할 수 있으며, 유효성 검증의 요구 사항이 변경될 경우 해당 클래스에서만 코드를 수정하면 된다는 장점이 존재합니다!
|
||
import java.util.List; | ||
|
||
public class 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.
Model
에서 출력의 역할도 하고 있는것 같은데 이러면 변경이 생겼을때 수정이 어려워질것 같은데 어떻게 생각하시나요??
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.
저도 이 부분에 대해 고민을 했습니다. Model
에서 바로 출력하는 것이 아닌 출력을 하기 위한 문자열을 반환하도록 하는 것은 어떨까 하는 생각도 해봤지만 역시 LineView
라는 클래스로 분리하는 것이 더 맞는 것 같네요!!
public List<Boolean> getConnections() { | ||
return connections; | ||
} |
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<Boolean>
을 받아오는데 그대로 반환하는 경우가 궁금해용
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.
해당 부분은 기능 자체에서는 사용하지 않지만 테스트 코드를 작성하면서 필요하기에 추가했습니다.
그대로 반환하게 된다면 값이 외부에서 수정될 우려가 존재할 수 있겠네요.... 그렇다면 새로운 리스트에 connections
에 있는 요소를 복사해서 반환하도록 해야 할 것 같습니다.
src/main/java/model/Player.java
Outdated
validate(name); | ||
|
||
this.name = 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.
요정도는 붙여도 될것 같아요 😄
src/main/java/model/Players.java
Outdated
this.players = init(playerNames); | ||
} | ||
|
||
public void validate(List<String> playerNames) { |
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.
validateEmpty
같은 좀 더 확실한 이름이면 좋을것 같아요 !
public InputView(Scanner sc) { | ||
this.sc = sc; | ||
} |
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.
Scanner
는 객체 생성 시 입력 스트림을 인자로 전달해야 하는데, 내부에 생성하고 사용하게 된다면 입력에 대한 테스트 코드를 작성할 수 없게 됩니다. 그렇기에 외부에서 주입받도록 하면 Scanner
객체 생성 시 ByteArrayInputStream
라는 입력 스트림을 주입해줄 수 있어 키보드로 입력 받을 필요 없이 String
참조 변수에 테스트하고 싶은 문자열을 저장하고 해당 문자열로 테스트할 수 있게 된다는 장점이 있습니다!
벌써 초록 스터디의 마지막 미션이네요.
시간 참 빠른 것 같습니다 ㅋㅋ....
테스트 코드를 작성하면서 컨트롤러에 대한 테스트도 작성하고 싶었으나
Mock
을 도입하지 않으면 도저히 완성할 수 없을 것 같아 테스트가 굳이 필요없는 모듈을 제외한 각 모듈의 메소드에 대해 테스트를 작성했습니다.다들 고생 많으셨습니다!!!