Step1 지뢰 찾기(그리기) #377

jaylene-shin · 2023-11-28T22:21:44Z

윤혁님 안녕하세요, 처음 뵙겠습니다!!
step1 미션 리뷰 요청드립니다.

리뷰 중점 사항

지뢰가 위치할 (랜덤) 좌표와 빈 좌표 정보를 각각 생성하여 지뢰판을 구성하였습니다. 현재 방식이 지뢰판 생성 중 발생 가능한 반복 탐색(e.g. 지뢰판의 모든 좌표가 랜덤하게 생성된 지뢰 좌표와 중복되는지 탐색. O(height*width))을 최소화하는 방식이라고 판단하였는데요...! 옳은 판단인지 피드백을 받고 싶습니다!
지뢰판의 map은 객체 내에서는 가변이지만 외부에서는 불변을 보장할 수 있도록 방어적 복사를 활용하였습니다. 추후 스레드 안정성을 고려했을 때 현재 방식은 최선의 방식일지 확인 부탁드립니다!

- [x] 지뢰와 지뢰가 아닌 칸을 구분하기 위한 문자 및 클래스를 생성한다.

- [x] 지뢰는 주어진 지뢰 개수만큼 랜덤으로 배치한다. - [x] `OutputView` 생성: 생성된 지뢰판을 출력한다.

malibinYun

안녕하세요 :) 1단계 미션 고생 많으셨어요!
생각해볼만한 곳에 코멘트 남겨두었어요.
다음 미션 진행을 위해 머지할게요!
피드백은 다음 미션에 반영해주세요!

malibinYun · 2023-11-29T16:16:26Z

src/main/kotlin/minesweeper/domain/Cell.kt

+data class Cell(
+    val state: CellState
+)


Cell class가 별 다른 역할 없이 CellState 를 가지고만 있는 걸로 보여요.
CellState자체가 Cell로 표현되어도 좋겠어요!

혹은 이후 한 칸에 대한 객체를 더 표현하기 위함이었다면, sealed class를 활용해보셔도 좋겠네요 :)

칸 객체를 표현할 수 있도록 sealed interface인 Cell 을 생성하고
Mine과 Empty를 하위 클래스로 구현하였습니다!

malibinYun · 2023-11-29T16:16:35Z

src/main/kotlin/minesweeper/domain/MineMap.kt

+class MineMap(
+    private val _values: MutableMap<Position, Cell> = mutableMapOf()
+) {
+    val values: Map<Position, Cell>
+        get() = _values


지뢰판의 map은 객체 내에서는 가변이지만 외부에서는 불변을 보장할 수 있도록 방어적 복사를 활용하였습니다.

방어적 복사를 활용했다고 보기에는 어려워보여요.
생성자에서 MutableCollection을 받을 때, 다른 개발자가 외부의 참조를 실수로 다뤘을 때, MineMap 객체 내부가 변경되는 상황이 발생할 수 있어요.
아래 코드를 보시면 쉽게 이해하실 수 있을거예요 :)

val mutableMap = mutableMapOf() val mineMap = MineMap(mutableMapOf()) mutableMap.clear()

생성자에서 전달 받은 컬렉션을 복사해서 멤버변수에 할당하는 것은 어떨까요?

앗 네네 설명해주셔서 감사합니다..! 방어적 복사는 외부 참조를 끊어내는게 핵심인데 위 코드에서는 끊어내질 못하고 있네요..!
(step2 최종 코드에는 제거되어 있지만) 방어적 복사를 올바르게 활용하도록 수정하였습니다!

