STEP 1 : 지뢰찾기(그리기) #386
STEP 1 : 지뢰찾기(그리기) #386
Conversation
| @@ -0,0 +1,10 @@ | |||
| package minesweeper.domain | |||
|
|
|||
| class Cell(var isMine: Boolean = false) { | |||
There was a problem hiding this comment.
이후에 지뢰와 아닌 블럭에 따라 분기 등 구현해야 할 책임이 늘어나면 Boolean 타입이 아니라 별도의 클래스로 분리하는것도 방법이 될 수 있을 것 같아요. 참고 부탁드립니다.
| isMine -> "*" | ||
| else -> "C" |
There was a problem hiding this comment.
*와 C는 ResultView에서 출력하기 위해 존재하는 것 같아요. 어떻게 보면 핵심 도메인 로직 내부에 클라이언트쪽 요구사항이 결합되었다고 볼 수 있습니다. 이후에 * 대신 -로 출력한다거나 하는 요구사항이 생기는 경우 이 toString 결과를 가져다 쓰는 곳이 있다면 영향 범위 파악이 어려워 코드를 수정하기 어려울 수 있겠죠.
저는 이러한 관점에서 Mine과 아닌것을 구분하는 로직이 존재하는것은 괜찮지만 그에 따른 출력 관련 부분이 비즈니스 로직 내부에 포함되는 것은 지양하는것이 좋다고 생각합니다.
|
|
||
| val board: Array<Array<Cell>> |
There was a problem hiding this comment.
Array가 아니라 조금 더 풍부한 표준 메서드를 제공하는 List를 쓰는 것은 어떨까요?
| data class GameBoard(private val _height: Int, private val _width: Int) { | ||
| val height get() = _height | ||
| val width get() = _width |
There was a problem hiding this comment.
val로 정의된 값들은 setter를 통해 수정할 수 없습니다. getter를 통해 가져가는것은 열려있는 상태인데요. Backing Properties를 사용하지 않고 그냥 private val로 정의하는건 어떨까요?
| require(_height > 0 && _width > 0) { | ||
| "자연수를 입력해주세요." | ||
| } |
There was a problem hiding this comment.
둘 중 하나만 자연수를 벗어난 경우 어떤 값이 자연수가 아니어 실패했는지 파악이 어려울 것 같아요. height 및 width에 대한 값도 예외 메시지에 남기는건 어떨까요?
| import kotlin.random.Random | ||
|
|
||
| class MineGenerator(private val gameBoard: GameBoard, private val mineCount: Int) { | ||
| fun generateRandomPoints(): List<Point> { |
There was a problem hiding this comment.
여기서 반환된 좌표의 리스트가 마인을 나타내는건지를 알 수 있는 방법은 변수명 밖에 없을 것 같아요.
List<Point>를 묶어 지뢰의 좌표들을 나타낸다는 의도를 드러내고 생성 책임 또한 그쪽으로 옮기면 Generator의 책임을 그쪽으로 옮겨 클래스를 줄이고 지뢰의 좌표 리스트에 대한 의도를 명확히 나타낼 수 있지 않을까요? 한번 고민해보시면 좋겠습니다.
안녕하세요 리뷰어님
지뢰찾기 1단계 그리기 PR입니다!
잘부탁드립니다 😀