[수달] 1, 2단계 오목 제출합니다. #9
Conversation
- 스톤을 받아 해당 스톤의 위치에 돌이 놓여있는지 확인하는 기능 - 스톤들을 받아서 두 스톤들을 더한 값을 반환하는 기능
- 오목 조건 충족 처리하는 기능 오류 수정 - 바둑판에 표기시 BLACK, WHITE, EMPTY 세가지로 표시
- 오목을 놓을 수 있는지 확인 - 오목 조건을 충족하는지 확인
- stone을 추가할 수 없는 상태라면 추가하지 않고 BlackTurn 반환 - stone을 추가한 후 WhiteTurn 반환 - 오목 조건 충족하면 End 상태로 반환
- stone을 추가할 수 없는 상태라면 추가하지 않고 WhiteTurn 반환 - stone을 추가한 후 BlackTurn 반환 - 오목 조건 충족하면 End 상태로 반환
Board가 stones를 관리하도록 하여 State에 인자를 줄임 OmokRule Object로 수정(한번만 생성되도록 하기 위해)
| outputView.printTurn(state, board.stones) | ||
|
|
||
| when (state) { | ||
| is BlackTurn -> state = state.put(Stone(inputView.inputStonePosition(), StoneType.BLACK)) |
There was a problem hiding this comment.
[질문 주신 부분]
stone 을 정의할 때 꼭 필요한 부분만 외부에서 전달받고 나머지는 State '스스로' 결정 하도록 바꿔보면, 분기처리가 필요없지 않을까요?
*black 다음에는 white, white 다음에는 black 이 고정되어 있는것을 잘 고민해보면 좋겠습니다 :)
There was a problem hiding this comment.
돌을 놓을 때 위치만 전달하도록 하여 상태에서 돌을 생성해 놓도록 하였습니다!!!
|
|
||
| interface State { | ||
|
|
||
| fun put(stone: Stone): State |
There was a problem hiding this comment.
함수가 수행하는 일을 생각해봤을 때,
'다음 상태' 라는 정보가 네이밍에 들어가는 것은 어떨까요?
put 은 너무 general 한 이름이라 함수가 무엇을 수행하는지 알기 어려울 것 같아요.
| if (!isValidPut(stone)) return BlackTurn(board) | ||
| if (checkForbidden(board, stone)) return BlackTurn(board) | ||
| board.putStone(stone) | ||
| if (OmokRule.isWinCondition(board.board, stone)) return End(StoneType.BLACK) |
There was a problem hiding this comment.
오목의 규칙은 대회에 따라 세부사항이 달라질 수 있는 걸로 알고 있는데요.
OmokRule 에 직접 접근하기보다 인터페이스를 통해 언제든 교체할 수 있도록 구성해보는 것은 어떨까요?
There was a problem hiding this comment.
오...!! 이 부분은 'State 인터페이스에서 선언해야하는건가...?'라는 등 잘 이해가 되지 않아 같은 모바일 크루인 해시와 이야기 해보았는데, 해시가 규칙에 interface를 둔 것을 보고 배웠습니다!
규칙에도 인터페이스를 둔다면 추후 세부 규칙에 변경이 있더라도 인스턴스화 하는 클래스만 변경하면 되니까 변화에 유리할 것 같네요!!
인터페이스는 정말 신기한 것 같군요 🤔🤔🤔
좋은 피드백 감사합니다!!!
| import domain.stone.Stone | ||
| import domain.stone.StoneType | ||
|
|
||
| class BlackTurn(board: Board) : Running(board) { |
There was a problem hiding this comment.
State 가 Board 를 주입받는 방식과
Board 가 State 를 소유하고 있는 방식의
장/단점은 무엇이 있을까요?
There was a problem hiding this comment.
State에 의존성을 생성자를 통해 주입하는 방식, 메서드를 통해 주입하는 방식에 대해 고민해 보았습니다!
아직까지는 두가지에 크게 차이가 없다고 생각합니다!
클래스 내부에서 다수의 메서드들이 사용한다면 생성자로 의존성을 받는 것이 유리할 것이고,
일부의 메서드에서만 이용한다면 메서드로 의존성을 받는 것이 유리할 것 같습니다.
또한, 생성자로 의존성을 유입받을 시에는 해당 객체의 의존성을 파악하기 편리한 것 같습니다!
피드백 주신 내용을 통해 제 코드를 살펴보니 board와 position은 사용하는 메서드에 차이가 없음에도
board는 생성자를 통해 주입하고, position은 메서드를 통해 주입하고 있었네요…!
두 객체는 돌을 놓고, 다음 상태로 넘어가는 메서드에서만 사용하기 때문에
메서드를 통해 의존성을 주입하도록 통일시키겠습니다!
There was a problem hiding this comment.
정리 잘해주신 것 같습니다 👍
제가 생각했을 때, 생성자 주입과 함수를 통한 주입의 또다른 큰 차이점은 상태의 지속성입니다.
생성자로 상태를 주입받을 경우, 보통은 그 클래스의 생명주기와 주입받은 상태의 생명주기가 같이가게 됩니다.
이는 2가지를 뜻합니다.
- 해당 클래스에 해당 상태는 필수적인 요소이다.
- 주입받은 상태가 외부로부터 변경이 발생했을때, 해당 클래스에서 예기치 못한 이슈가 발생해서는 안된다.
한편, 함수로 주입을 받을 경우 상태의 생명주기는 함수 콜스택에서 멈추게 됩니다.
위에서 얘기한 2가지와 반대의 효과를 가져올 것이구요.
주입 방식을 고민할 때, 위 내용도 같이 생각해보면 좋을 것 같습니다.
|
|
||
| fun getWinner(): StoneType | ||
|
|
||
| fun isValidPut(stone: Stone): Boolean |
There was a problem hiding this comment.
특정 상태에 대한 유효성 검증 함수가 인터페이스 요구사항에 들어가는 것이 적절할까요?
유효성 검증은 optional 한 제안이 아닐까 싶습니다 ㅎㅎ
There was a problem hiding this comment.
다시 생각해보니 그러네요!!
얼마전 "객체지향의 사실과 오해"라는 책을 읽었는데, 인터페이스와 관련해 개념을 조금은 알게 된 느낌입니다!
상태를 나타내는 객체들 외의 외부 객체들에서 사용하는 메서드들만 interface에 있는 것이 좋을 것 같습니다!
내부에서만 사용하는 함수들은 인터페이스를 상속 받는 클래스에서 구현하는 것이 좋을 것 같습니다!
좋은 피드백 감사합니다!
| private const val MESSAGE_FORMAT_LAST_STONE_POSITION = "(마지막 돌의 위치: %s)" | ||
| private const val MESSAGE_FORMAT_WINNER = "%s돌이 우승했습니다." | ||
|
|
||
| val board: MutableList<MutableList<Char>> = mutableListOf( |
There was a problem hiding this comment.
이 부분은 하드코딩하기보다 board 에 대한 view 로직을 구현해보는 것은 어떨까요?
board 를 draw 해주는 요구사항이 조금만 바뀌더라도 수정해줘야하는 부분이 너무 많을 것 같아요.
There was a problem hiding this comment.
이 부분은 로직으로 나타내고 싶었으나, 긴 시간 고민해 보았음에도 아직 어려운 것 같습니다...
다른 크루들의 코드를 살펴보며 수정해보도록 하겠습니다... 🥲
src/main/kotlin/view/OutputView.kt
Outdated
|
|
||
| private fun positionToText(value: Stone): String { | ||
| val xToString: String = (value.position.x.toChar() + 'A'.toInt() - 1).toString() | ||
| return "$xToString${value.position.y}" |
There was a problem hiding this comment.
string templete 에는 로직이 들어가는것을 지양해보는 것은 어떨까요?
어떤 결과로 나타날지 예측이 어려워지는 것 같아요.
| package domain.stone | ||
|
|
||
| enum class StoneType { | ||
| BLACK, WHITE, EMPTY |
There was a problem hiding this comment.
board는 2차원 리스트이고, 각 값은 null값을 기본값으로 하였습니다.
그러나 null을 사용할 시 외부에서 null 체크를 해주어야 했기 때문에 null을 사용하는 것 대신에
board 칸의 각 기본값을 EMPTY 타입의 스톤들로 채워두었습니다!
혹시 이렇게 사용할 경우 예상치 못한 문제가 발생하거나 가독성이 떨어질 수 있을까요? 🤔
There was a problem hiding this comment.
제가 생각했을 때, EMPTY 는 StoneType 의 카테고리라고 보기는 어려웠어서 코멘트를 남겼습니다.
구현의 편의를 위해 개념이 추가된 케이스라고 생각되는데, 그 반대가 되어야 맞을것 같아서요!
| private const val MAXIMUM_POSITION_NUMBER = 15 | ||
|
|
||
| fun from(x: Int, y: Int): StonePosition? { | ||
| if (x !in MINIMUM_POSITION_NUMBER..MAXIMUM_POSITION_NUMBER) return null |
There was a problem hiding this comment.
이부분은 도메인에 대한 요구사항을 지키지 못한 것이므로, null 을 리턴하기보다 예외를 던져보는 것은 어떨까요?
null 만을 리턴할 경우 외부에서 무엇이 잘못되었는지 코드를 직접확인하기 전까지는 알기 어려울 것 같아요.
There was a problem hiding this comment.
이 부분은 리뷰어님 중 한 분께서 남겨주신 글을 읽고 반영해보았습니다.
해당 리뷰어님의 글에서는 "정상적인 흐름에서는 Exception 대신 null을 반환하고, 예외적인 상황에만 예외를 던지라"고 하셨습니다.
(해당 글의 내용을 전부 이해한 것이 아니므로 글에서 유도하고자 하는 방식과는 조금 다를 수 있습니다...)
현재 저의 코드에서는 사용자가 입력한 값을 이용해 도메인을 만들고 있습니다. 때문에 도메인 검증 부분은 사용자 입력 오류를 검증하는 역할도 포함한다고 생각하였습니다.
또한, 사용자가 잘못 입력한 것은 정상적인 흐름이라고 생각합니다. 때문에 null을 반환하는 벙법을 택하였고, 사용자가 잘못 입력한 값에 대해 추가적으로 view에서 메시지를 출력하고, 다시 받을 수 있도록 처리를 하도록 하였습니다.
현재 저의 예외 처리 방식에 대한 문제인 것 같기도 합니다.
null을 보내도, 예외를 던져도 해당 문제에 대해 runcatcing을 하여 처리하기 때문에 두 방법에 차이가 없는 것 처럼 느껴집니다.
There was a problem hiding this comment.
전달주신 글의 내용도 공감합니다.
다만, 사용처가 조금 다르다는 생각은 하였습니다.
우선 null safe 가 언어적으로 보장된다고 하더라도, 저는 null 타입을 지주 활용하는 것은 결코 좋지 않다고 생각합니다.
모든 예외상황에 대해 null 을 리턴하게 되면, 결국 어디에선가는 null 에 대한 처리를 해줘야하고 그러한 처리의 깊이가 복잡해질수록 무엇이 문제인지 알기가 어려워 집니다.
또한, 팩터리 메서드는 그 내용 자체로 '생성'을 하는 것이 주 역할인데, null 을 리턴하는 것이 조금 어색합니다.
만약 null 을 리턴하고자 한다면 createOrNull 의 네이밍등으로 명시가 되었으면 더 좋았을 것 같아요 ㅎㅎ
- 유효성 검증은 running 내부에서만 이용되어 외부 노출이 불필요한 메서드라 판단
- InputView와 OutputView의 함수명 통일 - string templete에서 로직 분리
- 메서드명 변경
- controller에서 분기를 생성하지 않도록 state에 필수 정보인 돌의 위치를 넘기도록 함
- 불필요하게 외부 접근이 가능한 변수들을 private, protected로 변경 - index만 사용하는 forEachIndexed를 indices.forEach로 변경 - 학습 테스트 주석
- 게임이 진행중인 상태에서 우승 돌의 타입을 반환하는 getWinner 호출시 EMPTY를 반환하도록 함 - 게임이 종료된 상태에서 다음 턴을 실행하는 next를 호출시 End를 반환하도록 함
| private const val MAXIMUM_POSITION_NUMBER = 15 | ||
|
|
||
| fun from(x: Int, y: Int): StonePosition? { | ||
| if (x !in MINIMUM_POSITION_NUMBER..MAXIMUM_POSITION_NUMBER) return null |
There was a problem hiding this comment.
전달주신 글의 내용도 공감합니다.
다만, 사용처가 조금 다르다는 생각은 하였습니다.
우선 null safe 가 언어적으로 보장된다고 하더라도, 저는 null 타입을 지주 활용하는 것은 결코 좋지 않다고 생각합니다.
모든 예외상황에 대해 null 을 리턴하게 되면, 결국 어디에선가는 null 에 대한 처리를 해줘야하고 그러한 처리의 깊이가 복잡해질수록 무엇이 문제인지 알기가 어려워 집니다.
또한, 팩터리 메서드는 그 내용 자체로 '생성'을 하는 것이 주 역할인데, null 을 리턴하는 것이 조금 어색합니다.
만약 null 을 리턴하고자 한다면 createOrNull 의 네이밍등으로 명시가 되었으면 더 좋았을 것 같아요 ㅎㅎ
| outputView.printOmokStart() | ||
| while (state !is End) { | ||
| outputView.printTurn(state, board.stones) | ||
| state = state.next(board, inputView.inputStonePosition()) |
| MutableList(16) { MutableList(16) { StoneType.EMPTY } } | ||
|
|
||
| fun putStone(stone: Stone) { | ||
| _board[(stone.position.y)][stone.position.x] = stone.type |
src/main/kotlin/view/InputView.kt
Outdated
| } | ||
|
|
||
| private fun textToPosition(value: String): Pair<Int, Int> { | ||
| return Pair((value[0] + 1) - 'A', value.substring(1).toInt()) |
There was a problem hiding this comment.
각 파라미터에 들어가는 값을 변수로 선언해주고 넣어주는 것은 어떨까요?
어떤 값이 들어가는지 명확히 알기 어려운 것 같아요.
src/main/kotlin/view/InputView.kt
Outdated
| class InputView { | ||
|
|
||
| fun inputStonePosition(): StonePosition { | ||
| print("위치를 입력하세요: ") |
There was a problem hiding this comment.
OutputView 처럼 해당 텍스트도 상수화해보는 것은 어떨까요?
|
|
||
| abstract override fun next(board: Board, stonePosition: StonePosition): State | ||
|
|
||
| override fun getWinner(): StoneType = StoneType.EMPTY |
There was a problem hiding this comment.
해당 함수는 사실상 End 에서만 유효한 함수라고 생각되는데요, 모든 상태에서 구현될 필요가 있을까요?
There was a problem hiding this comment.
StoneType.EMPTY 라는 타입이 존재하기 때문에, 이런 애매한 상황이 생길 수 있는 것 같습니다.
이건 개인적인 부분이지만, 어떤 도메인이 나타나고자 하는 표현방식에 '중립적인' 내용이 있으면, 사람마다(=개발자) 해석의 여지가 다르기에 버그를 유발할 가능성이 높아진다고 생각해요.
- 식을 각각의 변수로 분리 - 불필요 연산 제거
- StonePosition을 생성해야하므로 null을 반환하는 것은 적절하지 않다고 판단
- 팩터리 메서드에서 값에 문제가 있을시 null이 아닌 throw를 하도록 변경하여 init을 통해 생성하는 것과 차이가 없다고 판단
- 팩터리 메서드를 사용해 생성하는 것과 생성자를 사용해 생성하는 것에 차이가 없다고 판단
안녕하세요! 제리 리뷰어님!!
모바일 수달입니다!!
레벨 1 마지막 미션인 오목 미션 기간 동안 잘 부탁드리겠습니다!!!
[피드백을 받고 싶은 부분]
이번에 처음으로 상태패턴을 적용시켜보며 클래스, 인터페이스, 추상클래스 등을 적절하게 나눈 것인지, 각각의 특징을 살려 잘 사용한 것인지 고민이 있습니다.
관련해서 고민되는 부분 하나를 꼽자면 컨트롤러에서 흑돌차례, 백돌차례에 따라 분기를 만들었는데, State라는 인터페이스를 활용해 분기를 사용하지 않고 처리하는 방법이 있을까요...?