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

Step3 지뢰찾기(게임실행) #399

Open
wants to merge 7 commits into
base: jaylene-shin
Choose a base branch
from

Conversation

jaylene-shin
Copy link

@jaylene-shin jaylene-shin commented Dec 3, 2023

윤혁님, 안녕하세요. step3 리뷰 요청드립니다!

작업 내용

  1. step2 리뷰 반영
  2. Cell을 sealed interface로 선언(openState 추상 프로퍼티와 open 추상 메서드 제공)
  3. Cell이 open 상태로 변경된 경우 MineMap의 Cell을 교체하도록 수정
  4. 랜덤 위치를 자동으로 선택해 지뢰 찾기 게임을 수행하고, 지뢰를 밟으면 게임을 종료하도록 처리

질문

객체 간 의존도가 높아질 수록 테스트가 어려워지는 상황을 어떻게 개선해볼 수 있을까요? ( 관련 질문 )

Copy link
Member

@malibinYun malibinYun left a comment

Choose a reason for hiding this comment

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

리뷰가 늦어져서 죄송합니다 😥
3단계 미션 고생 많으셨어요!
고민해보면 좋을 점들에 코멘트 남겨두었어요.
이전 미션에 달아두신 코멘트에도 답변 달아둘게요 :)
피드백 반영 후 다시 리뷰 요청 부탁드려요!

Comment on lines +21 to +33
private fun executeGame(mineMapMeta: MineMapMeta, mineMap: MineMap) {
val positionStack = Stack<Position>().apply { addAll(mineMap.values.keys) }
while (positionStack.isNotEmpty()) {
val position = positionStack.pop()
if (mineMap.getCell(position).openState == OpenState.OPENED) continue
OutputView.printOpenPositionMsg(position)
if (mineMap.isEmptyCellClicked(position)) {
OutputView.printMineMap(mineMapMeta, mineMap)
continue
}
OutputView.printGameLoseMsg()
break
}
Copy link
Member

Choose a reason for hiding this comment

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

view와 게임을 진행하는 도메인 로직이 섞여있는 것으로 보여요 :)
view의 일과 도메인의 일을 적절히 나눠보는 것은 어떨까요?

도메인에서 view의 일이 필요하다면, 전달 받아야 할 것들에 대해 interface를 작성하고, console에 대한 하위 구현체를 도메인 객체로 넘겨 의존 관계를 떼어낼 수 있겠어요 :)

Comment on lines +14 to 18
private fun createCell(position: Position, minePositions: Set<Position>): Cell {
val aroundPositions = position.aroundPositions()
return if (minePositions.contains(position)) {
Mine
Mine()
} else {
Copy link
Member

Choose a reason for hiding this comment

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

지뢰 판의 칸들이 열릴지 여부를 모르는 상태에서 미리 count를 모두 계산하는 것은 낭비라는 생각이 들어요.
칸이 열렸을 때, 그 때 count를 계산하게 만들어보는 것은 어떨까요?

Comment on lines +14 to +19
data class Empty(
val mineCount: Int = 0,
override val openState: OpenState = OpenState.CLOSED,
) : Cell {
override fun open(): Empty {
return copy(openState = OpenState.OPENED)
Copy link
Member

Choose a reason for hiding this comment

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

Cell 하위 구현체로, Open인 Cell과 Close인 셀로 구현해보는 건 어떨까요?

Comment on lines +12 to +13

fun isEmptyCellClicked(position: Position): Boolean {
Copy link
Member

Choose a reason for hiding this comment

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

메서드 명만 봐서는, 빈 칸이 클릭되었는지에 대한 여부를 반환하기만 할 것처럼 보여요.
그런데 실제로 내부 구현에서는 칸을 열기까지 하네요!
다른 개발자들이 보았을 때 이 메서드에 대한 내용을 착각하고 잘못 사용할 수 있을 것으로 보여요.
메서드가 수행하고 있는 것을 더욱 구체적으로 작성해보는 건 어떨까요?

Comment on lines +8 to +9
val rowRange = (y - 1)..(y + 1)
val colRange = (x - 1)..(x + 1)
Copy link
Member

Choose a reason for hiding this comment

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

#389 (comment) 여전히 유효한 피드백이에요!
enum으로 Direction 등의 객체를 만들어보는 건 어떨까요? 그리고 이 객체로 8방향을 표현해보면 좋겠어요 :)

enum class Direction(
    val dx :Int,
    val dy: Int,
) {
    TOP(0, 1)
    TOP_LEFT(-1, 1)
...
...
}

Comment on lines 17 to 18
require(minePositions.size == mineMapMeta.mineCount) { "지뢰의 개수가 맞지 않습니다." }
return minePositions
Copy link
Member

Choose a reason for hiding this comment

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

#389 (comment) 여전히 유효한 피드백입니다 :)

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