-
Notifications
You must be signed in to change notification settings - Fork 4
[자동차 경주] 박시영 미션 제출합니다. #2
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
holyPigeon
left a comment
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/racingcar/View.java
Outdated
| void printResult(Model model){ | ||
| System.out.println("\n실행 결과"); | ||
| while (model.numberOfAttempts --> 0) { | ||
| System.out.println(model.race()); | ||
| } | ||
| } |
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.
MVC에서는 Controller 에서만 Model과 View가 만나고 View 코드 안에서는 Model과 관련된 부분이 웬만하면 없는 게 맞아서 사소하지만 model 객체가 아닌 model.race()를 직접 넣는 게 더 좋을 듯 합니다!
src/main/java/racingcar/Model.java
Outdated
| len = new int[st.countTokens()]; | ||
| for(int i=0; i<car.length; i++){ | ||
| String str = st.nextToken(); | ||
| if(str.length()>5) 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.
사소한 거긴 한데 if 문은 한 줄이라도 괄호를 쳐주는 게 좋긴 합니다!
| try { | ||
| numberOfAttempts = Integer.parseInt(input); | ||
| } catch (Exception e){ | ||
| 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.
여기서는 아스키 코드를 이용해서 숫자 범위 안에 드는지 검사할 수 있을 듯! 근데 간단하게 짜려면 이렇게 짜도 되긴 합니당
src/main/java/racingcar/Model.java
Outdated
| if(rand>=4) len[i]++; | ||
| sb.append(car[i]).append(" : ") | ||
| .append("-".repeat(Math.max(0, len[i]))) | ||
| .append("\n"); | ||
| max = Math.max(len[i], max); |
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.
이 부분이 대충 경주 결과를 문자열로 나타내는 로직인 건 알겠는데, 정확히 어떤 과정을 통해 결과가 도출되는지 명확하지 않은 듯!
아마 최댓값을 넣는 등의 부분이 메서드 분리가 잘 안된 것도 있고 변수명이 직관적이지 않은 이유도 있는 것 같습니닷
ex. len이 length를 줄인 건 알겠는데 정확히 어떤 길이를 나타내는지? 명확하지 않음
src/main/java/racingcar/Model.java
Outdated
| StringBuilder findWinner(){ | ||
| StringBuilder sb = new StringBuilder(); | ||
| boolean jointWinner = false; | ||
| for(int i=0; i<car.length; i++){ | ||
| if(len[i]==max) { | ||
| if(jointWinner) sb.append(", "); | ||
| jointWinner = true; | ||
| sb.append(car[i]); | ||
| } | ||
| } | ||
| return sb; | ||
| } |
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.
이 분은 뭔가 알고리즘 느낌으로 최적화된 코드인 것 같은데, 아마도 유지보수의 측면에서는 우승자를 고르고 바로 StringBuilder에 넣기보다는
전체 우승자 목록을 먼저 반환한다음, 최종적으로 StringBuilder에 넣는 게 좋을 듯합니다...!
| void printResult(Map<String, Integer> result){ | ||
|
|
||
| StringBuilder sb = new StringBuilder(); | ||
| sb.append("\n실행 결과\n\n"); | ||
|
|
||
| result.forEach((name, distance)-> sb.append(name).append(" : ") | ||
| .append("-".repeat(distance)) | ||
| .append("\n")); | ||
| System.out.println(sb); | ||
| } | ||
|
|
||
| void printWinner(String winner){ | ||
| System.out.print("최종 우승자 : "); | ||
| System.out.println(winner); | ||
| } |
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 view = new View(); | ||
| Model model = new Model(); | ||
|
|
||
| model.saveCarName(view.inputCarName()); | ||
|
|
||
| model.saveNumberOfAttempts(view.inputNumberOfAttempts()); | ||
|
|
||
| view.printResult(model.race()); | ||
|
|
||
| view.printWinner(model.findWinner()); | ||
| Controller controller = new Controller(); | ||
| controller.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.
이거 깔끔하게 변경 잘된거 같아요
| winners.add(list.name); | ||
| } | ||
| } | ||
| return winners.toString().replace("[", "").replace("]", ""); |
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점 😃
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.
Formatting 방식이 너무 좋네요👏🏻
| int rand = Randoms.pickNumberInRange(0, 9); | ||
| list.addDistance(rand); | ||
| result.put(list.name, list.distance); | ||
| updateMaxValue(maxDistance, list.distance); |
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.
최댓값을 비교하여 찾는 로직을 매 시도 마다 수행하는 이유가 궁금합니다! findWinner() 메서드를 실행하기 전에 한 번만 찾는 방법은 어떨까요?
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.
큰 생각없이 최대값을 구하려고 백준문제 풀때처럼 반복문 중간에 집어넣었는데 최대값을 찾는 로직을 아예 함수로 분리하는게 좋았을것같은데 이건 제 실수네요..
| winners.add(list.name); | ||
| } | ||
| } | ||
| return winners.toString().replace("[", "").replace("]", ""); |
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.
Formatting 방식이 너무 좋네요👏🏻
| String findWinner(){ | ||
| ArrayList<String> winners = new ArrayList<>(); | ||
| for (Car list : carList) { | ||
| if(list.sameDistance(maxDistance)){ |
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.
44번 라인과 57번 라인에서 향상된 for문을 사용할 때 보통은 (Car �car : carList)와 같이 사용한다고 생각합니다. carList에서 car object 하나를 가져온다는 느낌을 준다고 생각합니다.
(Car list : carList)와 같이 작성한 이유가 있을까요?
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.
foreach문을 자주 사용하지 않아서 몰랐던 사실이었어요.. 하나 배워갑니다!
Model
View
Application