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

[step1] minesweeper(draw) #437

Merged
merged 15 commits into from
Dec 14, 2024
Merged

[step1] minesweeper(draw) #437

merged 15 commits into from
Dec 14, 2024

Conversation

jihoi-kang
Copy link

안녕하세요.
이번 미션동안 리뷰 잘 부탁드립니다🙏

Copy link

@BeokBeok BeokBeok left a comment

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 7
class Board(
height: Int,
width: Int,
mineCount: Int,
) {

Choose a reason for hiding this comment

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

프로그래밍 요구사항에는 아래와 같은 항목이 있습니다.

3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.

이 요구사항을 반영해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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


object BoardLinesGenerator {

fun generate(height: Int, width: Int, mineCount: Int): BoardLines {

Choose a reason for hiding this comment

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

내가 원하는 좌표에 지뢰를 심을 수 있는지 테스트 코드로 확인해볼 수 있도록 개선해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

1e6b1ec
BoardLinesGeneratorTest 에서 지뢰를 내가 원하는 위치에 심는 로직 테스트 해보는데 mineCount를 7이 아닌 다른 값으로 넣어도 테스트가 통과가 됩니다.

mineCount도 Param이 가지고 있어야 하고 내가 원하는 만큼 원하는 위치에 지뢰를 심어야 하다보니 둘 사이에 괴리가 생겨난거 같습니다. 혹시 관련하여 좋은 방법이 있을까요?

Choose a reason for hiding this comment

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

@jihoi-kang
mineCount를 Param에서 꼭 가져야하는 정보일까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

@BeokBeok
큰 힌트가 되었습니다. 감사합니다🙏🙏


fun showAndGetMineCount(): Int {
println("지뢰는 몇 개인가요?")
return readln().toInt()

Choose a reason for hiding this comment

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

너비와 높이를 곱한 값보다 큰 수치의 값을 입력하게 되면 어떻게 될까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

@BeokBeok BeokBeok left a comment

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 7
data class BoardParam(
val height: Int,
val width: Int,
val mineCount: Int,
) {

Choose a reason for hiding this comment

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

프로그래밍 요구사항에는 아래와 같은 항목이 있습니다.

3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.

이 요구사항을 반영해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

Comment on lines 13 to 14
if (position in minePositions) Cell.Mine
else Cell.Island

Choose a reason for hiding this comment

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

프로그래밍 요구사항에는 아래와 같은 항목이 있습니다.

else 예약어를 쓰지 않는다.

이 요구사항을 반영해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

3d2f7a2
프로그래밍 요구사항을 제대로 체크 못하였네요. 인지하여 작업하겠습니다🙏

Comment on lines 9 to 16
return BoardLines(List(boardParam.height) { row ->
BoardLine(List(boardParam.width) { col ->
val position = row * boardParam.width + col

if (position in minePositions) Cell.Mine
else Cell.Island
})
})

Choose a reason for hiding this comment

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

프로그래밍 요구사항에는 아래와 같은 항목이 있습니다.

한 메서드에 오직 한 단계의 들여쓰기만 한다.

이 요구사항을 반영해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,15 @@
package minsweeper.domain

data class BoardParam(

Choose a reason for hiding this comment

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

BoardParam 라는 이름만 봤을 때에는 어떤 것을 의미하는지 잘 모르겠는데요.
이름만 보고서 의미를 이해할 수 있도록 개선해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

addd195
BoardSize로 이름 명명하고 mineCount 삭제하였습니다


object BoardLinesGenerator {

fun generate(height: Int, width: Int, mineCount: Int): BoardLines {

Choose a reason for hiding this comment

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

@jihoi-kang
mineCount를 Param에서 꼭 가져야하는 정보일까요? 🤔

Copy link

@BeokBeok BeokBeok left a comment

Choose a reason for hiding this comment

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

피드백 반영하시느라 고생하셨습니다. 👍
추가로 고민해볼만한 의견들을 코맨트로 작성하였으니, 충분히 고민해보시고 도전해보세요. 💪
다음 미션을 진행하시면서, 추가로 작성된 코맨트들도 반영해주세요. 🙏

Comment on lines +7 to +11
fun of(
boardSize: BoardSize,
mineCount: Int,
boardLinesGenerator: BoardLinesGenerator,
): Board = Board(boardLinesGenerator.generate(boardSize, mineCount))

Choose a reason for hiding this comment

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

Board 객체 생성 방법은 생성자를 통한 방식과 팩토리 메서드를 통한 방식 두 가지가 있습니다.
팩토리 메서드를 통해서만 객체를 생성하시는 것으로 보이는데, 그렇다면 생성자를 통한 방식으로 객체를 생성하는 것은 제한해보면 어떨까요?

Comment on lines +4 to +7
val height: Int,
val width: Int,
) {
val area: Int = height * width

Choose a reason for hiding this comment

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

프로그래밍 요구사항에는 아래와 같은 항목이 있습니다.

3개 이상의 인스턴스 변수를 가진 클래스를 쓰지 않는다.

이 요구사항을 반영해보면 어떨까요?

class BoardLinesGeneratorTest {

@Test
fun `지뢰를 내가 원하는 위치에 심을 수 있다`() {

Choose a reason for hiding this comment

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

👍

@BeokBeok BeokBeok merged commit 208d6e7 into next-step:jihoi-kang Dec 14, 2024
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