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

[step5] 자동차 경주(리팩터링) #1752

Open
wants to merge 2 commits into
base: saerang
Choose a base branch
from

Conversation

Saerang
Copy link

@Saerang Saerang commented Nov 24, 2024

아래에 대해서 리팩터링을 진행해봤습니다.
Input, Output 도 분리하였습니다.

  • 타입추론 추가
  • 필드 외부 노출

이번 미션 하면서, 생성자나, static, 타입추론, 필드 노출 등 많은 이해가 된 것 같습니다.
미션 도움 주셔서 감사합니다 :)

Copy link

@vsh123 vsh123 left a comment

Choose a reason for hiding this comment

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

안녕하세요!

step5 빠르게 잘 구현해주셨네요 💯
몇가지 코멘트를 남겼으니 확인부탁드립니다~!

package racingcar.view

object InputView {
fun carNames(): String {
Copy link

Choose a reason for hiding this comment

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

함수명은 동사형으로 작성해주면 어떨까요?


fun roundCount(): Int {
println("시도할 횟수는 몇 회인가요?")
return readln().toInt()
Copy link

Choose a reason for hiding this comment

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

toInt()는 숫자가 들어오지 않으면 에러를 내뱉는데요!! toIntOrNull을 활용해서 숫자가 아닌 값을입력했을때의 에러처리를 고민해보면 어떨까요?

fun cars(): List<Car> {
return cars
}

companion object {
fun usernames(usernames: String): Cars {
Copy link

Choose a reason for hiding this comment

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

정적 팩토리 메서드 네이밍 컨벤션을 읽어보고, 네이밍에 대해 고민해보면 어떨까요?

Comment on lines 16 to 20
val positions: MutableList<Position> = ArrayList()
for (car in cars) {
positions.add(car.position())
positions.add(car.position)
}
return positions
Copy link

Choose a reason for hiding this comment

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

kotlin collection에서 지원하는 map을 이용해서 코드를 조금 더 간결하게 짤 수 있을 것 같아요!! 고민한번 해보면 좋을 것 같습니다 :)

println("시도할 횟수는 몇 회인가요?")
val round: Int = readln().toInt()
val usernames = InputView.carNames()
val round = InputView.roundCount()

val cars = Cars.usernames(usernames)
repeat(round) {
cars.race(RandomNumberGenerator())
Copy link

Choose a reason for hiding this comment

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

RandomNumberGenerator는 싱글톤 객체로 만들 수 있지 않을까요?

import racingcar.domain.Cars
import racingcar.domain.Winners

object OutputView {
Copy link

Choose a reason for hiding this comment

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

outView에 Car객체를 리턴하게 되면 View레벨에서 Car 객체 안에있는 함수를 실행할 수 있을 것 같아요 :)
DTO를 활용해보면 어떨까요?

fun gameWinner(cars: Cars) {
val winners = Winners(cars)
val winnersName = winners.getWinnerNames().joinToString(",")
println(winnersName + "가 최종 우승했습니다.")
Copy link

Choose a reason for hiding this comment

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

kotlin의 String templates을 활용해보면 어떨까요?

}

fun gameWinner(cars: Cars) {
val winners = Winners(cars)
Copy link

Choose a reason for hiding this comment

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

Winner를 구하는게 View의 역할일까요? Cars에 있어도 좋을 것 같아요 :)

@@ -1,18 +1,10 @@
package racingcar.domain

class Car(private var position: Position, private val username: String) {
class Car(var position: Position, val username: String) {
Copy link

Choose a reason for hiding this comment

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

position의 setter도 public으로 열리게된 것 같아요!

https://ksabs.tistory.com/247

위 글을 읽어보고 프로퍼티의 setter는 private으로 막아보면 어떨까요?

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.

2 participants