[자동차 경주 step1] 양두영 미션 제출합니다. #19

Due-IT · 2024-08-09T08:19:49Z

No description provided.

SeongHoonC

안녕하세요. 듀!
요구사항에 대해서 고민하신 흔적이 보여 좋았습니다. 또한 커밋 단위도 잘게 쪼개져있구요.
다만 관심사 분리에 대해 조금 아쉬운 부분들이 보여요. 관련해서 리뷰를 좀 많이 달았으니 반영하고 재요청 바랍니다!

SeongHoonC · 2024-08-11T08:21:23Z

README.md

상세한 리드미 작성이 보기 좋네요. 💯

감사합니다. 코드리뷰 달아주신후에 볼 시간이 없어서 조금 늦게 확인하게 되었는데 빠르게 수정해서 반영하도록 하겠습니다!

SeongHoonC · 2024-08-11T08:24:08Z

src/main/kotlin/racingcar/model/Cars.kt

+    fun printCarsPositions(){
+        carList.forEach{it.printNowPosition()}
+        println()
+    }
+
+    fun printWinners(){
+        val winners : List<String> = getWinnerNames()
+        val winnerNames = winners.joinToString(", ") { it }
+        println("최종 우승자 : $winnerNames")
+    }
+
+    fun getWinnerNames():List<String>{
+        val maxPosition = carList.maxOf { it.position }
+        return carList.filter { it.position == maxPosition }
+            .map { it.name }
+    }


출력 로직이 많네요. model 이 출력에 의존하고 있어요. (어떻게 출력되는지 알고있습니다.)

추가로 자동차들이라는 객체가 Winner (우승) 이라는 개념을 알아야할까요? 관심사 분리가 필요해보여요:)

동의합니다. 우승이라는건 게임을 주최 및 통제하는 Controller만 알고 있으면 될 것 같아요.
그리고 어떻게 출력이 되어야 하는지는 아나운서만 알고 있으면 될것 같네요 ㅎㅎ
관심사 분리에 대한 고민이 부족했던것 같습니다!

SeongHoonC · 2024-08-11T11:30:23Z

src/main/kotlin/racingcar/model/Car.kt

@@ -0,0 +1,17 @@
+package racingcar.model
+
+data class Car (val name : String, var position : Int = 0){


data class 로 한 이유가 무엇인가요?

현재 position 을 외부에서 변경 가능한 형태입니다. position 을 변경하는 책임은 Car 만 가지게 하는건 어떤가요?

kotlin 프로퍼티의 getter setter 에 대해 학습해보세요 :)
https://kotlinlang.org/docs/properties.html#getters-and-setters

position이 변경될 가능성이 많기 때문에 기본적으로 getter, setter를 적용하는 data class를 사용하였는데, 이러면 setter가 무분별하게 사용될 수 있겠네요. 외부에서 직접 변경할 수 없도록 수정하겠습니다.

SeongHoonC · 2024-08-11T11:36:05Z

src/main/kotlin/racingcar/view/GameInputer.kt

+        if (number < 0) {
+            throw IllegalArgumentException("음수는 허용되지 않습니다.")
+        }
+        return number


require 을 사용할 수 있을 것 같네요. IllegalArgumentException 은 어떤 상황에서 던지는 예외일까요?

require를 사용해서 보다 간결하게 작성하겠습니다!
또한 저는 사용자의 입력값이 시스템에서 유효하지 않을때 IllegalArgumentException을 발생시키도록 하였습니다.

SeongHoonC · 2024-08-11T11:40:51Z

src/main/kotlin/racingcar/model/Cars.kt

+    fun getCarList():List<Car>{
+        return carList.map{it.copy()}
+    }


이 코드에 대해 설명해주실 수 있나요?

일급 컬렉션에서 자동차 목록을 조회할때, 실제 객체가 변경될 것을 우려하여 복사본을 반환하려 하였으나 방법이 잘못됐다는 것을 깨달았습니다. 실제로는 리스트가 변경되지 않게 final과 같은 키워드로 막아두고, 메서드를 통해 변경을 적용할때 실제 객체를 반환하는 방법을 사용하는 것 같네요.

SeongHoonC · 2024-08-11T11:44:15Z

src/main/kotlin/racingcar/model/RandomDice.kt

