-
Notifications
You must be signed in to change notification settings - Fork 55
[이관우_BackEnd] 1주차 과제 제출합니다. #63
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: main
Are you sure you want to change the base?
Conversation
|
고생 많으셨습니다!
|
|
|
||
| import java.util.List; | ||
|
|
||
|
|
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.
11번째 줄 공백을 말하신 건가요? 수정하겠습니다
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.
13, 14번째 줄을 얘기한거에요!
보통 두 줄 사이의 간격은 한 칸으로 유지해주세요!
| private final OutputView outputView; | ||
| private final RaceManager raceManager; | ||
|
|
||
|
|
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.
영역을 나누는 것도 사이에 한 칸만 있으면 충분해 보입니다..!!
| outputView.getCars(); | ||
| String racerStr = inputView.getStringInput(); | ||
| List<Car> cars = StringUtils.makeCarsUsingString(racerStr); | ||
| for(var c : cars) CheckCarName.checkName(c.getName()); |
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.
검증 단계를 어느 위치에서 하는 것이 좋을까 고민을 많이 했었는데,
생각해보니 말씀해주신 것처럼 객체 생성 시점에 바로 검증하는 것이 좋을 것 같습니다
public Car(String name) {
CheckCarName.checkName(name);
this.name = name;
this.location = 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.
👍👍👍
| int location; | ||
| 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.
접근제어자가 빠진 것 같아요!
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.
앗 수정하겠습니다!
| package racingcar.model; | ||
|
|
||
| public class Car { | ||
| private static final int CAN_MOVE_STANDARD = 4; |
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을 붙인 이유가 있으실까요??
만약 무의식적으로 붙이셨다면 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는, 자동차가 움직이는 기준 값을 외부에 공개할 필요가 없기 때문에 private를 사용하였고,
인스턴스 생성 때마다 CAN_MOVE_STANDARD 변수를 생성할 필요는 없다고 판단하여서, 클래스 내에서 공유할 수 있도록 static을 사용하였습니다.
| @@ -0,0 +1,26 @@ | |||
| package racingcar.util; | |||
|
|
|||
| public class CheckCarName { | |||
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.
Check 라는 이름보다는 Validator가 더 적합해보여요.
관우님은 어떻게 생각하시나요?!
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.
유효성을 검증하는 클래스의 마땅한 이름이 생각이 안나서 Check 를 사용했는데,
Validator 라고 이름 짓는게 더 명확해 보이는 것 같습니다. 수정하겠습니다
| if(name == null) | ||
| throw new IllegalArgumentException("이름에는 null 값이 들어갈 수 없습니다."); | ||
| } | ||
|
|
||
| public static void isBlank(String name) { | ||
| if(name.isBlank()) | ||
| throw new IllegalArgumentException("이름은 공백일 수 없습니다."); | ||
| } | ||
|
|
||
| public static void isMoreThanMaxLength(String name) { | ||
| if(name.length() > NAME_MAX_LENGTH) | ||
| throw new IllegalArgumentException("이름은 5자를 넘을 수 없습니다."); |
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.
예외 발생 시 작성하는 메시지도 결국 매직 넘버라 상수화하거나 따로 ErrorMessage와 같은 이름의 Enum으로 분리할 수 있지 않을까요?
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.
이번 과제처럼 규모가 크지 않은 프로젝트에서는 Enum으로 분리해서 예외를 관리해도 괜찮겠다 생각했지만,
규모가 큰 프로젝트에서는 Enum으로 관리할 예외들이 너무 많아져서 관리하기 힘들어진다는 이야기를 들었던 것 같습니다.
때문에, Enum으로 예외를 관리하는 방법도 정말 좋은 방법이라 생각되지만,
Enum을 사용하지 않고 예외 처리를 할 수 있는 방법에 대해 생각하던 중에 우선 이렇게 예외 처리를 설계해봤습니다.
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.
생각해보니 Enum에서 관리한다면 ErrorMessage.OVER_NAME_LENGTH.getMessage() 와 같이 코드가 길어질 수 있겠네요..!
그렇다면 예외 메시지를 관리하는 클래스를 만드는 것이 더 깔끔할 것 같습니당
| public List<String> findWinners(List<Car> cars) { | ||
| int maxLocation = 0; | ||
|
|
||
| for(var c : cars) { | ||
| if(maxLocation < c.getLocation()) maxLocation = c.getLocation(); | ||
| } | ||
|
|
||
| List<String> winners = new ArrayList<>(); | ||
| for(var c : cars) { | ||
| if(maxLocation == c.getLocation()) | ||
| winners.add(c.getName()); | ||
| } | ||
|
|
||
| return winners; | ||
| } |
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.
이런 부분은 확실히 스트림으로 해야 직관적이고 가독성도 좋을 것 같습니다
스트림이 아직 익숙하지 않아서 우선 이렇게 만들었는데,
이번 리팩토링 과제에서 좀 더 학습하여 스트림에 익숙해져 보도록 하겠습니다
|
|
||
| import java.util.List; | ||
|
|
||
|
|
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 RacingController() { | ||
| this.inputView = new InputView(); | ||
| this.outputView = new OutputView(); | ||
| this.raceManager = new RaceManager(); | ||
| } |
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.
생성자 내부에서 new를 통해 만든 객체를 주입해준다면, 나중에 어떠한 요구사항이 변경되었을 때 주입해야 할 객체의 수정이 어려울 것 같아요.
생성자에 인자를 추가하여 외부에서 주입받도록 하는 방식은 어떤가요?
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 RacingController() { | ||
| this.inputView = new 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.
추가로 여기 new 쪽에 공백 하나가 더 있어요!
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.
헐 정말 꼼꼼하게 봐주시네요...! 수정하겠습니다
|
자동차 경주 게임 리팩토링 완료 했습니다. 리뷰 및 피드백 해주신 것을 반영하여, 아래 사항들을 중점으로 리팩토링 하였습니다.
그리고 리팩토링 하면서 추가로 고민이 됐던 부분이 있었습니다.
더 개선할 부분이 있다면 피드백 부탁드리겠습니다...! 감사합니다 |
|
이번 주차도 고생하셨습니다~ |
| outputView.getCars(); | ||
| String racerStr = inputView.getStringInput(); | ||
|
|
||
| outputView.getTryCount(); | ||
| String tryCnt = inputView.getStringInput(); | ||
| TryCountValidator.checkTryCount(tryCnt); | ||
| int tryCount = Integer.parseInt(tryCnt); |
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.
RacingController에서 자동차 이름, 시도 횟수를 입력받는 메소드를 보시면 한 번에 두 입력을 받도록 하고 있어요. 이 부분은 분리가 가능하지 않을까요?
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 String getCarNames() {
outputView.getCars();
String raceStr = inputView.getStringInput();
...
}
public String getTryCount() {
outputView.getTryCount();
String raceStr = inputView.getStringInput();
...
}
이렇게 한 다음 데이터를 처리하는 메소드를 아래 처럼 만들어서,
public RaceInfoResponse readyForRace(String carStrs, String tryCount) {
TryCountValidator.isPositive(tryCnt);
List carNames = StringUtils.splitByComma(carNames);
}
이렇게 작성하는 방식은 어떤가요??
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.
좋은거 같아요!
그리고 입력 받는 메소드 명도 get~~ 대신 다른 단어로 바꿔보는 것도 좋을 것 같아요
|
|
||
| List<String> carNames = StringUtils.splitByComma(raceInfoInput.carNames()); | ||
|
|
||
| return new RaceInfoResponse(carNames, tryCount); |
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.
new로 객체를 생성하는 것도 좋지만 정적 팩토리 메소드를 적용해보시면 가독성 측면에서 더 좋아질 것 같아요!
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.
보고서에서 첨부해 주신 링크 잘 읽었습니다
명확하게 알고 있지는 못했던 개념인데, 기존의 생성자 호출 방식에서 of, from 등을 활용하는 방식으로 다시 리팩토링 해보겠습니다..!
| return new RaceInfoResponse(carNames, tryCount); | ||
| } | ||
|
|
||
| public void raceStart(RaceInfoResponse raceInfo) { |
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.
raceStart() 메소드는 경주 시작을 의미하니 자동차의 움직임을 담당하는 책임을 지니고 있어요.
이 곳에서 자동차 리스트를 만들고, 우승자를 구하는 책임이 가중되는 것은 아닐까요?
하나의 메소드 호출만으로 모든 과정을 마치도록 하는 것이 아니라 메인 메소드에서 컨트롤러를 통해 입력, 자동차 객체 생성, 경주, 우승자 구하기 메소드를 각각 호출하는 형태에 대해서는 어떻게 생각하시나요??
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 List<String> findWinners(List<Car> cars) { | ||
| int maxLocation = cars.stream().mapToInt(Car::getLocation).max().orElse(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.
메소드 체이닝은 주로 두 번째부터 줄바꿈을 사용해주고 있어요.
아래 예시 참고 해주세요
| int maxLocation = cars.stream().mapToInt(Car::getLocation).max().orElse(0); | |
| int maxLocation = cars.stream() | |
| .mapToInt(Car::getLocation) | |
| .max() | |
| .orElse(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.
앗 참고하겠습니다..
| public List<String> findWinners(List<Car> cars) { | ||
| int maxLocation = cars.stream().mapToInt(Car::getLocation).max().orElse(0); | ||
|
|
||
| List<String> winners = cars.stream().filter(c -> c.getLocation() == maxLocation).map(Car::getName).toList(); |
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 Arrays.stream(str.split(",")).toList(); | ||
| } | ||
|
|
||
| public static String NumToSticks(int count) { |
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,21 @@ | |||
| package racingcar.util; | |||
|
|
|||
| public class TryCountValidator { | |||
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.
TryCount 라는 일급 객체를 만들고 Car처럼 생성자에서 입력받은 값의 유효성을 검증하는 것에 대해서는 어떻게 생각하시나요?
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.
보고서에서 언급해 주셨듯이, 말씀해주신 방법으로 일급 객체를 만든 다음 생성자 부분에서 유효성 검증을 옮기는 방법을 시도해 봐야겠습니다.
그렇게 하면 RacingController에 있던 유효성 관련 내용들을 일관성 있게 tryCount, Car 모두 생성자에 옮길 수 있을 것 같습니다
| import java.util.List; | ||
|
|
||
| public class OutputView { | ||
| public void getCars() { |
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.
이 메소드 이름은 자동차 리스트를 가져오는? 의미로 볼 수 있을 것 같아요.
출력문만 있다면 get~~보다는 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.
맞습니다 출력에 관련된 내용이니 get 보다는 print 가 어울릴 것 같습니다 수정하겠습니다!
| throw new IllegalArgumentException("이름은 5자를 넘을 수 없습니다."); | ||
| } | ||
|
|
||
| public static void checkName(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.
check~~ 라는 네이밍은 그저 값을 '확인'하기만 하는 의미로 많이 쓰여 값이 유효한지 검증하는 의미의 validate~~를 사용하는 것은 어떠세요??
비슷한 내용의 글 공유드립니다.
자료
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.
비슷한 의미를 가지는 것처럼 보여도 디테일한 차이가 있었군요..!
말씀해주신 것처럼 유효성 검증이라는 의미에 맞게 validate 로 수정하도록 하겠습니다
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.
check는 해석하는 방식에 따라 의미가 크게 바뀔 수 있어서 웬만해서는 사용을 지양하는 편으로 알고있어요!
안녕하세요, 백엔드 비기너 이관우 입니다.
성장할 수 있는 기회를 이렇게 마련해주셔서 감사드리고,
많은 것을 배우고 크게 발전할 수 있는 한 학기가 될 수 있도록 노력하겠습니다.
이번 과제를 진행하면서 들었던 주요한 고민은 다음과 같은 것들이 있었습니다.
위의 사항들에 대해서는 좀 더 고민해보고,
다음 주차에 예정된 OOP 교육을 듣고난 후 개선해볼 수 있도록 하겠습니다.
감사합니다!