Skip to content
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

김승찬 - 자동차 경주 게임 #8

Open
wants to merge 14 commits into
base: sschan99
Choose a base branch
from

Conversation

sschan99
Copy link

@sschan99 sschan99 commented Mar 18, 2023

구현 클래스 및 메서드

  • Car : 자동차 경주 게임의 주요 모델

    • forward : 랜덤한 숫자를 생성하고 4 이상일 경우 position 증가
    • toString : sout을 보조하기 위해 구현
  • InputView : 입력을 담당하는 클래스

    • receiveCarNames : 자동차 이름들을 입력받음
    • receiveAttemptNumber : 시도할 횟수를 입력받음
    • validate : 입력이 잘못되었는지 검증하고 잘못되었다면 예외를 발생시킴
  • OutputView : 출력을 담당하는 클래스

    • printExecutionResults : 각 차수별 실행 결과를 출력
    • printPreface : 머리말 출력
    • printFinalWinner : 순차 탐색을 통해 최종 우승자 목록을 가져온 뒤 출력
  • Application : 구현한 함수들을 사용하여 main 함수에서 게임을 실행하는 클래스

    • initializeCars : 입력받은 자동차 이름들을 사용해서 인스턴스들을 생성
    • moveCars : 각 자동차 객체마다 forward 메서드를 실행

리뷰 받고 싶은 부분

  • 변수 및 메서드의 이름들에 부자연스러운 점이 있는지, 혹은 다른 좋은 예시가 있는지
  • input과 output을 위한 로직을 최대한 분리해 봤는데 적절한지
  • 구현을 위해 더 좋은 디자인 패턴이 있는지

코드 리뷰 후기

  • 다른 사람들에게 보여지는 것을 전제로 하다보니 최대한 이해하기 쉽게 가독성을 신경써서 코드를 작성하게 되었습니다.
  • 코드 리뷰를 통해서 알 수 있었던 개선할 부분은 다음과 같습니다.
    • 단일 책임 원칙을 준수하기 위해 더욱 신경써야 합니다.
    • 메서드 네이밍을 더 자세히 작성하는 것이 좋습니다.
    • 이외에도 다양한 새로운 관점들을 통해 자신의 코드를 다시 관찰해보는 계기가 되었습니다.
  • 최종적으로 잘 작성된 코드를 두고 혼자 만족하는 것이 아닌 다른 사람들에게 내 코드를 공유하는 경험이 재밌었습니다.

Copy link
Collaborator

@rohsik2 rohsik2 left a comment

Choose a reason for hiding this comment

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

전반적으로 코드를 깔끔하게 잘 짜신것 같습니다! 코드의 양이 적고, 필요한 메소드만 존재하는게 굉장히 마음에 들어요!

Comment on lines 22 to 30
for (Car car : cars) {
if (car.getPosition() > max) {
finalWinner.clear();
max = car.getPosition();
}
if (car.getPosition() == max) {
finalWinner.add(car.getName());
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Output View에서 우승자를 가려내는 알고리즘이 실행되고 있는데, 대부분의 다른 로직들은 Application에 있는것으로 보입니다. 해당 로직만 여기서 처리된 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

미처 생각하지 못한 부분이네요.
해당 부분을 Application으로 옮겨서 역할에 맞는 기능을 수행하도록 수정하겠습니다.
감사합니다!

List<String> carNames = Arrays.asList(readLine().split(","));
try {
validate(carNames);
} catch (IllegalArgumentException iae) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

메번 에러 메세지 변수명을 e로만 지었는데 iae와 같이 조금 더 정보가 있는 이름도 좋은것 같습니다.

Choose a reason for hiding this comment

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

메세지 변수명도 예외 항목에 따라서 바꾸는 디테일을 배울 수 있어 흥미로웠습니다

import java.util.List;
import java.util.regex.Pattern;

import static camp.nextstep.edu.missionutils.Console.readLine;
Copy link
Collaborator

Choose a reason for hiding this comment

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

static import 보다는 그냥 해당 클래스를 import하는게 조금 더 좋은 것으로 알고 있습니다. STATIC import 는 주로 상수를 가져올때 많이 쓰는것 같던데, 혹시 해당 부분에서 함수를 static import하신 이유가 있으신가요?

Copy link
Author

Choose a reason for hiding this comment

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

코드의 가독성을 높이기 위해 static import를 사용했습니다.
하지만 자주 사용하는 메서드가 아닌 만큼 기본적인 import문을 사용해도 괜찮아 보이네요.
의견 주셔서 감사합니다!

별개로 질문드리고 싶은 것은 import문의 경우 컴파일 시에 처리하니까 성능적인 차이는 없는게 아닌가요?
조금 더 좋다고 얘기하신 근거에 대해서 궁금합니다!

Comment on lines 46 to 50
private void validate(String AttemptNumber) {
if (!Pattern.matches("^[0-9]+$", AttemptNumber)) {
throw new IllegalArgumentException("[ERROR] 숫자만 입력할 수 있습니다.");
}
}
Copy link

@Untaini Untaini Mar 21, 2023

Choose a reason for hiding this comment

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

패턴을 이용해서 자연수를 검사하는 방식은 참신해보여요. 원하는 에러메시지를 보내기 위해 NumberFormatException을 catch하고 새로 IllegalArgumentExcetion을 만드는 것보다 직관적으로 보입니다. XD

Comment on lines +27 to +33
public String toString() {
StringBuilder bar = new StringBuilder();
for (int i = 0; i < this.position; i++) {
bar.append("-");
}
return this.name + " : " + bar;
}
Copy link

Choose a reason for hiding this comment

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

StringBuilder를 사용하는 이유가 + 연산보다 속도가 빠르기 때문으로 알고 있습니다. 혹시 bar만 StringBuilder를 쓰고 return 에서는 + 연산을 사용한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

java 컴파일러에서 + 연산을 StringBuilder의 append로 바꿔준다고 합니다!
따라서 둘 중 무엇을 사용하든 성능상으로는 비슷하다고 하는데, 개인적으로는 간결하게 + 연산을 사용하는 것을 좋아합니다. :)
그러나 반복문 내에서는 컴파일시에 변환이 안된다고 하여 StringBuilder를 사용했습니다!

참고
https://stackoverflow.com/questions/7817951/string-concatenation-in-java-when-to-use-stringbuilder-and-concat
https://choichumji.tistory.com/135

}
}

private void validate(List<String> carNames) {
Copy link

Choose a reason for hiding this comment

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

같은 이름의 함수가 서로 다른 기능을 수행하고 있는 것으로 보여요. 혹시 좀 더 구체적으로 함수의 이름을 지어보시는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

서로 다른 이름으로 변경하였습니다!
의견 감사합니다!

Copy link

@Untaini Untaini left a comment

Choose a reason for hiding this comment

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

제가 사용해보지 않은 방식으로 짜여져 있는 코드가 있어 해석하는 재미가 있었습니다.
LGTM XD

@Jaehooni
Copy link

흥미롭게 읽었습니다!
LGTM XD

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.

4 participants