Step2 지뢰 찾기(지뢰 개수) #389
Step2 지뢰 찾기(지뢰 개수) #389
Conversation
| return listOfNotNull( | ||
| topOrNull(), | ||
| bottomOrNull(), | ||
| leftOrNull(), | ||
| rightOrNull(), | ||
| topOrNull()?.leftOrNull(), | ||
| topOrNull()?.rightOrNull(), | ||
| bottomOrNull()?.leftOrNull(), | ||
| bottomOrNull()?.rightOrNull() | ||
| ) | ||
| } | ||
|
|
||
| private fun leftOrNull(): Position? = runCatching { Position(y, x - 1) }.getOrNull() | ||
| private fun rightOrNull(): Position? = runCatching { Position(y, x + 1) }.getOrNull() | ||
| private fun topOrNull(): Position? = runCatching { Position(y - 1, x) }.getOrNull() | ||
| private fun bottomOrNull(): Position? = runCatching { Position(y + 1, x) }.getOrNull() |
There was a problem hiding this comment.
특정 위치의 인접 8방향에 대한 내용을 도메인 객체로 표현해보는건 어떨까요?
There was a problem hiding this comment.
또, 인접한 칸을 찾는 과정에서, out of index 이슈를 회피하기 위해, 지뢰판 가장 바깥에 아무 역할도 수행하지 않는 칸을 두어서 지뢰판을 구성해볼 수도 있어요 :)
| .toPositions() | ||
| require(allPositions.size == mineMapMeta.getCellCount()) { "모든 위치를 생성하지 못했습니다" } |
There was a problem hiding this comment.
불필요한 검증문이라 생각해요.
오히려 mineMapMeta.getCellCount() 변경점에 대해 불필요한 의존관계를 가지고 있다는 생각도 들어요.
getCellCount()를 리팩터링 하면서 실수로 잘못만들었을 때 엉뚱한 녀석이 에러를 뱉어 빠른 디버깅을 방해할지도 모른다는 생각이 드네요!
오히려 이런 부분은 테스트코드로 검증해보는 게 의도를 더 잘 나타낼 수 있는 방법이라 생각해요 :)
There was a problem hiding this comment.
항상 require를 충족할 불필요한 검증문이 맞아 제거하였습니다!
(질문) 만약 generateAllPositions 과 같은 private 함수를 테스트하려면 public으로 공개해야 할까요? 테스트를 위해 접근 제어자를 변경해야 할지 고민입니다..! 🤔
There was a problem hiding this comment.
테스트만을 위해서 메서드를 public으로 열 필요는 없습니다.
private 메서드는 결국 public 메서드로 인해 호출되기에 간접적으로 테스트할 수 있다고 생각해 볼 수 있어요 :)
| class Positions( | ||
| private val positions: Set<Position> = emptySet() |
There was a problem hiding this comment.
Positions 객체가 특별히 수행하는 역할을 가지고 있지 않아보여요.
Positions를 반환하는 곳에서 Set을 반환하더라도 무방해보여요.
Positions가 갖고있는 컬렉션으로 도메인 역할을 수행하게 만들거나, 혹은 언래핑 해보는 건 어떨까요?
There was a problem hiding this comment.
특별한 비즈니스 로직을 수행하거나 유지보수 용이성이 없는 코드로 보여 언래핑하였습니다!
| FixedPositionSelector | ||
| ) |
There was a problem hiding this comment.
FixedPositionSelector가 어떤 값을 반환하는지는 직접 구현체를 보지 않고는 알 수 없을 거라 생각해요.
테스트 코드에서 바로 람다로 원하는 위치를 반환하게 명시해보는 건 어떨까요?
테스트 코드만을 보더라도, 어떤 값을 반환하는지 한 눈에 볼 수 있을것으로 기대돼요 :)
There was a problem hiding this comment.
현재 아래와 같은 주입 구조를 가지고 있어 MineCountMapFactory가 PositionSelector로부터 받은 위치 정보를 바로 활용할 수는 없습니다ㅠ
(MineCountMapFactory <- PositionGenerator <- PositionSelector)
객체 의존 관계가 복잡해질 수록 테스트하기 어렵거나 명확한 의미 전달이 어려워지는데요..! 의존도를 낮추기 위해 객체 협력 관계를 다시 정의해야 할지 고민이 됩니다 😭
There was a problem hiding this comment.
해당 주입 구조를 가지고 있기에 충분히 테스트 가능해 보여요!
FixedPositionSelector를 넣을 수 있는 것이 테스트 가능함을 암시한다고 볼 수 있겠어요 :)
제가 드린 제안은, FixedPositionSelector처럼 직접 상속해서 구현하기보다, 람다로 만들어서 넘겨보시라는 의미었어요.
해당 interface가 람다로 변환이 되지 않는것이 문제였다면,
fun interface에 대해 학습해보셔도 좋겠어요 :)
| // then | ||
| assertSoftly { | ||
| assertThat(mineMap.size).isEqualTo(9) | ||
| assertThat(mineMap.getCell(Position(1, 1))).usingRecursiveComparison().isEqualTo(Mine) | ||
| assertThat(mineMap.getCell(Position(1, 2))).usingRecursiveComparison().isEqualTo(Mine) | ||
| assertThat(mineMap.getCell(Position(1, 3))).usingRecursiveComparison().isEqualTo(Mine) | ||
| assertThat(mineMap.getCell(Position(2, 1))).usingRecursiveComparison().isEqualTo(Empty(2)) | ||
| assertThat(mineMap.getCell(Position(2, 2))).usingRecursiveComparison().isEqualTo(Empty(3)) | ||
| assertThat(mineMap.getCell(Position(2, 3))).usingRecursiveComparison().isEqualTo(Empty(2)) | ||
| assertThat(mineMap.getCell(Position(3, 1))).usingRecursiveComparison().isEqualTo(Empty(0)) | ||
| assertThat(mineMap.getCell(Position(3, 2))).usingRecursiveComparison().isEqualTo(Empty(0)) | ||
| assertThat(mineMap.getCell(Position(3, 3))).usingRecursiveComparison().isEqualTo(Empty(0)) | ||
| } | ||
| } |
There was a problem hiding this comment.
어떤 지뢰판인지 주석을 활용해서 시각적으로 표현해보는 건 어떨까요?
테스트를 더욱 가독성있게 작성할 수 있도록 도와줍니다 :)
너무 급하게 미션을 진행하실 필요 없어요! |
윤혁님 안녕하세요, step2 리뷰 요청드립니다!
작업 내용
DIP원칙이 지켜질 수 있도록 인터페이스를 활용하였습니다.SRP원칙에 근거해 상대 좌표를 함께 제공할 수 있도록 확장하였습니다.리뷰 중점 사항
SOLID 원칙을 잘 지킨 부분과 지키지 못한 부분을 중심으로 피드백 주시면 감사하겠습니다....! (_ _)
요청
step2 리뷰는 step3에서 반영하겠습니다! (step2 리뷰 후 바로 마지 부탁드릴게요..!)
신속하게 이번 미션 끝내보겠습니다!!!!!!