+
+import kotlin.random.Random
+
+object RandomDice {


object 와 class 의 차이는 무엇일까요?

object는 코틀린에서 특정 클래스를 싱글톤으로 사용하기 위한 키워드로 알고 있습니다.
class와 달리 어디서든 사용되는 주사위는 같을 것으로 예상되어 싱글톤을 적용하였습니다.

확장에 유연하지 않아 이렇게 사용한다면 랜덤 함수를 직접 호출하는 것과 크게 다르지 않아보여요

SeongHoonC · 2024-08-11T11:48:24Z

src/main/kotlin/racingcar/controller/GameController.kt

+        val rounds = GameInputer.getRounds()
+
+        GameAnnouncer.printProcess()
+        for (i in 1..rounds) {


repeat 을 활용해 볼 수 있겠네요:)

유용한 키워드네요 :)

SeongHoonC · 2024-08-11T11:50:04Z

src/main/kotlin/racingcar/controller/GameController.kt

+        val carNum = cars.size
+        for (i in 1..carNum) {
+            if (RandomDice.rollDice() >= 4) {
+                cars.moveCar(i)
+            }
+        }


Controller 에 도메인 로직이 많아 보입니다. MVC 에서 비대해진 컨트롤러는 테스트하기 힘들고 재사용성이 떨어집니다. 관심사 분리를 통해 Model 과 View 를 연결해주는 책임만 가지도록 해보는건 어떤가요?

저도 고민했던 문제지만 어떻게 해야 할지 여전히 잘 모르겠습니다 ㅠ
저는 컨트롤러가 게임의 사회자라고 생각하고 있습니다. 그리고 게임이 시작한 후에 사회자는 한 라운드마다 주사위를 굴리고 자동차를 이동 시키는 책임을 가지고 있다고 생각하는데, 이 일을 어떤 클래스에게 위임해야 할까요..?

SeongHoonC · 2024-08-11T11:53:15Z

src/test/kotlin/study/utils/RandomDiceTest.kt

+    fun `주사위를 굴려 0~9사이의 정수를 반환한다`(){
+        //given
+
+        //when
+        val result = dice.rollDice()
+
+        //then
+        assertTrue(result in 0..9)
+    }


저는 보통 요렇게까지는 테스트하지 않는 편입니다. 랜덤 함수 자체를 테스트하는 느낌이 강해서요!

그렇군요! 동의합니다 :)

SeongHoonC · 2024-08-11T11:55:15Z

src/main/kotlin/racingcar/model/Cars.kt

+        val nameSet = mutableSetOf<String>()
+        for (car in carList) {
+            if (!nameSet.add(car.name)) {
+                throw IllegalArgumentException("중복된 차 이름이 있습니다 : ${car.name}")
+            }
+        }


중복 체크로 이런 방법도 있습니다. :)

Suggested change

val nameSet = mutableSetOf<String>()

for (car in carList) {

if (!nameSet.add(car.name)) {

throw IllegalArgumentException("중복된 차 이름이 있습니다 : ${car.name}")

}

}

val carNames = carList.map {it.name}.toSet()

require(carNames.size == carList.size) { "중복된 차 이름이 있습니다" }

헐 이런 방법이 있었네요 ㄷㄷ

SeongHoonC

추가 작업한 내용에 대해서는 코드리뷰 남겼습니다.
이번 미션으로 얻어갔으면 하는 것은 세 가지 입니다.

Model - View - Controller 가 각자 어떤 역할을 가지는지
객체의 관심사(책임)를 분리하고 테스트하기 쉬운 구조의 설계하는 것이 다형성을 높이고 객체의 유연한 협력으로 이어진다는 것
kotlin 기본 문법 및 코틀린스러운 코드 찍먹

세가지에 만족하지 못하신다면 반영 후 재요청하셔도 좋습니다.

자동차 경주 미션 수고하셨습니다..!

SeongHoonC · 2024-08-24T12:16:00Z

src/main/kotlin/racingcar/controller/GameController.kt

+    private fun resolveWinners(cars : Cars) : Cars{
+        val carList = cars.getCarList()
+        val maxPosition = carList.maxOf { it.position }
+        val winners = carList.filter { it.position == maxPosition }
+            .map { it }
+        return Cars(winners)
+    }


private 함수는 테스트하기 힘들죠.
이 부분도 심판 도메인 모델로 분리하면 관심사도 분리되고 및 테스트 용이성도 증가할 것 같아요.!

