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

[자동차 경주] 이인협 미션 제출합니다. #99

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

cucumber99
Copy link

@cucumber99 cucumber99 commented Oct 28, 2024

설계 및 구현

MVC 패턴에 기반하여 코드를 작성하였습니다.

Model

  • Car : 자동차를 나타내기 위한 클래스
  • CarList : Car 객체 리스트를 멤버 변수로 갖고 있는 클래스
  • Round : 난수 생성을 통해 자동차 전진 여부 결정
  • Referee : Car 객체를 비교하여 가장 많이 전진한 자동차의 이름 반환
  • RacingStatus : 라운드마다 각 자동차의 위치 반환
  • RandomNumberGenerator : 난수 생성

객체 간의 결합도 및 응집도보다 책임 분리에 더 중점을 두고 설계하였습니다.
이러한 이유 때문에 아직까지 결합도 문제를 해결하지 못했습니다.
리뷰하러 오신 분들 이 부분 최대한 많이 피드백 해주시면 감사하겠습니다..

View

  • InputView : 입력 담당
  • OutputView : 출력 담당

두 기능 모두 싱글톤 패턴을 위해 object를 사용했습니다.

Controller

RacingController 객체의 상태를 최소화하기 위해서 필요할 때만 모델의 객체를 생성하고,
해당 객체가 데이터만 반환하게끔 코드를 작성했습니다.

util

  • Validator : 입력에 대한 예외 처리
  • ErrorMessage : 예외 발생 시 출력할 오류 메시지

사용자에게서 입력을 받습니다.
특정 메시지와 실행 결과를 출력합니다.
예외 발생 시 오류 메시지가 출력됩니다.
입력에 대한 유효성을 검사합니다.
유효하지 않은 경우 예외를 발생시키고 오류 메시지를 출력합니다.
자동차의 정보(이름, 위치)를 나타냅니다.
Car 객체 리스트를 관리합니다.
숫자를 생성하는 generate 메소드를 정의합니다.
NumberGenerator 인터페이스를 구현합니다.
특정 범위 내에서 무작위로 숫자를 생성합니다.
라운드마다 각 자동차의 전진 여부를 결정합니다.
각 라운드에서 자동차들의 상태를 반환합니다.
자동차들의 위치를 통해 최종 우승자를 산출합니다.
라운드 진행 횟수를 저장합니다.
코드 컨벤션에 맞게 코드를 수정합니다.
자동차의 초기 위치를 직접 설정합니다.
Copy link

@donghyun81 donghyun81 left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다. 정말 열심히 고민하고 구현한 흔적이 보이네요 ㅎㅎ

import racingcar.util.Validator

class AttemptCount(private val input: String) {
val count: Int = input.toInt()

Choose a reason for hiding this comment

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

여기서 문자 포함될 경우 toInt() 하면 예외가 발생할거 같은데 밑에 init에서 예외 처리를 하신거 같아서
한번 확인 해보시겠어요?

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다. init 블록을 이용해 객체가 생성되는 동시에 입력값에 대한 검증을 진행했습니다.
테스트에서는 문제가 발생하지 않았는데, 코드 가독성 면에서 좋은 코드는 아닌 것 같네요.

Choose a reason for hiding this comment

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

init 블록이랑 input.toInt()는 지금 따로 실행되고 있어서 문자가 입력 되면
"시도할 횟수는 몇 회인가요?

Exception in thread "main" java.lang.NumberFormatException: For input string: "ㅁ" " 이렇게 출력 됩니다!
init에서 검증하기전에 toInt()에서 에러가 먼저 처리되어서 init에서 예외 처리를 못하고 종료되고 있는것 같아서 확인 해보시겠어요?

Copy link
Author

Choose a reason for hiding this comment

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

보통 문제가 아니었네요 지적 감사합니다
이 부분에 대해서는 따로 처리를 해주어야겠네요


fun getAttempt(): String {
return getUserInput()
}

Choose a reason for hiding this comment

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

View는 어떤 역할을 한다고 생각하실까요?
데이터 처리 부분에서 getCars에서는 split이 들어가는데 getAttempt는 toInt()와 같은 처리가 안들어가서 질문드립니다!

Copy link
Author

Choose a reason for hiding this comment

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

View는 단순히 사용자와 상호작용하며 입력을 받거나 출력을 할 뿐, 그 이상의 역할은 하지 않는다고 생각합니다.
값에 대한 검증은 특정 객체가 생성될 때 수행하게끔 코드를 작성해서 getCars는 입력된 문자열을 미리 구분자로 나누어 문자열 리스트를 반환하게 했습니다.
만약 구분자가 잘못 포함되어 있다고 해도, 이름 길이 제한 등 여러 유효성 검사 메소드에서 확인되기 때문에 위와 같이 콛를 작성했습니다.
하지만 이렇게 보니 바람직한 코드는 아닌 것 같네요. 수정해서 코드의 재사용성을 높일 수 있을 것 같습니다.

validateLength(it)
validateNull(it)
}
}

Choose a reason for hiding this comment

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

함수명이 validateInput이라 모든 사용자 입력을 검증 하는것 같아서 validateCarsName 처럼 대상을 알려주는것이 좋을것 같습니다.

@@ -0,0 +1,57 @@
package racingcar.util

object Validator {

Choose a reason for hiding this comment

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

Validator가 예외에 관한 모든 책임을 가지고 있는것 같은데 분할 하는게 좋을것 같습니다!
한 파일에 입력 두개에도 이렇게 많은 예외 처리를 가지게 되는데
입력이 더 늘어난다고 생각하면 파일이 너무 커서 가독성이 더 떨어질것 같습니다!

Copy link

@AppleCEO AppleCEO left a comment

Choose a reason for hiding this comment

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

우와... 클래스들을 자유자재로 사용하시네요. 많이 배웠습니다.
고생하셨어요~
제 PR도 가능하시면 리뷰 부탁드립니다!~
#52

Comment on lines +9 to +13
val carList = getCarList()
val attemptCount = getAttemptCount()
showResult()
processRacing(carList, attemptCount)
showWinners(carList)

Choose a reason for hiding this comment

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

이름들이 명확하게 어디에서 어떤 내용이 진행되는지 순서대로 알기 쉽네요!


private fun processRacing(carList: CarList, attemptCount: AttemptCount) {
val round = Round(RandomNumberGenerator())
repeat(attemptCount.count) {

Choose a reason for hiding this comment

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

저는 이 부분 바로 읽히지는 않았어요. count 가 반복되기도 해서 반복횟수.횟수 라고 번역되기도 해서요.
AttemptCount 라는 객체에 int형 변수만 있던데요. 바로 int형 변수를 사용하는 것은 어떨가 싶습니다~
아니면 AttemptCount 대신에 Attempt 라는 이름으로 변경되어도 좋겠어요. 횟수에 대한 객체인 Attemp 에 count 라는 프로퍼티가 저장되는 식으로요.

Copy link
Author

Choose a reason for hiding this comment

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

확실히 여러 피드백에서 네이밍에 대한 문제가 많다는 것을 깨닫게 되네요.. 감사합니다!!

Comment on lines +40 to +51
private fun showRoundStatus(carList: CarList) {
val racingStatus = RacingStatus()
val carDtoList = racingStatus.getCurrentRacingStatus(carList.cars)
carDtoList.forEach { OutputView.printPosition(it.name, it.position) }
println()
}

private fun showWinners(carList: CarList) {
val referee = Referee()
val winners = referee.getWinners(carList.cars)
OutputView.printWinners(winners)
}

Choose a reason for hiding this comment

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

대부분의 함수에 길이가 5줄 이하라서 너무 좋네요! 👍


import camp.nextstep.edu.missionutils.Randoms

class RandomNumberGenerator: NumberGenerator {

Choose a reason for hiding this comment

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

임의의 숫자를 반환하는 역할을 하는 객체로 만든 것 멋지네요~ 🥇

Comment on lines +4 to +7
fun getWinners(cars: List<Car>): List<String> {
val maxPosition = cars.maxOf { it.position }
return cars.filter { it.position == maxPosition }.map { it.name }
}
Copy link

@AppleCEO AppleCEO Oct 29, 2024

Choose a reason for hiding this comment

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

getWinners 라는 네이밍으로는 자동차 목록이 반환되는 것으로 오해될 수 있겠어요.
getWinnersNames 등의 이름으로 차의 이름이 반환되는 것을 명시해도 좋겠습니다~ 🔢

Comment on lines +3 to +12
enum class ErrorMessage(private val message: String) {
INPUT_ERROR("올바르지 않은 입력입니다."),
LENGTH_ERROR("자동차 이름은 5자 이하여야 합니다."),
DUPLICATE_ERROR("중복되는 자동차가 존재합니다."),
COUNT_ERROR("자동차는 2대 이상이어야 합니다."),
ATTEMPTS_ERROR("횟수는 숫자여야 합니다."),
ATTEMPTS_VALUE_ERROR("횟수는 1 이상이어야 합니다.");

fun getMessage(): String = "[ERROR] $message"
}

Choose a reason for hiding this comment

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

에러들도 모아놓으셨네요! 💯

Copy link

@wjdrjs00 wjdrjs00 left a comment

Choose a reason for hiding this comment

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

2주차 미션 진행하느라 고생많으셨습니다!!
확실히 메소드 분리가 잘되어있어서 읽기 편했던 코드였던거 같아요!! 남은 미션들도 파이팅 입니다!

private fun showRoundStatus(carList: CarList) {
val racingStatus = RacingStatus()
val carDtoList = racingStatus.getCurrentRacingStatus(carList.cars)
carDtoList.forEach { OutputView.printPosition(it.name, it.position) }

Choose a reason for hiding this comment

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

인협님 방식도 코드가 간략해진다는 장점이 있지만!!! it말고 car를 사용하면 가독성 측면에서는 이 방법이 조금 더 좋지 않을까? 생각에 남겨봅니다! 이건 아마 취향차이..? 일거 같기도 합니다!!

Suggested change
carDtoList.forEach { OutputView.printPosition(it.name, it.position) }
carDtoList.forEach { car ->
OutputView.printPosition(car.name, car.position)
}

Copy link
Author

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 +8
if (numberGenerator.generate() >= MIN_FORWARD_VALUE) {
car.moveForward()
}

Choose a reason for hiding this comment

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

이번 프리코스 코드리뷰 과정을 통해 가지게된 생각을 공유해봅니다.
저도 인협님처럼 바로 메소드를 사용했었는데, 한번 변수를 거친 후 사용하는게 디버깅 측면이나, 명확성 측면에서 좀더 유리하겠다는 생각을 가졌습니다!😀

Suggested change
if (numberGenerator.generate() >= MIN_FORWARD_VALUE) {
car.moveForward()
}
val randomNumber = numberGenerator.generate()
if (randomNumber >= MIN_FORWARD_VALUE) {
car.moveForward()
}

Comment on lines +12 to +14
fun printStartMessage() {
println(START_MESSAGE)
}

Choose a reason for hiding this comment

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

이미 알고계실거 같지만, 이렇게 단일 표현식으로 사용할수도 있슴다!😀

Suggested change
fun printStartMessage() {
println(START_MESSAGE)
}
fun printStartMessage() = println(START_MESSAGE)

Copy link
Author

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.

4 participants