[민섭] racingGame Step2 구현완료 했습니다.#33
Conversation
hyukjin-lee
left a comment
hyukjin-lee
left a comment
There was a problem hiding this comment.
일단 큰 리뷰들만 남겼습니다 반영해주세요.
| tempWinnerPosition = car.getCarPosition(); | ||
| } | ||
| return tempWinnerPosition; | ||
| } |
There was a problem hiding this comment.
RacingGame 클래스가 요구사항이 추가될수록 점점 무거워진다는 느낌이 들지 않나요?
이 중에서 공통으로 묶을 수 있는 역할들을 추상화 시켜서 새로운 클래스를 만들어보고, 그 클래스로 행위들을 위임시켜보세요
| private String splitPlayersName(String players, int index) { | ||
| String[] names = players.split(","); | ||
| return names[index]; | ||
| } |
There was a problem hiding this comment.
이 메소드들도 RacingGame 이 책임을 갖고 있기 보다는 새 클래스를 만들어서 위임시켜도 될 것 같습니다. 제가 보기엔 그냥 String 관련 작업같아보여서요.
|
|
||
| public void settingCar(int numCar) { | ||
|
|
||
| public void settingCar(String players) { |
There was a problem hiding this comment.
애초에 view에서 players를 넘겨줄 때 분리시킨 결과(배열 or List)를 넘기면 RacingGame에 아래와 같은 슬데없이 복잡한 로직이 생기지 않아도 될 것 같습니다
hyukjin-lee
left a comment
hyukjin-lee
left a comment
There was a problem hiding this comment.
저번엔 봐드렸는데 이번엔 안넘어갑니다. 각 클래스들의 테스트 코드도 작성하세요
| return playerName; | ||
| } | ||
|
|
||
| public String inputPlayerName(String name) { |
There was a problem hiding this comment.
setPlayerName이 아닐까요. input은 View에서 받아오는 것처럼 읽힙니다
| public class Main { | ||
| public static void main(String[] args) { | ||
| RacingGame racingGame = new RacingGame(); | ||
| Winner winner = new Winner(); |
There was a problem hiding this comment.
의미상 WinnerGenerator 혹은 WinnerMaker가 어울려보입니다
| Car car = new Car(); | ||
| cars.add(car); | ||
| cars.get(i).inputPlayerName(splitPlayersName(players, i)); | ||
| cars.get(i).inputPlayerName(names[i]); |
There was a problem hiding this comment.
메소드 이름이 Input 이니까 뭔가 입력을 받아야할 것 같습니다. 근데 역할로 보면 세터같은데 setPlayerName이 아닐까요?
|
|
||
| public class Winner { | ||
| private List<Car> winnerList; | ||
| private static int winnerPosition; |
There was a problem hiding this comment.
RacingGame에서 MOVE_RESTRICTION을 static으로 선언한거 보고 의미 파악도 안한테 무의식 중에 그런것 같습니다. static이란 모든 객체가 공유할만한 변수에 필요한건데 혁진님의 말대로 winnerPosition은 각자 다른 값을 갖는 변수임으로 static으로 선언한것은 잘 못 한것 같습니다.
|
|
||
| public List<Car> makeWinners(List<Car> cars) { | ||
| calculateWinnerPosition(cars); | ||
| winnerList = new ArrayList<>(); |
There was a problem hiding this comment.
여기서 초기화 시켜주는 것보다 다른 방법이 더 안전하지 않을까요?
|
|
||
| private void checkWinnerName(Car car) { | ||
| if (car.getCarPosition() == winnerPosition) { | ||
| winnerList.add(car); |
There was a problem hiding this comment.
check 라고 이름 지어놓고 add를 하고 있네요
There was a problem hiding this comment.
메소드 이름이 곧 그 메소드의 역할과 행위를 나타내므로 매우 중요합니다. 메소드 이름을 잘 짓느냐 안 짓느냐에 따라서 그 메소드를 분리해야하느냐 마느냐가 결정되기도 합니다
| private void calculateWinnerPosition(List<Car> cars) { | ||
| int tempWinnerPosition = 0; | ||
| for (Car car : cars) { | ||
| tempWinnerPosition = checkWinnerPosition(tempWinnerPosition, car); |
| winnerPosition = tempWinnerPosition; | ||
| } | ||
|
|
||
| private int checkWinnerPosition(int tempWinnerPosition, Car car) { |
There was a problem hiding this comment.
위너의 포지션값을 체크한다는 뜻인 것 같은데 실제 동작하는건 비교해서 결과를 반환해주는 것 같네요.
There was a problem hiding this comment.
이름을 다르게 하던지 아니면 이름에 맞는 기능한 수행하게 하던지. 둘중 하나
|
|
||
| private int checkWinnerPosition(int tempWinnerPosition, Car car) { | ||
| if (car.getCarPosition() > tempWinnerPosition) { | ||
| tempWinnerPosition = car.getCarPosition(); |
2 participants
리뷰 부탁드립니다.