Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Step2 - 지뢰 찾기(지뢰 개수) #413

Merged
merged 10 commits into from
Dec 18, 2023
Merged

Conversation

oyj7677
Copy link

@oyj7677 oyj7677 commented Dec 9, 2023

Step1 피드백
MapInfo 유효성 검사
MapInfo를 생성할 때 유효성 검사를 하는 것으로 수정하였습니다.

랜덤 로직
해당 로직이 프로퍼티로 생성될 경우 지뢰가 생성되는 로직에 대한 테스트를 하지 못할 것 같습니다.
프로퍼티로 구성되었을 때 테스트 로직을 어떤식으로 구현되어야 하는지 알려주시면 감사드리겠습니다.

Cell 클래스에 대한 수정이 적절한지 검토부탁드립니다.
sealed class를 지금까지는 object로만 사용했지만 일부를 class로 바꾸어 파라미터를 가지게 하는 경우는 처음입니다

또한, Open클래스를 만드는 과정에 대해서 들여쓰기를 최소화하기 위해 많이 나누어져 있는데 그것도 적절한지 알고싶습니다.

감사합니다.

Copy link

@sah3122 sah3122 left a comment

Choose a reason for hiding this comment

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

개인적인 사정으로 리뷰가 늦어져 죄송합니다. 🙏
몇가지 코멘트 남겨두었는데 확인 부탁드리며 궁금한 부분이 있다면 편하게 DM 주세요 !

@@ -4,37 +4,37 @@ import ramdom.RandomInterface

class Board(private val mapInfo: MapInfo, private val randomLogic: RandomInterface) {
Copy link

Choose a reason for hiding this comment

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

해당 로직이 프로퍼티로 생성될 경우 지뢰가 생성되는 로직에 대한 테스트를 하지 못할 것 같습니다.
프로퍼티로 구성되었을 때 테스트 로직을 어떤식으로 구현되어야 하는지 알려주시면 감사드리겠습니다.

개인적으론 프로퍼티로 정의하지 않고 부 생성자의 파라미터 또는 정적 팩토리 메서드를 활용해볼수 있을것 같습니다.
randomLogic이 프로퍼티로 정의되어야 하는 이유가 없다고 생각하기 때문에 랜덤으로 지뢰를 만드는 부 생성자를 추가하는것에 대한 코멘트 였습니다 😄

Comment on lines 74 to 106
private fun setOpen(x: Int, y: Int) {
val cell = mineBoard[x][y]
if (cell is None) {
val mineCnt = getMineCnt(cell)
mineBoard[x][y] = Open(mineCnt)
}
}

private fun getMineCnt(cell: None): Int {
val addIndexList = PERIPHERAL_INDEX_LIST
val cellX = cell.x
val cellY = cell.y
var mineCnt = 0

for (addIndex in addIndexList) {
val newX = cellX + addIndex.first
val newY = cellY + addIndex.second

mineCnt = increaseMineCnt(mineCnt, newX, newY)
}

return mineCnt
}

private fun increaseMineCnt(mineCnt: Int, newX: Int, newY: Int): Int {
if (!checkIndex(newX, newY)) return mineCnt

return if (mineBoard[newX][newY] is Mine) mineCnt + 1 else mineCnt
}

private fun checkIndex(newX: Int, newY: Int): Boolean {
return newX >= INDEX_ZERO && newX < mapInfo.width && newY >= INDEX_ZERO && newY < mapInfo.height
}
Copy link

Choose a reason for hiding this comment

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

PERIPHERAL_INDEX_LIST 대신 상, 하, 좌, 우, 대각에 대한 enum 을 정의하여 Cell 객체가 가지고 있는 position 을 기준으로 주변 탐색을 할수 있도록 정의해보는건 어떨까요 😄
position 을 가진 객체에게 메시지를 던지는 구조로 변경해보는것도 좋을것 같습니다 !

object Mine : Cell()
class Open(val mineCnt: Int) : Cell()
Copy link

Choose a reason for hiding this comment

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

Open 클래스를 별도로 정의하지 않고 Mine, None 의 속성으로 정의하는것도 고려해보면 좋을것 같아요 😄
추가로 sealed class 를 object가 아닌 class로 정의하여 파리미터, 메서드를 다르게 가져가는 방식의 구현은 많이 사용하는 편입니다.
적절한 상태값과 행위를 지정해줄수 있다면 유연하게 사용할수 있는 방법이라 익숙해지는것을 권장드려요 🙏

- Board생성자 프로퍼티 직접 참조
- class Open 삭제 -> class None 생성자 프로퍼티 추가(mineCnt: Int)
- outputView drawCell() 수정 (Open 삭제, None.mineCnt 추가)
@oyj7677
Copy link
Author

oyj7677 commented Dec 16, 2023

지뢰 탐색 시 사용하는 position에 대한 리뷰 적용하였습니다.

랜덤 로직 생성자 프로퍼티에 관련된 첫 리뷰 잘못 이해했었습니다.
'프로퍼티로 정의하지 않아도 되지 않을까요 ?' 의 내용 다음과 같이 이해했습니디.
'프로퍼티로 정의해야되지 않을까요?' 라고 봤던 것 같고 그것이 인터페이스 사용이 적절하지 안하고 말씀하신 것으로 이해했었습니다.
'프로퍼티 == 직접 객체 전달' 이라는 잘못된 이해였습니다.

이에 관한 수정으로 남겨주신 리뷰중 부 생성자의 경우 init블록 이후에 호출이 되기에 init블록에서 랜덤 로직을 사용하기 때문에 적절하지 않은 것으로 판단되었습니다.
정적 팩토리 메서드를 사용하여 프로덕트 코드에서는 따로 정의해주지 않아도 되고 테스트 코드 작성 시 Borad생성 전 미리 fake랜덤 로직으로 변경해주는 작업을 시행하도록 하였습니다.

부 생성자와 정적 팩토리 메서드를 이해할 수 있어서 좋았습니다. 감사합니다.

DM으로 질문 남겼었는데 생성자 값의 참조 범위를 어떤 식으로 구성해야될지 알고싶습니다.

  1. init에서만 사용하기 떄문에 init에서 호출되는 파라미터에 전달하기만 하면 된다
  2. 전역변수로 생성하여 파라미터로 전달하지 않아도 된다.

감사합니다.

Copy link

@sah3122 sah3122 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 +89 to +91
for (addIndex in RelativeDirection.values()) {
val newX = cellX + addIndex.x
val newY = cellY + addIndex.y
Copy link

Choose a reason for hiding this comment

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

RelativeDirection 정의 👍
한단계 더 나아가 좌표 값을 가지고 있는 Cell 객체에게 cell.searchAround 와 같이 메시지를 던져볼수 있을까요 ?

@sah3122 sah3122 merged commit a418967 into next-step:oyj7677 Dec 18, 2023
@oyj7677 oyj7677 deleted the step2 branch December 19, 2023 10:12
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants