Skip to content

[자동차경주] 이성준 미션 제출합니다.#7

Open
szoon2426 wants to merge 2 commits intoJava-JavaScript-Language-Stuty:mainfrom
szoon2426:szoon2426
Open

[자동차경주] 이성준 미션 제출합니다.#7
szoon2426 wants to merge 2 commits intoJava-JavaScript-Language-Stuty:mainfrom
szoon2426:szoon2426

Conversation

@szoon2426
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@seulnan seulnan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 고생하셨습니다:) 리뷰하면서 저도 많은 걸 얻어가서 뜻깊었습니다 좋은 코드 잘읽었습니당

전체적으로 말씀드리고 싶은 점

  1. 벡터나 for문에서 cpp의 향기가 느껴져서 stream이나 arraylist를 활용하시면 더 좋은 코드가 될 듯 해용
  2. 커밋, PR description 등을 더 적극활용해주시길 부탁드려용! 리뷰할 때 흐름이해에 도움이 됩니당ㅎㅎ..

import java.util.regex.Pattern;

public class Application {
static Vector<Player> listOfPlayers = new Vector<>();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vector를 java에서 쓰지않는이유

vector대신 arraylist를 활용하는 것은 어떨까용?? 불필요한 동기화처리도 줄이고 성능적으로도 더 적절해요!

}

// 게임 결과
static String getGameResult(int player){
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

player을 매개변수로 받기보다는 player객체 자체를 받아서 출력하는 방향이 더 좋을 듯 해용

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니다~~ 이러면 내부 로직의 함수 호출을 한번으로 줄일 수 있을 듯 싶습니다~~

return name;
}
String printDistance(){
String answer = "";
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stirng은 변경 불가능한 문자열을 생성하지만 StringBuilder는 변경 가능한 문자열을 만들어 주기 때문에, String을 합치는 작업 시 하나의 대안이 될 수 있다. (링크한 블로그에서 따온 문구)
StringBuilder사용법

answer을 ""로 선언하고 answer += "-"; 을 반복하는 방식은 문자열이 계속 생성되기때문에 성능저하가 일어날 수 있대용
StringBuilder를 사용하여 최적화를 해보는건 어떨까용?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니다

Comment on lines +6 to +13
Player(){
this.name = "";
this.distance = 0;
}
Player(String name){
this.name = name;
this.distance = 0;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Player(){
this.name = "";
this.distance = 0;
}
Player(String name){
this.name = name;
this.distance = 0;
}
public class Player {
private static final int DEFAULT_DISTANCE = 0;
private String name;
private int distance;
Player() {
this("", DEFAULT_DISTANCE); // 기본 생성자에서 다른 생성자 호출
}
Player(String name) {
this(name, DEFAULT_DISTANCE); // 중복 제거
}
Player(String name, int distance) {
this.name = name;
this.distance = distance;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이런식으로 바꾸면 중복코드도 제거되고 가독성도 좋아질 것 같아용
기존 코드는 모든 생성자에서 중복으로 distance를 설정해야해서 기본값이 바뀔 경우 모든 생성자를 다 수정해야하지만,
수정 코드는 기본값을 한 곳에서 통제가능해서 유지보수성이 좋아질 듯 해용

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 저렇게 수정하는게 실수도 줄이고 읽고 이해하기 편해 보이네요. 저도 한 수 배우고 갑니다..!

}
System.out.println();
};
System.out.printf("최종 우승자 : " + printWinner());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

단순 문자열 출력이라 포맷팅이 필요하지않아보이는데 printf를 쓰신 이유가 있을까용??

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어,, 잘 모르고 그냥 사용했는데 printf의 의미를 더 알아보고 사용해보도록 하겠습니다!!!

Copy link
Copy Markdown

@Regyung Regyung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코드 너무 잘 봤어요! 보면서 저도 새롭게 고치거나 배워야 할 부분들을 얻어 간 것 같네요 ㅎㅎ 스터디도 2주 밖에 안 남았으니 힘내봅시다!!

System.out.printf("최종 우승자 : " + printWinner());
}
}
} No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석으로 설명을 붙인 메서드들을 다른 폴더들에 분리해서 작성하고 정리하는 게 프로젝트 파일을 보는데 더 편할 것 같아요! 한 번 시간 나실 때 MVC 패턴에 대해 검색해 보세요. 저도 처음 코딩할 땐 굳이? 싶었지만 직접 해보니 코드 작성이 더 깔끔해지는 것 같아요!

if(player.getDistance() == value){
listOfWinners.add(player.getName());
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 for 문도 가능하다면 stream을 이용하면 코드가 더 간략해지고 이해하기 쉬울 것 같아요! 대신 한 번 열린 stream은 재사용이 불가하니 한 번 해당 내용에 대해 알아보시는 것도 좋을 것 같네요,

Comment on lines +6 to +13
Player(){
this.name = "";
this.distance = 0;
}
Player(String name){
this.name = name;
this.distance = 0;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 저렇게 수정하는게 실수도 줄이고 읽고 이해하기 편해 보이네요. 저도 한 수 배우고 갑니다..!

Copy link
Copy Markdown

@daeGULLL daeGULLL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔하게 필요한 것들만 잘 구현하신 것 같아요!! 리뷰하면서 저도 많은 것 얻어갑니다. 정말 수고하셨습니다~~~

for(int i = 0; i<listOfPlayers.size(); i++){
if(Randoms.pickNumberInRange(0,9) >=4){
listOfPlayers.get(i).addDistance();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listofPlayers에 대해서 for문을 돌리시는 것 같은데, 만약 내부의 인스턴스를 사용하실거면 .forEach()를 찾아보고 그걸 쓰는 방향으로 고치고 써보시면 더 좋을 것 같습니다!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니다

}

// 게임 결과
static String getGameResult(int player){
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니다~~ 이러면 내부 로직의 함수 호출을 한번으로 줄일 수 있을 듯 싶습니다~~

String names = Console.readLine();
System.out.println("시도할 횟수는 몇회인가요?");
int times = Integer.parseInt(Console.readLine());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 이렇게 하시면 .parseInt부분에서 미션 요구조건의 예외 타입과 다른 예외가 나올 수도 있을 것 같습니다! 잘못된 입력에 따라 IllegalExpression이었나..아무튼 이 에러를 throw하게 고치시면 더 좋을 것 같습니다~~

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니당

for(int i = 0; i<listOfPlayers.size(); i++){
if(Randoms.pickNumberInRange(0,9) >=4){
listOfPlayers.get(i).addDistance();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니다

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

거의 대부분 static을 쓰셨는데 그 이유가 있으실까용?

String names = Console.readLine();
System.out.println("시도할 횟수는 몇회인가요?");
int times = Integer.parseInt(Console.readLine());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니당

System.out.println("시도할 횟수는 몇회인가요?");
int times = Integer.parseInt(Console.readLine());

// 입력받은 이름들을 player 리스트에 담아줌
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

호건티쳐가 말씀해주셨는데 메서드로 분리해서 사용한다면 주석이 없이도 직관적으로 코드를 이해할 수 있을 것 같아요

return name;
}
String printDistance(){
String answer = "";
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동의합니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants