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

[코드 리뷰용 PR입니다!] - 재구현 스터디 #236

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

Conversation

getupminaaa
Copy link

No description provided.

getupminaaa and others added 30 commits October 28, 2023 15:49
각 자동차마다 움직인 거리 저장하는 기능 추가
regex를 제거하고 조건문으롤 바꿈. regex의 경우 패턴을 중심으로 파악하기 때문에 정확하게 작성하기 어려웠음
getupminaaa added 21 commits October 31, 2023 12:06
# Conflicts:
#	docs/README.md
Comment on lines +7 to +8
val validAttemptsNum: Int
get() = getValidAttemptsNum().toInt()
Copy link

Choose a reason for hiding this comment

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

validAttemptsNum 라는 변수를 호출할 때 마다
getValidAttemptsNum().toInt() 의 연산을 계속 하게 됩니다.
아예 init 키워드로 한번의 계산만 하고 계산된 값만 가져오게 수정하면 더 좋을 것 같습니다.

Comment on lines +21 to +22
_carNames = carNameList
return _carNames
Copy link

Choose a reason for hiding this comment

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

이 부분에서 이미 _carNames = carNameList를 통하여 값을 넘겨준 것 같은데
한번 더 return으로 값을 넘겨줄 필요가 없어보입니다.
return carNameList 만 하던가 아니면 void 형으로 만드는 것이 좋아 보여요

Comment on lines +26 to +28
val initializedCarList: List<Car> = _carNames.indices.map { Car(name = "", distance = 0) }
_carNames.forEachIndexed { idx, name -> initializedCarList[idx].name = name }
return initializedCarList
Copy link

Choose a reason for hiding this comment

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

_carNames.map { Car(name = it, distance = 0) }
으로 한줄로 간결하게 만들 수 있을 것 같아요

cars.forEach { car ->
val randomNumber = makeRandomNumber()
if (determineMoveOrStop(randomNumber)) car.distance++
println(car.distance)
Copy link

Choose a reason for hiding this comment

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

model에서는 view와 의존하면 안돼요.
모델은 뷰와 컨트롤러와 의존하지 않아야 하며 데이터 관련해서만 처리해야 합니다.

private var _winner = ""


private fun makeRandomNumber() = Randoms.pickNumberInRange(0, 9)
Copy link

Choose a reason for hiding this comment

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

0, 9 값 전부 상수로 만들면 가독성이 좋아질 것 같습니다.

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

object Validator {
Copy link

Choose a reason for hiding this comment

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

require 문을 사용해보시는 것도 좋을 것 같습니다

private fun doRacingAndPrintResult() {
val racing = Racing(carList)
repeat(attemptsNum) { racing.runRaceOnce().apply { outputView.printMatchProgress(carList) } }
racing.getWinner()
Copy link

Choose a reason for hiding this comment

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

getWinner() 라는 이름은 '우승자를 가져오다' 라는 의미로 사용돼 우승자를 반환하는 의미 해설될 수 있을 것 같아요.
차라리 determineWinner() 같은 이름은 어떨까요?

Comment on lines +10 to +12
private fun getValidAttemptsNum(): String {
isNumberAttemptsValid(attempts)
return attempts
Copy link

Choose a reason for hiding this comment

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

함수로 만들어 검증하는 것 보다는 init 키워드를 만들어 검증만 하는 식으로 만드는 것도 좋을 것 같습니다

Comment on lines +16 to +17
if (carName.length > Constants.CAR_NAME_MAX_LENGTH) throw IllegalArgumentException(NAME_OVER_LENGTH_MSG)
}

Choose a reason for hiding this comment

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

이부분에서 혹시 Constants를 import하지 않고 사용하신 이유가 있을까요?만약 특별한 이유가 없다면 Constants를 import하는 쪽이 좀 더 간결하게 보일 것 같아서요

Comment on lines +24 to +27
- 자동차 이름이 올바른 값인 지 검사하는 기능
- 올바른값: Null X, 5자리 이하, 중복 X
- 사용자가 몇 번 이동할 지 입력 할 때 숫자를 올바른 값을 입력하는 지 검사하는 기능
- 올바른값: Null X, 0 보다 큰 정수

Choose a reason for hiding this comment

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

예외 처리에 대해 꼼꼼하게 정리해주시고 README를 두분 모두 깔끔하게 정리해주셔서 여러모로 많이 배우네요:)

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.

3 participants