Conversation
dh2906
left a comment
There was a problem hiding this comment.
고생하셨습니다!
메소드 명을 더 명확하게, 이게 무슨 역할을 담당하는지를 고민해주시면 좋을 것 같아요.
| static class Car { | ||
| String name; | ||
| int score = 0; | ||
| boolean winner = true; | ||
|
|
||
| Car(String name) { | ||
| this.name = name; | ||
| } | ||
| } |
There was a problem hiding this comment.
Application.java에 Car 클래스가 종속되는 것보단 파일을 추가로 만들어서 분리하는 것은 어떻게 생각하시나요?
There was a problem hiding this comment.
클래스를 사용하는 의도에 맞게 유지,보수가 편해지도록 따로 분리하여 코딩하는 습관이 있어야 할 것 같다고 생각하게 되었습니다. 참고하겠습니다.
| String name; | ||
| int score = 0; | ||
| boolean winner = true; |
There was a problem hiding this comment.
- 접근 제어자를 지정하지 않으면 무슨 일이 생길까요??
- 자동차 경주를 시작하지도 않았는데 기본값으로
winner를true로 둔 이유가 있으신가요?
There was a problem hiding this comment.
접근제어자를 지정하지 않으면 같은 패키지 안에서만 접근이 가능해집니다. 어떤 의도로 클래스를 만들었는지 알아보기 어려워질 수 있겠네요. 참고하겠습니다. winner는 실행 마지막에 score 비교 후 낮은 객체의 winner를 False로만 바꾸면 간단할 것 같다고 생각해서 미리 true로 설정했는데 이것도 경주시작도 안했는데 winner라면 말이 안되네요. 참고하겠습니다.
| } | ||
| } | ||
|
|
||
| private static boolean nameAgain(ArrayList<Car> Cars){ |
There was a problem hiding this comment.
private static메소드는 무슨 특징을 지닐까요?- 이렇게 지정한 이유가 있으신가요?
There was a problem hiding this comment.
해당 클래스 안에서만 호출이 가능하고 객체 없이 호출이 가능한 특징이 있습니다. 그리고 여기 클래스 안에서만 사용하고 따로 객체 없이 기능을 하는 메소드라는 특징이 있습니다. 따로 이유는 없이 유틸 메소드를 만들려고 static으로 지정했습니다.
| } | ||
|
|
||
| private static boolean nameAgain(ArrayList<Car> Cars){ | ||
| Set<String> nameSet = new HashSet<>(); |
| } else { | ||
| Cars.add(new Car(names[i])); | ||
| } |
There was a problem hiding this comment.
별 차이는 없지만, if 문에서 조건이 걸린다면 어차피 예외를 발생해서 프로그램을 종료하므로 else 문은 명시하지 않아도 될 것 같아요!
| for (int i = 0; i < Cars.size(); i++){ | ||
| random = Randoms.pickNumberInRange(0, 9); | ||
| if(random >= 4){ | ||
| Cars.get(i).score++; |
There was a problem hiding this comment.
이런 식으로 객체의 멤버 변수를 직접 접근하는 방식은 지양해주세요!
전용 메소드를 하나 만들어서 이를 호출하는 방식을 채택해주세요
There was a problem hiding this comment.
이것 또한 클래스를 사용한 의도랑 맞지 않네요. 메소드를 만들도록 참고하겠습니다.
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
깃허브에 코드를 올릴때는 맨 마지막 라인에 공백이 들어와야 해요!
자료 참고바랍니당
There was a problem hiding this comment.
참고하겠습니다. 피드백 공부 많이 됐습니다 감사합니다.
| public boolean isWinner() { | ||
| if (this.winner) { | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
이 메소드를 더 간결하게 만들 수도 있어요!
| public boolean isWinner() { | |
| if (this.winner) { | |
| return true; | |
| } | |
| return false; | |
| } | |
| public boolean isWinner() { | |
| return this.winner; | |
| } |
There was a problem hiding this comment.
그리고 추가로, 해당 자동차가 승자인지에 대한 정보 + 이를 판단하는 메소드를 Car 객체 안에 포함시킨 이유가 있을까요??
| public String getName() { | ||
| return this.name; | ||
| } |
There was a problem hiding this comment.
아래에 있는 메소드들의 return 도 마찬가지지만,
제공받은 인자가 따로 없으므로 this를 빼고 필드만 써주셔도 무방합니다
| if (random >= 4) { | ||
| this.cars.get(i).move(); | ||
| } |
There was a problem hiding this comment.
숫자 4 와 같이 어떤 의미가 있는 수들은 상수로 처리하는 것을 권장합니다!
| for (int i = 0; i < this.round; i++) { | ||
| moveCars(); | ||
| printScore(); | ||
| System.out.println(" "); |
There was a problem hiding this comment.
하나 팁을 드리자면,
개행을 할 때에는 System.out.print('\n') 을 하는 것이 성능 상 이득을 볼 수 있습니다!
그 이유도 같이 찾아보심 좋을 것 같아요
| private void printScore() { | ||
| for (int i = 0; i < this.cars.size(); i++) { | ||
| System.out.print(this.cars.get(i).getName() + " : "); | ||
| for (int j = 0; j < this.cars.get(i).getScore(); j++) { | ||
| System.out.print("-"); | ||
| } | ||
| System.out.println(" "); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 메소드의 내부 for문을 살펴보시면,
조건식에 this.cars.get(i).getScore() 가 있어서, for문 한 바퀴를 돌 때마다 이 조건식이 호출됩니다
그래서 비효율적일 수 있기 때문에, 이렇게 반복적으로 쓰이는 것은 필드로 저장해서 사용하는 것을 추천합니다
| private void printScore() { | |
| for (int i = 0; i < this.cars.size(); i++) { | |
| System.out.print(this.cars.get(i).getName() + " : "); | |
| for (int j = 0; j < this.cars.get(i).getScore(); j++) { | |
| System.out.print("-"); | |
| } | |
| System.out.println(" "); | |
| } | |
| } | |
| private void printScore() { | |
| for (int i = 0; i < this.cars.size(); i++) { | |
| System.out.print(this.cars.get(i).getName() + " : "); | |
| int score = cars.get(i).getScore(); | |
| for (int j = 0; j < score; j++) { | |
| System.out.print("-"); | |
| } | |
| System.out.println(" "); | |
| } | |
| } |
| Race(ArrayList<Car> cars, int round) { | ||
| this.cars = cars; | ||
| this.round = round; | ||
| } |
| private ArrayList<Car> cars; | ||
| RaceResult(ArrayList<Car> cars) { | ||
| this.cars = cars; | ||
| } |
There was a problem hiding this comment.
여기로 마찬가지로 생성자 앞에 public 써주셔야 합니다
그리고 상단에 있는 cars 필드 다음에 한 칸 개행해주시는 것을 권장합니다(코드 작성 관례와 관련이 있습니다)
| public void printWinner() { | ||
| boolean flag = true; | ||
|
|
||
| this.findWinner(); | ||
| System.out.print("최종 우승자 : "); | ||
|
|
||
| for (int i = 0; i < this.cars.size(); i++) { | ||
| if (this.cars.get(i).isWinner() && !flag) { | ||
| System.out.print(", "); | ||
| System.out.print(this.cars.get(i).getName()); | ||
| } | ||
| if (this.cars.get(i).isWinner() && flag) { | ||
| System.out.print(this.cars.get(i).getName()); | ||
| flag = false; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
flag 변수를 이용해서 쉼표 처리 로직을 구현해주신 것 같은데, 이것 또한 좋은 방법인 것 같아요!
다만 이를 StringJoiner 등을 이용해서 좀 더 단순화 시킬 수도 있습니다!!
이와 관련해서는 여러 가지 방법이 있으니, 구글링을 하시거나, AI한테 물어보거나, 다른 비기너들이 작성한 코드들도 보면서 추가적으로 고민해보시면 도움 많이 될 거예요!
| public void printWinner() { | |
| boolean flag = true; | |
| this.findWinner(); | |
| System.out.print("최종 우승자 : "); | |
| for (int i = 0; i < this.cars.size(); i++) { | |
| if (this.cars.get(i).isWinner() && !flag) { | |
| System.out.print(", "); | |
| System.out.print(this.cars.get(i).getName()); | |
| } | |
| if (this.cars.get(i).isWinner() && flag) { | |
| System.out.print(this.cars.get(i).getName()); | |
| flag = false; | |
| } | |
| } | |
| } | |
| public void printWinner() { | |
| findWinner(); | |
| StringJoiner joiner = new StringJoiner(", "); | |
| for (Car car : this.cars) { | |
| if (car.isWinner()) { | |
| joiner.add(car.getName()); | |
| } | |
| } | |
| System.out.println("최종 우승자 : " + joiner); | |
| } |
| for (int i = 0; i < this.cars.size(); i++) { | ||
| if (this.cars.get(i).getScore() >= maxScore) { | ||
| maxScore = this.cars.get(i).getScore(); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분도 Math 라이브러리를 활용해서 단순화시킬 수 있어요!
| for (int i = 0; i < this.cars.size(); i++) { | |
| if (this.cars.get(i).getScore() >= maxScore) { | |
| maxScore = this.cars.get(i).getScore(); | |
| } | |
| } | |
| for (Car car : this.cars) { | |
| maxScore = Math.max(maxScore, car.getScore()); | |
| } |
| private ArrayList<Car> cars; | ||
| private int round; | ||
|
|
||
| RaceSetup(){} |
There was a problem hiding this comment.
해당 생성자를 작성해주신 이유가 있을까요??
자바에서는 생성자를 작성해주지 않아도 기본 생성자(어떤 인자도 받지 않는 생성자)를 자동으로 생성해주는 기능이 있습니다!
| for (int i = 0; i < names.length; i++) { | ||
| if (names[i].length() > 5 || names[i].contains(" ") || names[i].isEmpty()) { | ||
| throw new IllegalArgumentException(); | ||
| } | ||
| cars.add(new Car(names[i])); | ||
| } |
There was a problem hiding this comment.
for-each 문으로 바꾸면 좀 더 간결하게 작성하실 수 있습니다
| for (int i = 0; i < names.length; i++) { | |
| if (names[i].length() > 5 || names[i].contains(" ") || names[i].isEmpty()) { | |
| throw new IllegalArgumentException(); | |
| } | |
| cars.add(new Car(names[i])); | |
| } | |
| for (String name : names) { | |
| if (name.length() > 5 || name.contains(" ") || name.isEmpty()) { | |
| throw new IllegalArgumentException(); | |
| } | |
| cars.add(new Car(name)); | |
| } |
There was a problem hiding this comment.
그리고 자동차 이름 검증하는 부분(if (names[i].length() > 5 || names[i].contains(" ") || names[i].isEmpty())) 은 별도의 메소드로 분리해주셔도 좋을 것 같습니다! 현재 setupCars() 메소드가 맡고 있는 역할이 조금 많아 보입니다
| import java.util.Set; | ||
|
|
||
| public class RaceSetup { | ||
| private ArrayList<Car> cars; |
There was a problem hiding this comment.
리스트형 변수를 선언할 때에는 ArrayList 보단 List 를 사용하시는 것을 권장합니다
구현체(ArrayList)보단 인터페이스(List)에 의존하는 것이 더 좋습니다
| RaceSetup setup = new RaceSetup(); | ||
| setup.raceSetup(); |
There was a problem hiding this comment.
RaceSetup 클래스의 raceSetup 메소드를 RaceSetup 생성자 내에서 실행되도록 하면 setup.raceSetup() 없이 new RaceSetup() 만 해도 레이스 세팅이 자동으로 되도록 하는 것도 가능합니다..!
테스트 모두 확인하고 올립니다.
