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단계 - 지뢰 찾기(그리기) #367

Merged
merged 5 commits into from
Nov 29, 2023
Merged

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

merged 5 commits into from
Nov 29, 2023

Conversation

jdalma
Copy link

@jdalma jdalma commented Nov 26, 2023

안녕하세요 ㅎㅎ 지뢰찾기 게임 리뷰 잘 부탁드립니다!

Copy link

@MyStoryG MyStoryG left a comment

Choose a reason for hiding this comment

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

현준님 안녕하세요!
지뢰 찾기 미션을 함께 하게 된 서준수입니다.
1단계 미션 잘 구현해 주셨네요! 👍
몇 가지 의견을 남겼는데 확인 후 다시 리뷰 요청 부탁 드릴게요!

Comment on lines 11 to 18
Output.printAny(Message.INPUT_HEIGHT)
val height: String = Input.getLine()

Output.printAny(Message.INPUT_WIDTH)
val width: String = Input.getLine()

Output.printAny(Message.INPUT_MINES)
val mineCount: String = Input.getLine()

Choose a reason for hiding this comment

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

아래 요구 사항을 참고하여 입력받는 값들을 포장해 보면 어떨까요?
그리고 원시 값을 포장하면 어떤 장점이 있을까요?

모든 원시 값과 문자열을 포장한다.

Copy link
Author

@jdalma jdalma Nov 28, 2023

Choose a reason for hiding this comment

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

Width와 Height 그리고 Mines까지 value class로 포장해봤습니다 ㅎㅎ
문맥상 편하게 보기 위해 minus operator를 구현했는데.. 현재 원시 값을 포장하는 것의 장점이 있을까라는 생각이 들었습니다
Width와 Height에 추가적으로 책임이 필요하다고 판단될 때가 아니면 미리 포장할 생각을 안할 것 같습니다

모든 곳에 모든 원시 값과 문자열을 포장한다. 이 법칙을 항상 적용해야할까 생각이 드는데 리뷰어님은 어떤 장점이 있다고 생각하시는지 궁금하네요 ㅎㅎ

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