class MineMap( values: Map<Position, Cell> = mapOf() ) { private val _values = values.toMutableMap() val values: Map<Position, Cell> get() = _values.toMap() // 이하 생략 }

malibinYun · 2023-11-29T16:16:38Z

src/main/kotlin/minesweeper/domain/MineMapGenerator.kt

+        val mineMap = MineMap()
+        minePositions.forEach { mineMap.plantCell(it, Cell(CellState.MINE)) }
+        emptyPositions.forEach { mineMap.plantCell(it, Cell(CellState.EMPTY)) }


plantCell을 호출함으로써 지뢰판을 초기화 하기보다,
지뢰판을 생성할 때 생성자에 필요한 모든 정보를 넘겨 만들어내는 것은 어떨까요?

좋습니다!
plantCell을 활용하다보니 MineMap의 프로퍼티를 mutable로 써야했었는데요. 이 문제를 같이 해결할 수 있을 것 같습니다..!
피드백 주신 방향으로 수정 완료했습니다!

malibinYun · 2023-11-29T16:16:43Z

src/main/kotlin/minesweeper/domain/MineMapMeta.kt

+        require(height > 0) { "높이는 0 이거나 음수일 수 없습니다" }
+        require(width > 0) { "너비는 0 이거나 음수일 수 없습니다" }
+        require(mineCount > 0) { "지뢰 개수는 0 이거나 음수일 수 없습니다" }
+        require(height * width >= mineCount) { "지뢰 개수는 (높이 x 너비) 개수를 초과할 수 없습니다" }


어떤 값이 들어와서 예외가 발생했는지 입력값도 메시지에 같이 표시해주면 디버깅할 때 더욱 도움이 될 것으로 기대돼요 :)

트러블슈팅에 훨씬 유리하겠네요..! 수정하였습니다!

malibinYun · 2023-11-29T16:16:57Z

src/main/kotlin/minesweeper/domain/PositionGenerator.kt

+    tailrec fun generateMinePositions(
+        minePositions: Positions = Positions()
+    ): Positions {
+        if (minePositions.size == mineMapMeta.mineCount) { return minePositions }
+        val randomMinePosition = positionSelector.select(mineMapMeta)
+        return if (randomMinePosition in minePositions) {
+            generateMinePositions(minePositions)
+        } else {
+            generateMinePositions(minePositions + randomMinePosition)
+        }
+    }


지뢰가 위치할 (랜덤) 좌표와 빈 좌표 정보를 각각 생성하여 지뢰판을 구성하였습니다. 현재 방식이 지뢰판 생성 중 발생 가능한 반복 탐색(e.g. 지뢰판의 모든 좌표가 랜덤하게 생성된 지뢰 좌표와 중복되는지 탐색. O(height*width))을 최소화하는 방식이라고 판단하였는데요...! 옳은 판단인지 피드백을 받고 싶습니다!

운이 좋지 않으면 굉장히 많이 실행되어서 시간이 꽤 걸릴지도 모르겠어요.

아래 형태로 접근 방식을 바꿔보는 건 어떨까요?
로직적인 것만 참고해주시고, 이름은 지영님이 잘 지어보시면 좋겠어요 :)

val allPosition = ~~ val minePosition = RandomSelector.select(allPosition, 개수) // 전달 받은 positions을 shuffle하고, 개수만큼 take() 한positions을 반환 val cells = minePosition.map { Mine() } + (allPosition - minPosition).map { Block() } ~~

재귀 써야한다는 생각에 빠져있었는데, shuffle-take 연산이 훨씬 효율적이겠네요..!! 감사합니다!
수정하였습니다!

malibinYun · 2023-11-29T16:17:03Z

src/main/kotlin/minesweeper/domain/Positions.kt

+class Positions(
+    private val positions: Set<Position> = emptySet()
+) : Set<Position> by positions {


일급 컬렉션을 kotlin delegate를 활용해서 작성해주셨네요!
delegate를 활용하면 외부에서 활용하지 않아도 될 컬렉션의 api 조차도 모두 공개되기 때문에 일급 컬렉션을 만드는 의미가 희석된다고 생각해요.
필요한 기능들만 외부에 공개해보는 건 어떨까요?

사용하는 것은 편리해질 수 있겠지만, Collection을 래핑만 한 것과 크게 다르지 않다는 생각이에요 :)

기존에는 내부 프로퍼티를 바로 순회할 수 있게 delegate을 사용했었는데요.
불필요한 컬렉션 api 를 모두 공개한다면, 일급 컬렉션의모든 행위를 내부에서 관리한다는 목적이 희석될 수 있겠네요...!
좋은 피드백 감사합니다. 👍
delegate 제거하도록 수정하였습니다!

malibinYun · 2023-11-29T16:17:07Z

src/test/kotlin/minesweeper/domain/MineMapGeneratorTest.kt

+        // then
+        assertEquals(mineMapMeta.getCellCount(), mineMap.values.size)


테스트 검증문은 assertJ 혹은 kotest를 활용해보는 건 어떨까요?

가독성 확보와 메서드 체이닝 활용 측면에서 더 유리할 것 같습니다!
step2에서는 assertJ의 assertThat().isEqualTo()... 방식으로 수정하였습니다! (예시)

malibinYun · 2023-11-29T16:17:11Z

src/test/kotlin/minesweeper/domain/MineMapTest.kt

+        // then
+        assertEquals(2, mineMap.size)
+        assertEquals(CellState.EMPTY, mineMap.values[Position(1, 1)]?.state)
+        assertEquals(CellState.EMPTY, mineMap.values[Position(2, 2)]?.state)


assert 구문을 나열하는 것과 assertAll로 감싸는 것은 어떤 차이점이 있을까요?

assert 구문 나열 시 실패한 구문 이후로 테스트가 수행되지 않을 것 같습니다..!
assertSoftly를 도입해 위 문제를 해결하였습니다!

jaylene-shin added 6 commits November 28, 2023 14:51

- [x] 지뢰판 위치 정보를 프로퍼티로 가진 DTO를 생성한다.

4838734

- [x] 지뢰와 지뢰가 아닌 칸을 구분하기 위한 문자 및 클래스를 생성한다.

InputView로 받은 정보 맵핑할 DTO 생성

9986492

지뢰판 생성을 위한 객체를 생성한다. (InputView로 받은 DTO 객체 활용)

649bff4

- [x] InputView 생성: 사용자로부터 높이와 너비, 지뢰 개수를 입력받고 DTO 객체로 변환한다.

6a5cb80

- [x] 지뢰는 주어진 지뢰 개수만큼 랜덤으로 배치한다. - [x] `OutputView` 생성: 생성된 지뢰판을 출력한다.

Cell에서 position 제거

e69f6e7

일급 컬렉션 Positions 생성

1b56c4f

malibinYun approved these changes Nov 29, 2023

View reviewed changes

malibinYun merged commit e58cc6c into next-step:jaylene-shin Nov 29, 2023

jaylene-shin mentioned this pull request Dec 2, 2023

Step2 지뢰 찾기(지뢰 개수) #389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Step1 지뢰 찾기(그리기) #377

Step1 지뢰 찾기(그리기) #377

jaylene-shin commented Nov 28, 2023 •

edited

Loading

malibinYun left a comment

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023 •

edited

Loading

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023 •

edited

Loading

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023 •

edited

Loading

malibinYun Nov 29, 2023

jaylene-shin Dec 2, 2023

		// then
		assertEquals(mineMapMeta.getCellCount(), mineMap.values.size)

Step1 지뢰 찾기(그리기) #377

Step1 지뢰 찾기(그리기) #377

Conversation

jaylene-shin commented Nov 28, 2023 • edited Loading

malibinYun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaylene-shin Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaylene-shin Dec 2, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaylene-shin Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaylene-shin commented Nov 28, 2023 •

edited

Loading

jaylene-shin Dec 2, 2023 •

edited

Loading

jaylene-shin Dec 2, 2023 •

edited

Loading

jaylene-shin Dec 2, 2023 •

edited

Loading