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

1단계 - 지뢰 찾기(그리기) #393

Open
wants to merge 9 commits into
base: oeeen
Choose a base branch
from
Open

Conversation

oeeen
Copy link

@oeeen oeeen commented Dec 2, 2023

안녕하세요 리뷰어님

기간이 하루 남았지만.. 마지막 미션 리뷰 요청 드립니다!

@oeeen oeeen changed the base branch from main to oeeen December 2, 2023 13:10
Copy link

@laco-dev laco-dev left a comment

Choose a reason for hiding this comment

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

안녕하세요, 지뢰찾기 미션까지 잘 와주셨네요
지금까지 미션을 해 오신것처럼, 협력이나 객체 분리에 대한 모습이 보여서 좋았습니다.

다만 이름?과 이 객체들을 사용하는 협력 관계에 있어서는 조금 아쉬운 부분이 있어요.
관련해서 의견 남겨드렸으니 충분히 고민 해 보세요

@@ -0,0 +1,13 @@
package minesweeper

import minesweeper.domain.*
Copy link

Choose a reason for hiding this comment

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

와일드 카드 import는 꺼주시는게 좋습니다!
다른 파일에서도 모두 확인 해주세요 🙏🏻

@@ -0,0 +1,7 @@
package minesweeper.domain

data class Width(val value: Int) {
Copy link

Choose a reason for hiding this comment

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

단순한 값을 표현할 때에는 value class를 활용 해보셔도 좋습니다.
https://kotlinlang.org/docs/inline-classes.html

@@ -0,0 +1,5 @@
package minesweeper.domain

enum class State(val display: String) {
Copy link

Choose a reason for hiding this comment

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

지뢰 게임이라는 도메인에서 State 라는 타입 이름만 본다면, 무엇 이라고 생각되시나요?
저는 게임 전체적인 진행 상태로 생각이 듭니다.

Cell 이라는 타입 자체가 지뢰 셀, 지뢰가 아닌 셀로 개념을 공유할 수 있지 않을까요?

package minesweeper.domain

enum class State(val display: String) {
MINE("*"), CELL("C")
Copy link

Choose a reason for hiding this comment

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

display 값은 UI의 관심사입니다.
가령 지뢰 모양을 별이 아닌 💣 으로 한다면, 도메인 모델의 수정이 불가피합니다.

@@ -0,0 +1,9 @@
package minesweeper.domain

class BoardInformation(
Copy link

Choose a reason for hiding this comment

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

Information 이라는 의미는 가급적 사용을 지양하면 좋은데요.
정보라는 이름이 어떤 역할을 하는지 모호하게 만들고, 유틸처럼 느껴져서 필요한 모든 내용을 가지는 것처럼 느껴집니다.

Width, Height 을 가지는 Size 라는 개념을 만들어 봐도 좋구요.
지뢰의 경우는 보드가 Cell을 가지고 있다면 별도의 프로퍼티가 아닌 가지고 있는 것으로도 확인이 가능 해 보이네요

Comment on lines +4 to +6
override fun compareTo(other: Cell): Int {
return compareValuesBy(this, other, { it.point.x }, { it.point.y })
}
Copy link

Choose a reason for hiding this comment

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

셀을 비교하는것에 있어서 위치값으로 단순히 크고 작다가 결정될 수 있을까요?
위치를 찾기 편하게 하려고 만들어 진 것이라면, 다른 사람들도 이해할 수 있는 개념일까? 를 고민 하면 좋습니다.


import java.util.*

class Row(val cells: SortedSet<Cell>) : Comparable<Row> {
Copy link

Choose a reason for hiding this comment

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

Row 라는 이름도 마찬가지로 Cells을 가지고 있다는 느낌이 잘 들지 않아요.
값 객체러럼 몇 행, 몇 열 을 나타내는 Row, Column 처럼 느껴집니다.

이 객체는 Cells 를 표현하고 싶었던 것일까요?

Comment on lines +3 to +5
fun interface MinePlacementStrategy {
fun placeMine(rows: List<Row>, mineCount: Int): List<Row>
}
Copy link

Choose a reason for hiding this comment

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

지뢰를 심기위한 전략을 인터페이스로 역할 분리를 잘 해주셨네요. 👍🏻
다만 현재 구조에서는 이 인터페이스를 사용할 때에 있어의 사용 방법이 다소 까다롭다고 느껴지는데요.

1. Cell을 준비한다
2. Row를 준비한다
3. Rows를 준비한다.
4. 지뢰 갯수를 준비한다.
5. placeMine()을 호출한다.

완성된 셀에서 지뢰를 움직이는 방법 외에도, 단순히 지뢰가 심어질 위치값들만 받아온다거나 할 수도 있지 않을까요?
보드를 생성할 때 필요한 규칙이니, 생성 이후에는 사용되지 않아서요

private val minePlacementStrategy: MinePlacementStrategy = RandomMinePlacementStrategy()
) {
val mineMap: List<Row>
get() = generateMap()
Copy link

Choose a reason for hiding this comment

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

mineMap은 커스텀 getter()로 작성되었습니다.
그렇다면 아래 코드의 mineMap은 모두 같은 결과를 만들어낼까요?

val board: Board
board.mineMap  // (1)
board.mineMap  // (2)
board.mineMap  // (3)
board.mineMap  // (4)
board.mineMap  // (5)

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