data class Mines(

Choose a reason for hiding this comment

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

data class를 사용해 주셨네요.
이 경우엔 value class를 사용해 보면 어떨까요?
그러면 어떤 장점이 있을까요?

Copy link
Author

@jdalma jdalma Nov 28, 2023

Choose a reason for hiding this comment

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

불필요한 객체 생성을 피할 수 있는 것으로 알고 있습니다 ㅎㅎ

Comment on lines 3 to 9
object Message {

const val INPUT_HEIGHT = "높이를 입력하세요."
const val INPUT_WIDTH = "너비를 입력하세요."
const val INPUT_MINES = "지뢰는 몇 개인가요?"
const val OUTPUT_START_MINESWEEPER = "지뢰찾기 게임 시작"
}

Choose a reason for hiding this comment

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

이렇게 상수를 모아 놓은 오브젝트를 구현하셨네요!
이런 방법도 좋지만, 실제로 해당 문구를 사용하는 객체에 직접 선언되면 어떨까요?
그러면 중복이 발생할 수 있다는 단점이 있을 수 있지만 객체의 역할이 분명해 진다는 장점이 생길 것 같아요.
결합도가 낮아지고 응집도는 올라가겠죠! 각각 장단점이 있기 때문에 제안 드려보아요.

Comment on lines 16 to 20
else generate(
positions.apply {
this.add(randomPosition.generate())
}
)

Choose a reason for hiding this comment

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

이번 미션에서는 아래와 같은 프로그래밍 요구 사항이 있습니다.
한번 도전해 보시는 게 어떨까요?

else 예약어를 쓰지 않는다.

import minesweeper.RandomPosition
import view.Input
import view.Output
import kotlin.math.min

Choose a reason for hiding this comment

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

불필요한 import는 제거해도 좋을 것 같아요!
push 전에 터미널에서 ./gradlew clean check 명령어로 한번 확인해 보세요.

Comment on lines 18 to 24
val (row, col) = minesweeperBoard.gameBoard.let {
it.height to it.width
}
val map = Array(row) { Array(col) { 'C' } }
minesweeperBoard.mines.mines.forEach {
map[it.x][it.y] = '*'
}

Choose a reason for hiding this comment

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

이 부분이 View에 있네요. 생각하기에 따라 View 로직이라고 볼 수도 있겠지만 저는 도메인 로직에서 처리한 후에 View에서는 처리된 데이터를 받아와서 단순하게 출력해 주는 방향이 더 어울린다고 생각해요. 어떻게 생각하시나요?

Copy link
Author

Choose a reason for hiding this comment

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

이때까지 출력을 위한 필드 접근을 위해 도메인 필드들을 public으로 열어놓고 출력 클래스에서만 접근하고 다른 도메인 객체는 접근하지 않게 작성했었는데 이번에는 출력을 위한 준비를 도메인 객체 내부에서 하도록 해보겠습니다 ㅎㅎ

항상 고민이 되는 부분인 것 같습니다. 리뷰어님은 내부 필드들을 private으로 감추고 출력을 위한 준비를 도메인 객체 내부에서 하도록 하는 것도 괜찮다고 생각하시는지 궁금하네요 ㅎㅎ

Choose a reason for hiding this comment

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

상황에 따라 다를 수 있겠지만 지금처럼 명확히 보드에 대한 데이터를 도메인 로직에서 가공할 수 있다면 도메인 객체 내부에서 하는 게 좋다고 생각합니다. 뷰에서 그러한 로직을 담당한다면 매번 해당 데이터를 출력할 때 그 로직을 구현해야 합니다. 중복 사용하지 않기 위해서 별도의 함수로 분리하면 되겠지만 그러면 그걸 다시 클래스로 분리할 수 있을 것이고 그러면 그건 결국 뷰에 있어야 할 객체가 아니라 도메인으로 옮겨지게 되겠죠.

1. Width, Height, Mines 값 클래스로 포장
2. 상수 문자열을 관리하는 Message 클래스 제거하고 필요한 클래스 내부로 이동
3. 출력을 위한 게임판 준비를 Output 클래스에서 제거
Comment on lines +11 to +14
private const val INPUT_HEIGHT = "높이를 입력하세요."
private const val INPUT_WIDTH = "너비를 입력하세요."
private const val INPUT_MINES = "지뢰는 몇 개인가요?"
private const val OUTPUT_START_MINESWEEPER = "지뢰찾기 게임 시작"

Choose a reason for hiding this comment

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

여기보다는 실제 해당 문구를 사용하는 뷰에 있으면 좋을 것 같아요.
해당 뷰에 있어야 의존없이 그 뷰의 역할을 다할 수 있으니까요.

Comment on lines 4 to 5
val gameBoard: GameBoard,
val mines: Mines

Choose a reason for hiding this comment

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

private으로 하면 되겠네요!

Comment on lines 18 to 24
val (row, col) = minesweeperBoard.gameBoard.let {
it.height to it.width
}
val map = Array(row) { Array(col) { 'C' } }
minesweeperBoard.mines.mines.forEach {
map[it.x][it.y] = '*'
}

Choose a reason for hiding this comment

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

상황에 따라 다를 수 있겠지만 지금처럼 명확히 보드에 대한 데이터를 도메인 로직에서 가공할 수 있다면 도메인 객체 내부에서 하는 게 좋다고 생각합니다. 뷰에서 그러한 로직을 담당한다면 매번 해당 데이터를 출력할 때 그 로직을 구현해야 합니다. 중복 사용하지 않기 위해서 별도의 함수로 분리하면 되겠지만 그러면 그걸 다시 클래스로 분리할 수 있을 것이고 그러면 그건 결국 뷰에 있어야 할 객체가 아니라 도메인으로 옮겨지게 되겠죠.

Copy link

@MyStoryG MyStoryG left a comment

Choose a reason for hiding this comment

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

현준님 안녕하세요!
피드백 반영 고생하셨습니다.
코멘트로 질문 하신 내용은 코멘트로 답변 드렸습니다!
빠른 진행을 위해 이번 단계는 머지하도록 하겠습니다.
간단한 코멘트 남긴 것이 있는데 다음 미션에 같이 반영해 주세요.

@MyStoryG MyStoryG merged commit 871bc3d into next-step:jdalma Nov 29, 2023
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.

None yet

2 participants