[크롱] 1,2단계 미션 제출합니다. #5
Conversation
| board.move(stone, state) | ||
| } | ||
|
|
||
| fun isEmpty(stone: Stone): Boolean = board.isEmpty(stone) |
There was a problem hiding this comment.
코드를 작성할 때 디미터의 법칙을 준수해서 omokBoard.board.isEmpty() 와 같은 방식으로접근하는 것이 좋지 않다고 생각이 듭니다. 그래서 omokBoard에서 isEmpty함수를 호출하면 board의 isEmpty함수를 호출하도록 만들게 되는데 결국 코드의 중복이 아닌가 하는 생각이 들고 좋은 방법인지 의문이 드네요😢
각 클래스의 책임이 무엇인가 고민해보셔도 좋을거 같아요.
현재의 구조로는 사실 OmokBoard는 Board의 래핑클래스로 별다른 책임을 가지고 있지 않은걸로 보여지네요,
OmokBoard를 설계했던 책임이 무엇인가에 대해 고민해보아요!
또 위와 같이 작성했을 때 테스트 코드에서 해당 함수의 동작을 확인해야 하는가도 의문이 듭니다!
위의 코멘트와 같은 이유인데요,
해당 클래스의 책임에 대한 테스트를 작성하지만, 지금 같은 경우는 책임이 없기때문에 무의미한것 처럼 보여지네요!
만약 구조상 위와 같은 구조라면, 저는 테스트를 작성하지 않아도 될거라 생각이 들어요!
There was a problem hiding this comment.
답변 감사합니다! Board클래스를 만든 이유는 일급컬렉션으로 사용하기 위함이었고, OmokBoard클래스를 만든 이유는 Board가 일급컬렉션이니까 이를 들고 있을 클래스가 필요하다는 이유였던 것 같습니다!
하지만 되돌아 생각해보니 정보를 갖고 있는 Board에 메시지를 넘겨 결과를 받고 있고 결과적으로 OmokBoard가 있는 이유가 불분명해진 것 같습니다!
덕분에 명확하게 깨달은 것 같습니다! 감사해요 :)
src/main/kotlin/domain/Referee.kt
Outdated
| if (board.value[i][j] == State.EMPTY) continue | ||
|
|
||
| // 가로 방향 체크 | ||
| if (j + 4 < size && | ||
| board.value[i][j] == state && | ||
| board.value[i][j + 1] == state && | ||
| board.value[i][j + 2] == state && | ||
| board.value[i][j + 3] == state && | ||
| board.value[i][j + 4] == state | ||
| ) { | ||
| return true | ||
| } | ||
|
|
||
| // 세로 방향 체크 | ||
| if (i + 4 < size && | ||
| board.value[i][j] == state && | ||
| board.value[i + 1][j] == state && | ||
| board.value[i + 2][j] == state && | ||
| board.value[i + 3][j] == state && | ||
| board.value[i + 4][j] == state | ||
| ) { | ||
| return true | ||
| } | ||
|
|
||
| // 대각선 방향 체크 (좌상단에서 우하단) | ||
| if (i + 4 < size && j + 4 < size && | ||
| board.value[i][j] == state && | ||
| board.value[i + 1][j + 1] == state && | ||
| board.value[i + 2][j + 2] == state && | ||
| board.value[i + 3][j + 3] == state && | ||
| board.value[i + 4][j + 4] == state | ||
| ) { | ||
| return true | ||
| } | ||
|
|
||
| // 대각선 방향 체크 (우상단에서 좌하단) | ||
| if (i + 4 < size && j - 4 >= 0 && | ||
| board.value[i][j] == state && | ||
| board.value[i + 1][j - 1] == state && | ||
| board.value[i + 2][j - 2] == state && | ||
| board.value[i + 3][j - 3] == state && | ||
| board.value[i + 4][j - 4] == state | ||
| ) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
이미 주석과, 문단을 분리함으로, 로직을 분리하셧지만,
함수로 분리해보면 어떨까요?
가독성이나 관리측면에서 좋을거같아요 😄
src/main/kotlin/domain/Referee.kt
Outdated
| } | ||
|
|
||
| fun checkForbidden(myBoard: OmokBoard, stone: Stone): Boolean { | ||
| return OmokRuleAdapter().checkForbidden(myBoard, stone) |
There was a problem hiding this comment.
OmokRuleAdapter의 객체 생성을 Referee내에서 하게된다면,
Referee는 OmokRuleAdapter에 대한 강한 의존성이 생기게됩니다.
OmokRuleAdapter는 생성 시점에 주입받아보면 어떨까요?
There was a problem hiding this comment.
생성 시점에 주입 받는 방법도 있을 것 같다고 생각이 듭니다! 또 다른 의견으로 checkForbidden()함수를 실행할 때 함수 파라미터로 주입 받는 것도 좋은 방법으로 생각되어 해당 방법으로 의존성을 제거해보았습니다!
|
|
||
| import omok.domain.state.EmptyStoneState | ||
|
|
||
| class Rule(private val board: ArkBoard) { |
There was a problem hiding this comment.
domain.rule 패키진 내의 Rule, ArkBoard 등의 알고리즘은 따로 리뷰하지 않도록 할게요!
src/main/kotlin/domain/OmokGame.kt
Outdated
| private fun successBlackTurn( | ||
| getStone: () -> Stone, | ||
| onMoveFail: () -> Unit, | ||
| onForbidden: () -> Unit, | ||
| onMove: (Board, State, Stone) -> Unit | ||
| ): Boolean { | ||
| val blackStone = getStone() | ||
| if (!omokBoard.isEmpty(blackStone)) { | ||
| onMoveFail() | ||
| return successBlackTurn(getStone, onMoveFail, onForbidden, onMove) | ||
| } | ||
|
|
||
| if (!referee.checkForbidden(omokBoard, blackStone)) { | ||
| onForbidden() | ||
| return successBlackTurn(getStone, onMoveFail, onForbidden, onMove) | ||
| } | ||
| omokBoard.move(blackStone, State.BLACK) | ||
| onMove(omokBoard.board, State.WHITE, blackStone) | ||
| return true | ||
| } | ||
|
|
||
| private fun successWhiteTurn( | ||
| getStone: () -> Stone, | ||
| onMoveFail: () -> Unit, | ||
| onMove: (Board, State, Stone) -> Unit | ||
| ): Boolean { | ||
| val whiteStone = getStone() | ||
| if (!omokBoard.isEmpty(whiteStone)) { | ||
| onMoveFail() | ||
| return successWhiteTurn(getStone, onMoveFail, onMove) | ||
| } | ||
| omokBoard.move(whiteStone, State.WHITE) | ||
| onMove(omokBoard.board, State.BLACK, whiteStone) | ||
| return true | ||
| } |
There was a problem hiding this comment.
checkForbidden을 제외하면, 중복 코드로 보여지네요!,
중복코드를 제거해보아요!
| } | ||
|
|
||
| // 3*3 or 4*4 check | ||
| override fun checkForbidden(myBoard: OmokBoard, stone: Stone): Boolean { |
There was a problem hiding this comment.
check라는 접두사는 boolean을 리턴할거라고 보기 힘들거같아요!
is, has 같은 접두사를 사용하면 좋을거 같아요!
| class OmokRuleAdapter : OmokRule { | ||
|
|
||
| // OurBoard -> ArkBoard | ||
| private fun boardConverter(myBoard: OmokBoard): ArkBoard { |
There was a problem hiding this comment.
함수 순서에 대한 이야기입니다!
개발자들은 보통 코드를 작성하는 시간보다 작성을 위해 코드를 읽는 시간이 훨씬 많다고해요!
코드를 읽는 사람의 가독성을 위해, 위에서 아래로 흐름이 흐르도록 순서를 바꿔보면 어떨까요?
코틀린 공식 컨벤션을 가볍게 읽어봐도 좋을거같아요!
https://kotlinlang.org/docs/coding-conventions.html#class-layout
There was a problem hiding this comment.
코틀린 공식 컨벤션에서는 관련 항목을 함께 배치하여 위에서 아래로 읽는 사람이 무슨 일이 일어나고 있는지 논리를 따를 수 있도록 합니다. 순서를 선택하고(높은 수준의 항목을 먼저 또는 그 반대로) 이를 고수하십시오.라고 되어있네요!
높은 수준의 함수를 먼저 위로 올린 뒤에 나눠진 함수를 아래로 배치했습니다 :)
| import omok.domain.state.EmptyStoneState | ||
| import omok.domain.state.WhiteStoneState | ||
|
|
||
| class OmokRuleAdapter : OmokRule { |
| turn.add(white.removeFirst()) | ||
| } | ||
| var index = 0 | ||
| omokGame.runGame({ turn[index++] }, ::onMove, {}, {}, {}) |
There was a problem hiding this comment.
검증단계가 없어요
검증 단계를 작성해보아요!
흑돌이 오목을 완성하면 게임이 종료되고, 흑돌이 승리한다.
There was a problem hiding this comment.
테스트 검증을 위해 제가 생각한 방법은 OmokGame의 OmokBoard에게 돌의 색을 넘겨주고 승리하였는지 확인하는 것입니다. 하지만 이를 위해서 OmokGame의 omokBoard를 private을 풀게 되었는데요. 결국 테스트를 위해 private을 풀게 된 것이 아닌가 하는 생각이 듭니다.
runGame 함수는 반환 값도 없는 상태이기 때문에 다른 좋은 방법이 떠오르지 않아서 위와 같이 진행했는데, 혹시 다른 좋은 방법이 있을지 여쭤보고 싶습니다!! 🙃
| // when, then | ||
| assertThat(referee.isWin(board, State.BLACK)).isTrue |
There was a problem hiding this comment.
| // when, then | |
| assertThat(referee.isWin(board, State.BLACK)).isTrue | |
| // when | |
| val result = referee.isWin(board, State.BLACK) | |
| // then | |
| assertThat(result).isTrue |
실행 단계와, 검증 단계를 분리하면 어떤점이 좋을까요?
실행/검증 단계가 복잡해지면, 무엇을 테스트하는지 직관적이지 않을 가능성도 있어요!
분릴를 통해 무엇을 테스트하는지 명확하게 작성하면, 가독성이나 관리 측면이서 좋을거 같아요!
| val inputView: InputViewInterface, | ||
| val outputView: OutputViewInterface | ||
| ) { | ||
| private val omokGameListener = object : Listener { |
There was a problem hiding this comment.
Listener를 통해 만들어주셨네요 👍
어떤 리스터인지 구체적인 네이밍을 추가하면 좋을거같아요!
There was a problem hiding this comment.
네이밍을 해주었고, 컨트롤러에서 직접 구현해서 사용하고 있기 때문에 사용하지 않는 클래스도 삭제했습니다.
| if (!omokBoard.isEmpty(stone)) { | ||
| listener.onMoveFail() | ||
| return doTurn(state) | ||
| } | ||
|
|
||
| if (state == State.BLACK && omokBoard.isForbidden(stone)) { | ||
| listener.onForbidden() | ||
| return doTurn(state) | ||
| } | ||
|
|
||
| omokBoard.move(stone, state) |
There was a problem hiding this comment.
다만 말씀해주신 lamda를 활용한 방법보다, 리턴하는 구조라면 어떨까요?라는 부분은 잘 이해가 되지 않는데 혹시 조금 더 자세하게 설명부탁드려도 될까요?🙃
개인적인 의견이니 참고만해주셔도 되요!
omokBoard에게 메세지를 던져서 확인하고 있지만,
isForbidden, isEmpty 등의 동작이 OmokGame에서 확인과 동작을 하고있어요,
OmokGame은 순서에 대한 턴에 맞는 동작을 하는 역할이라면,
턴을 통해, 오목돌을 두고, 동작을 하는부분은 omokBoard의 책임이라고 볼수도 있을거같아요1
omokBoard move내에서 실패처리를 하고, Result객체나, sealed class등을 통해
성공, 실패(Forbidden, Empty 등)의 리턴값을 반환하는 방법도 있을수 있을것 같아요!
| omokGame.runGame() | ||
|
|
||
| // when | ||
| val result = omokGame.omokBoard.isVictory(State.BLACK) | ||
|
|
||
| // then | ||
| assertThat(result).isTrue |
There was a problem hiding this comment.
테스트 검증을 위해 제가 생각한 방법은 OmokGame의 OmokBoard에게 돌의 색을 넘겨주고 승리하였는지 확인하는 것입니다. 하지만 이를 위해서 OmokGame의 omokBoard를 private을 풀게 되었는데요. 결국 테스트를 위해 private을 풀게 된 것이 아닌가 하는 생각이 듭니다.
runGame 함수는 반환 값도 없는 상태이기 때문에 다른 좋은 방법이 떠오르지 않아서 위와 같이 진행했는데, 혹시 다른 좋은 방법이 있을지 여쭤보고 싶습니다!! 🙃
테스트를 위해 프로그램 코드에 영향을 주는건 지양하는게 좋아요!
private을 푸는 방법이 아닌, 다른 방법을 찾아보아요!
게임의 결과 값을 리스너를 통해 전달하고있지만,
val result = omokGame.runGame()로 결과값을 리턴하는 구조로 변결할수 있고,
fakeListener를 활용하여, onFinish의 state를 확인할수도 잇을거같아요!
There was a problem hiding this comment.
말씀해주신대로 runGame()의 반환값을 변경하여 테스트를 무사히 진행했습니다. 한 가지 궁금한 것은 생겼는데요, runGame의 반환값을 설정해주는 것은 테스트를 위한 코드가 아니라고 볼 수 있는 건가요?
| assertThat(result).isTrue | ||
| } | ||
|
|
||
| private class fakeListener(val turn: MutableList<Stone>) : Listener { |
There was a problem hiding this comment.
fakeListener를 작성해주셨네요, Fake객체도 클래스는 첫글자 대문자를 잊지마세요!
개인적으로는 FakeListener를 만들기 위해선 내부의 모든 함수를 override해주어야하는 단점이 있어요,
FakeListener를 잘쓰기 위해, 아무것도 하지않는 FakeListener를 만들고,
FakeListener를 상속받아, 해당 테스트에 필요한 함수만 정의하곤해요,
참고만 해주셔도 좋아요!
open class FakeListener : Listener {
override fun a() {}
override fun ab() {}
...
}
val fakeListener = object: FakeListener() {
override fun onFinish(state: State)() {
~~
}
}There was a problem hiding this comment.
FakeListener를 여러 곳에서 사용한다면 굉장히 좋은 방법이 될 것 같아요! 감사합니다🙃
그럼 이와 같은 이유로 InputView, OutputView도 Fake 객체를 만들어두면 좋을 것 같다고 생각되는데 괜찮은지 궁금합니다!
또, 테스트를 위한 코드이기 때문에 Test안에 클래스를 만드는 게 좋을지 궁금합니다!
| } | ||
|
|
||
| @Test | ||
| fun `3*3 test3`() { |
There was a problem hiding this comment.
같은 로직의 테스트라면, ParameterizedTest를 통해 테스트할수도 잇을거같아요!
There was a problem hiding this comment.
4x4에서는 백목의 위치도 지정해줘야 할 때도 있기 때문에 3x3 금수 테스트만 ParameterizedTest를 사용하여 묶었습니다!
| } | ||
|
|
||
| @Test | ||
| fun `좌상단에서 우하단 대각선으로 5개 놓여있으면 승리다`() { |
There was a problem hiding this comment.
구체적인 테스트 케이스 👍
테스트케이스를 하나의 요구사항 명세서로 볼수도 있을것 같아요 👍
| fun `3*3 test4`() { | ||
| // given | ||
| val referee = Referee() | ||
| val myBoard = OmokBoard() | ||
|
|
||
| myBoard.move(Stone.create('J', 9), State.BLACK) | ||
| myBoard.move(Stone.create('M', 10), State.BLACK) | ||
| myBoard.move(Stone.create('N', 9), State.BLACK) | ||
| myBoard.move(Stone.create('M', 12), State.BLACK) |
There was a problem hiding this comment.
isMovable의 테스트지만,
테스트 준비단계에서 myBoard.move 함수가 여러번 실행되고있어요.
해당 테스트는 myBoard.move 함수에 의존적인 테스트라고 할수 있을거같아요!
해당 함수를 실행하는 방법이 아닌, myBoard 생성시, 초기값을 넣어주면 어떨가요?
안녕하세요 리뷰어님! 안드로이드 크루 크롱이라고 합니다! 잘부탁드려요🙃
미션을 진행하면서 몇 가지 질문이 생겨 남겨봅니다!
코드를 작성할 때 디미터의 법칙을 준수해서 omokBoard.board.isEmpty() 와 같은 방식으로접근하는 것이 좋지 않다고 생각이 듭니다. 그래서 omokBoard에서 isEmpty함수를 호출하면 board의 isEmpty함수를 호출하도록 만들게 되는데 결국 코드의 중복이 아닌가 하는 생각이 들고 좋은 방법인지 의문이 드네요😢
또 위와 같이 작성했을 때 테스트 코드에서 해당 함수의 동작을 확인해야 하는가도 의문이 듭니다!
또 금수 알고리즘을 만드는 것이 중요한 것이 아니라는 말씀을 해주셔서 다른 크루의 알고리즘을 가져와 사용하도록 구현하였습니다. 올바르게 작성했는지는 잘 모르겠지만 어댑터 패턴을 적용하려고 노력하였습니다!