[자동차 경주] 표현록 미션 제출합니다.#3
[자동차 경주] 표현록 미션 제출합니다.#3Regyung wants to merge 6 commits intoJava-JavaScript-Language-Stuty:mainfrom
Conversation
- Car 객체 생성 - 입출력 기능 - 문자열 슬라이싱 - 우승자 구분
- 자동차 이름이 비어있을 때 - 자동차 이름이 하나거나 쉼표가 없을 때 - 자동차 이름이 5자를 초과할 때 - 횟수 입력이 잘못 되었을 때
- 기본적인 기능 테스트 - 자동차 이름이 비어있을 때 - 횟수가 비어있을 때 - 자동자 이름들 중 공란이 있을 때 - 횟수가 오버플로우 됐을 때 - 횟수가 양수가 아닐 때
- 실행 결과 출력 코드를 기능의 사용처를 고려해 Controller에서 View로 이동 - 기존 View에서 불필요했던 코드 수정
seulnan
left a comment
There was a problem hiding this comment.
너무 고생하셨습니다:) 갓현록...그저 갓..
일단 코드를 보면서 많이 생각하신 게 보여서 좋았습니다
많은 예외를 생각하신 점, 함수를 최대한 분리하신 점이 보이네용👏🏻👏🏻 저도 리뷰하면서 많이 배워갑니다!
추가로 feat(ALL) 대신 변경사항 단위로 하나하나 쪼개서 커밋하시면 더더욱 이해가 될 것 같아요ㅎㅎ
| } | ||
|
|
||
| public void forward() { | ||
| if (Randoms.pickNumberInRange(0, 9) >= 4) { |
There was a problem hiding this comment.
자동차 이동로직은 자동자 객체에 포함시키기보다 비즈니스 로직이니 service에서 처리하는 것은 어떨까용?
There was a problem hiding this comment.
그리고 상수도 적극적으로 활용하면 유지보수성면에서 더 좋아질 듯 해용
There was a problem hiding this comment.
저는 개인적으로 자동차 이동 로직이 자동차 객체에 있어도 된다고 생각해요! 자동차의 이동 자체는 프로그램의 비지니스 로직이라기 보다는 자동차라는 데이터에 내재된 비지니스 로직, 즉 행동이니 모델에 있는게 더 적절하다고 볼 수 있는 부분도 충분히 있다고 생각합니다
| return Console.readLine(); | ||
| } | ||
|
|
||
| public static void interimTally(List<Car> c) { |
There was a problem hiding this comment.
view는 사용자에게 데이터를 입력받고 출력하는 용도로만 사용하는 게 좋을 것 같아서 이 비즈니스로직은 service로 분리하는 것이 좋아보여용
There was a problem hiding this comment.
뷰와 모델 사이에 직접적인 의존성을 갖는 것이 유지보수성 등을 이유로 MVC 패턴에는 바람직하지 않다고 생각합니다.
| import java.util.List; | ||
|
|
||
| public class Car { | ||
| public static List<Car> cars = new ArrayList<>(); |
There was a problem hiding this comment.
cars 라는 리스트가 static으로 선언되면
모든 car객체가 동일한 cars list를 참조가 가능해짐 -> 데이터가 한곳에서만 관리되지않고 여러군데에서 수정될 위험이 커보여요!
데이터전송을 위한 단순 데이터객체라면 DTO에서 해야하지만, carlist를 관리하는 로직을 같이 분리해서 service에 넣는 것은 어떨까요?
There was a problem hiding this comment.
동의합니다! 메서드를 이용해서 수정하도록 해서 캡슐화하는 것이 좋아보이네용
final 키워드를 통해서 name을 설정하는 것도 좋아보입니당
| if (s.isEmpty()) throw new IllegalArgumentException("횟수값이 비어있습니다."); | ||
|
|
||
| try { | ||
| Integer.parseInt(s); |
There was a problem hiding this comment.
Integer.parseInt(s); 가 너무 불필요하게 중복호출되는 것 같아용!
변환값을 변수에 저장하고 이를 재사용하는 방식으로 바꾸면 좋을 것 같아요
There was a problem hiding this comment.
추가로 checkNumber에 대한 예외처리를 좀 분리하는 건 어떨까용?
parseInt를 아예 분리하면서 숫자가 아니거나 너무 큰 숫자에 대한 예외를 처리하는것도 분리하면 좋을 것 같아용!
There was a problem hiding this comment.
Integer.parseInt(s);가 너무 불필요하게 중복호출되는 것 같아용! 변환값을 변수에 저장하고 이를 재사용하는 방식으로 바꾸면 좋을 것 같아요
동의합니다.
There was a problem hiding this comment.
추가로 checkNumber에 대한 예외처리를 좀 분리하는 건 어떨까용? parseInt를 아예 분리하면서 숫자가 아니거나 너무 큰 숫자에 대한 예외를 처리하는것도 분리하면 좋을 것 같아용!
동의합니다. 어떤 예외를 처리하는지 메소드명에 추가해서 분리하면 가독성이 증가할 것 같은데 어떻게 생각하시나요? @Regyung
BYEONGHWALEE-dev
left a comment
There was a problem hiding this comment.
MVC 모델을 잘 넣으셔서 많이 배워갑니다. Service 레이어도 잘 몰랐는데 어떤 기능인지 알고 갑니다.
| if (s.isEmpty()) throw new IllegalArgumentException("횟수값이 비어있습니다."); | ||
|
|
||
| try { | ||
| Integer.parseInt(s); |
There was a problem hiding this comment.
Integer.parseInt(s);가 너무 불필요하게 중복호출되는 것 같아용! 변환값을 변수에 저장하고 이를 재사용하는 방식으로 바꾸면 좋을 것 같아요
동의합니다.
|
|
||
| </details> | ||
|
|
||
| ## 구현할 기능 목록 |
There was a problem hiding this comment.
구현할 기능 목록에 미리 변수와 메소드명을 미리 구성하는 것도 정리에 도움이 많이 될 것 같네요
메소드의 길이가 길어져서 메소드를 더 분리한다던가 할 땐 어떻게 하시나요?
| import java.util.List; | ||
|
|
||
| public class Car { | ||
| public static List<Car> cars = new ArrayList<>(); |
There was a problem hiding this comment.
동의합니다! 메서드를 이용해서 수정하도록 해서 캡슐화하는 것이 좋아보이네용
final 키워드를 통해서 name을 설정하는 것도 좋아보입니당
| if (s.isEmpty()) throw new IllegalArgumentException("횟수값이 비어있습니다."); | ||
|
|
||
| try { | ||
| Integer.parseInt(s); |
There was a problem hiding this comment.
추가로 checkNumber에 대한 예외처리를 좀 분리하는 건 어떨까용? parseInt를 아예 분리하면서 숫자가 아니거나 너무 큰 숫자에 대한 예외를 처리하는것도 분리하면 좋을 것 같아용!
동의합니다. 어떤 예외를 처리하는지 메소드명에 추가해서 분리하면 가독성이 증가할 것 같은데 어떻게 생각하시나요? @Regyung
|
|
||
| String name; | ||
| int count; | ||
|
|
There was a problem hiding this comment.
개인적으로 Car 객체 속 변수들은 private 접근 지정자를 사용하는게 어떨까 싶습니다
| public static void outputWinner(String winner) { | ||
| System.out.println("최종 우승자 : " + winner); | ||
| } | ||
| } |
There was a problem hiding this comment.
저도 잘 모르는 입장에서 input output 뷰 파일 나누면 더 좋을 것 같아요.....
There was a problem hiding this comment.
저도 이번에 처음 MVC 패턴을 도입해서 Input과 Output을 나누어서 하긴 하였는데 Racing Game과 같은 소규모는 굳이 나눌 필요성을 잘 못느끼겠더라고요 저는 이정도의 규모의 코드는 View안에 전부 구현해도 될 것 같습니당
| import java.util.List; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class GetWinner { |
| public static List<String> slice(String s) { | ||
| return Arrays.asList(s.split(",")); | ||
| } | ||
| } |
There was a problem hiding this comment.
서비스 파일을 너무 쪼개놓은것 같다고 생각이 드는데,,, 제가 아직 잘 몰라서 그렇게 생각하는걸수도 있어요,,! 쪼개는게 더 좋은 방식이라면 알려주시면 감사하겠습니다!
KIM-GOING
left a comment
There was a problem hiding this comment.
기능을 최대한 분리하려고 노력하신 것이 보입니다! 고생하셨어요:)
|
|
||
| String name; | ||
| int count; | ||
|
|
| import java.util.List; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class GetWinner { |
| - outputResult() | ||
| - output() | ||
| ### Controller | ||
| - startRace() No newline at end of file |
There was a problem hiding this comment.
저는 readme에 구현할 기능 목록을 글로만 작성했는데 메소드명과 함께 적는 것을 배워가요!
| import java.util.List; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class GetWinner { |
신경써서 구현한 부분
피드백이 필요한 부분
고려한 예외