Cars 를 받아 Winners 를 구하는 책임을 누군가 가지는게 어떤가요!

아하 심판 도메인을 구현하는 부분 참 좋은 생각인것 같습니다!

SeongHoonC · 2024-08-24T12:18:18Z

src/main/kotlin/racingcar/model/Car.kt

+class Car (val name : String, position : Int = 0){
+    var position = position
+        private set


private set 👍

Due-IT added 27 commits August 6, 2024 14:13

feat : readme 작성

643b604

feat : 게임 흐름 코드 추가

1c60e62

feat : 게임 준비 컨트롤러 코드 작성

76ee82c

feat : 게임 준비 안내문구 출력 코드 작성

1332d36

feat : 게임 준비 입력기 코드 작성

480295e

feat : Cars객체 생성

c6b78b0

test : 자동차 이름을 입력받아 쉼표 기준으로 구분한다

ed68363

fix : 한글 메서드 역따옴표 사용 및 출력테스트는 삭제

540ba0c

test : 라운드 입력과 예외처리에 대한 테스트 코드 작성

4a8fa93

docs : readme에 수행한 작업을 체크

0aae47d

test : 자동차 객체 집합 생성 테스트 코드 작성

7322a9a

feat : 자동차 집합 생성 코드 작성

469efd7

test : 자동차의 위치를 출력한다.

e76d519

feat : 이동 및 위치 출력 코드 작성

128c5b2

test : 주사위를 굴려 한자리 정수를 반환한다

1886873

feat : 주사위를 굴려 1~9사이의 정수를 반환한다

c39982d

test : 특정 위치의 자동차를 한 칸 움직인다

fbe849d

feat : 테스트 코드 통과 완료

6da8a3d

feat : cars에 속한 car들의 위치 출력

21b64a2

fix : Cars를 일급 컬렉션 정의 규칙에 맞게 수정

a8747eb

feat&test : 우승한 차들의 이름을 출력한다.

b3dcfca

fix : 오로지 생성자만을 사용한 Cars객체 생성 및 전체적인 기능 작성 완료

73495d0

docs : 작업한 내용 readme 적용

ca514d0

refactor : mvc패턴을 고려하여 리팩토링

30bb064

refactor : GameInputer 위치 변경

0bdeb43

feat : 입력값 검증 기능 추가

a0631cf

docs : README 수정

77fff6b

Due-IT changed the title ~~[자동차 경주 step1] 최성훈 미션 제출합니다.~~ [자동차 경주 step1] 양두영 미션 제출합니다. Aug 9, 2024

SeongHoonC requested changes Aug 11, 2024

View reviewed changes

refactor : cars는 우승에 대한 개념과 출력에 대해 모른다.

7c200d5

Due-IT added 4 commits August 15, 2024 19:58

refactor : 우승자는 컨트롤러가 결정한다.

075ae77

fix : position은 외부에서 변경할 수 없다.

ea21bb6

refactor : require를 통해 입력값을 검사한다.

5157d6a

refactor : 중복검사 최적화

be3bcb1

SeongHoonC approved these changes Aug 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[자동차 경주 step1] 양두영 미션 제출합니다. #19

[자동차 경주 step1] 양두영 미션 제출합니다. #19

Due-IT commented Aug 9, 2024

SeongHoonC left a comment

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC Aug 11, 2024

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024 •

edited

Loading

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC Aug 16, 2024

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024 •

edited

Loading

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC Aug 11, 2024

Due-IT Aug 15, 2024

SeongHoonC left a comment

SeongHoonC Aug 24, 2024

SeongHoonC Aug 24, 2024

Due-IT Aug 24, 2024

SeongHoonC Aug 24, 2024

Due-IT Aug 24, 2024

		@@ -0,0 +1,17 @@
		package racingcar.model

		data class Car (val name : String, var position : Int = 0){

[자동차 경주 step1] 양두영 미션 제출합니다. #19

Are you sure you want to change the base?

[자동차 경주 step1] 양두영 미션 제출합니다. #19

Conversation

Due-IT commented Aug 9, 2024

SeongHoonC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Due-IT Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Due-IT Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeongHoonC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Due-IT Aug 15, 2024 •

edited

Loading

Due-IT Aug 15, 2024 •

edited

Loading