[🚀 사이클1- 미션 (보드 초기화 + 기물 이동)] 루덴스 미션 제출합니다.#234
[🚀 사이클1- 미션 (보드 초기화 + 기물 이동)] 루덴스 미션 제출합니다.#234poketopa wants to merge 41 commits intowoowacourse:poketopafrom
Conversation
There was a problem hiding this comment.
다소 엽기적으로 보일 수 있는 코드라고 생각합니다.
이동 가능한 좌표를 순회하는 부분에 있어서는 페어와 많은 대화를 했습니다.
기물이 갈 수 있는 위치에 대해 좌표 기반으로 수식을 만들어 짧게 표현할 수도 있었지만, 객체지향 방법론들을 배우면서 느꼈던 것은 유지보수, 확장, 그리고 가독성이 1순위가 되어야 한다는 것이었습니다.
그렇기에, 이동 가능한 위치를 모두 나타내는 하드코딩을 선택했습니다.
마, 상 졸 등은 이동 가능한 경우가 많지 않아 충분히 용인 가능한 수준이었지만, 차, 포는 훨씬 많은 이동의 경우가 있었기 때문에 기이한 수준으로 코드가 늘어났습니다.
이에 대해 차와 포의 이동은 반복문으로 구현하는게 나은가? 하는 물음에 다음과 같은 기준으로 다른 기물과 같이 하드코딩을 선택했습니다.
- 왜 하드코딩을 선택했는가? -> 이해하지 못하는 짧은 수식을 쓰는 것보다 길지만 직관적인 (심지어 다 읽어보지 않고도 이해할 수 있는) 가독성 좋은 코드를 작성하는게 좋기 때문이다.
- 그렇다면 기물의 이동 방식이 변경되거나 추가된다면? -> 그런 경우가 생길 때마다 코드를 수정해야 하지만 그것은 수식을 사용하거나 반복문을 사용할 때도 마찬가지가 아닌가?
저의 하드코딩을 선택한 판단이 충분히 타당한지 이에 대한 피케이의 생각도 궁금합니다!
There was a problem hiding this comment.
Q. UP UP UP UP UP UP UP UP UP 하드코딩, 괜찮은가
ㅋㅋㅋㅋ 나쁘지 않은 아이디어라고 생각합니다.
난해한 반복문보단 훨씬 이해하기 좋고, 말해주신 것처럼 뭔가 추가할 때도 어렵지 않게 할 수 있을 듯 해요.
루덴스의 주장이 이 상황에는 타당하다고 생각합니다.
그리고 **심지어 다 읽어보지 않고도 이해할 수 있는** 코드라고 해주셨는데...! 꽤나 자신감 넘치는 모습, 보기 좋습니다.
그런데 new Direction(List.of(UP, UP, UP, UP, UP, UP, UP, UP, UP)) 안에 있는 UP 의 개수는 몇 개일까요?
저는 커서 움직이며 하나씩 세어봤습니다. ㅎ
오히려 이런 구조였다면, 정말 안 세고도 몇 개인지 바로 알 수 있지 않았을까 싶어요:
Direction.straight(UP, 9)결론 : 하드코딩 아이디어 좋다. 그런데 읽어야 그 수를 알 수 있는 상황이긴 하다.
There was a problem hiding this comment.
Direction.straight(UP, 1),
Direction.straight(UP, 2),
Direction.straight(UP, 3),
Direction.straight(UP, 4),
Direction.straight(UP, 5),
Direction.straight(UP, 6),
Direction.straight(UP, 7),
Direction.straight(UP, 8),
Direction.straight(UP, 9),특정 방향으로 전진한다는 메서드를 만들어버리는 것은 생각을 못했네요..
팩토리 메서드로 Vector과 반복 횟수를 받아 Direction을 생성할 수 있도록 기능을 추가했습니다!
훨씬 깔끔하고 이해도 쉬운 것 같습니다!!!!! 👍
There was a problem hiding this comment.
장기 미션을 수행하면서 왜 이렇게 불변 객체에 대한 언급이 많을까? 하는 의문이 들었습니다.
불변 객체의 사용 이유들을 찾아보며 공부했고, 제가 생각하게 된 불변 객체 사용의 이유는 잘못된 동작의 책임을 동작 로직이 아니라 생성 자체로 확정지을 수 있다는 것이었습니다.
예시로, 어떤 기물의 동작이나 생성이 잘못되었을 때 게임이 진행되는 과정에서 그 영향이 확산되어 어떤 지점에서 잘못된 동작이 발생했는지 발견하기 힘들 수 있습니다.
하지만, 장기 관련 객체가 모두 불변이라면 특정 기물이 잘 못 생성되었다.라는 정보를 확실하게 알 수 있다는 것입니다.
과연 그렇다면 왜 모든 객체를 불변으로 사용하지 않는가? 하는 질문이 생깁니다.
JVM의 GC는 강력하기에 메모리 점유는 문제가 되지 않고, 장기 미션 수준에서는 모든 객체를 불변으로 설정한다.라는 조건을 충족하는 것이 충분히 가능할테니까요.
그럼에도 불변 객체의 사용을 선택으로 남겨놓는 것은 현업에서는 모든 값을 불변으로 사용한다는 것 자체가 매우 힘들기 때문인 것인지, 그 수준에서는 메모리 낭비에 대한 압박이 있는 것인지, 혹은 그저 취향 차이인 것인지에 대한 궁금증이 있습니다.
이에 대한 피케이의 의견이 궁금합니다!
There was a problem hiding this comment.
참고로 이는 구현이 거의 끝나갈 때 쯤, 장기판 자체를 불변으로 만들 수 있다는 사실을 알게 되면서 생긴 궁금증이었습니다.
제 코드에서도 장기판을 불변으로 만든다고 하면 머리가 아픈데, 실제 프로덕션 수준에서 그런 설계를 도입하는 것 자체가 상당한 오버헤드인가? 하는 생각도 듭니다..!
There was a problem hiding this comment.
Q. 불변은 취향인가 오버헤드인가 필수인가
Direction 하드코딩도 그렇고 꽤나 재밌는 도전 및 의문을 가지시는군요!
일단 취향 차이는 분명히 아니고, 말해주신 것처럼 이 정도 규모에서는 메모리 압박이 그리 크지 않습니다.
제가 생각했을 때 장기게임에서 불변이 선택사항인 이유는 크게 두 가지입니다 :
- 아직 그정도로 안 만들어도 되기 때문에
- 게임은 그 특성상 상태변화가 너무나도 당연하고, 그만큼 자주 일어나기 때문에
상태변화가 자주 일어나는 것들을 어떻게든 불변으로 만들면
얻는 이점보다 불편한 점이 더 크다는 것을 느낄 때가 있습니다.
코드도 복잡해지고, 일관성을 챙기기도 어려운데 그에 비해 실질적으로 느끼는 장점이 많지 않은거죠.
즉 불변은 '모든 것을 불변으로 만들겠다'는 생각으로 만든다기 보단,
'상태변화가 특징인 것들은 불변으로 만들지 않고, 그 외엔 불변 객체로 둬서 그 이점을 챙기겠다'는 생각으로 만드는 것 같네요. 이점은 루덴스가 잘 설명해주신 듯 하고요.
실제 개발을 할 때도, 그리고 더 나아가 Java 와 Spring 진영 로직들도 다 비슷한 생각으로 만들어집니다.
그래서 제 결론은 '굳이 모든 객체를 불변으로 만들 필요가 있을까? 필요에 의해 가변 객체를 만들 수도 있지' 입니다.
There was a problem hiding this comment.
피케이의 생각 들려주셔서 감사합니다!
처음엔 불변 객체를 사용하는 이유가 개발자의 실수를 막기 위해서 라는 것이 이해가 잘 안됐습니다.
불변으로 만들어 놓으면 수정이 안되니까 실수를 사전에 방지한다? 그렇다면 new로 만들어질 때 실수하는 것도 똑같이 위험하지 않은가? 하는 질문이 계속 생겼거든요.
지금은 그럼에도 불변 객체를 만드는 것이 좋은 이유를 구체적으로 알아가고 있지만,
잘못된 동작의 책임을 동작 로직이 아니라 생성 자체로 확정지을 수 있다는 것 등
그럼에도 조금의 모호함이 느껴지는 것 같습니다.
피케이의 코멘트에서도 조금의 의문이 생기는데요,
그래서 제 결론은 '굳이 모든 객체를 불변으로 만들 필요가 있을까? 필요에 의해 가변 객체를 만들 수도 있지' 입니다.
여기서 필요라는 것은 불변으로 만들기 위해 소비될 코드를 작성하는 시간, 바뀌어야 하는 코드, 사람의 스트레스 등 > 객체를 불변으로 만듦으로써 얻는 안정성인 상황인걸까요?
혹은 정말 도저히 불변을 사용할 수 없는 상황을 가정하는 것일까요?
| public BoardStatusDTO boardStatus() { | ||
| return janggiBoard.boardStatus(); | ||
| } |
There was a problem hiding this comment.
제 코드에서 DTO는 생성자로 원시값 -> DTO 매핑을 스스로 담당하고 있습니다.
이런 구조에서 도메인(Game)에서 controller로 반환하면서 DTO를 넘겨주는 것은 잘못된 설계인가요?
도메인 수준에서 DTO(View넘기는 데이터 매핑)을 담당하는 것은 잘못된 것이라고 보았지만, 이 경우 처럼 값을 DTO 생성자로 리턴하면서 만들어지는 DTO도 문제가 되는가? 하는 궁금증이 있습니다!
+추가
이전 미션에서도 리뷰어와 크루와의 대화를 통해 위와 같은 질문과 답변을 주고 받았지만, '그렇구나' 라는 생각이 들 뿐, 정말 이렇게 하지 않으면 불편하다., 확실히 문제가 있는 방식이다.라는 것이 체감되지는 않았습니다.
그것이 제 이해력이 부족해서인지, 이 정도 레벨에서는 책임 분리를 한다는 것이 오히려 불편함을 불러오기 때문인 것인지 저는 아직 잘 모르겠습니다.
혹시나 과거에 객체지향을 공부하거나 미션을 진행하면서 이런 생각을 해보셨다면, 이럴 때에 '어떤 상황을 가정하여 판단한다.' 혹은 '어떤 것을 기준으로 장단점을 판단해야 한다'하는 조언을 주신다면 제게 정말 큰 도움이 될 것 같습니다!
There was a problem hiding this comment.
Q. DTO 는 어디서 어떻게 만들고, 필요하기는 할까?
찰형(찰리형 ㅎ)과 재밌는 대화를 나누셨군요. 한참을 읽었습니다. ㅋㅋㅋ
우선 저는 우테코 당시 해당 미션을 진행 할 때, DTO가 뭔지도 몰랐습니다.
사람들마다 약간씩 말하는게 다 다르고 추상적이라 혼란스럽기도 했고요. (안타깝게도 그 땐 물어볼 AI 조차 없었습니다)
자 이제 루덴스의 질문을 하나씩 뜯어서 다뤄보죠. 마지막 부분부터 짚고 넘어가면 좋을 듯 해서 순서를 좀 바꿨습니다 :
'어떤 상황을 가정하여 판단한다.' 혹은 '어떤 것을 기준으로 장단점을 판단해야 한다'하는 조언을 주세요
다른 리뷰어와 치열한 토론을 하고 난 후 제게 남기는 꽤나 당돌한(?) 부탁이네요.
우선 저는 쉽고, 단순하고, 편안하며, 게으른 방식을 가장 좋아합니다.
그것이 유지보수에 가장 좋은 사고방식이라고 봐요.
그런 의미에서 지금처럼 TUI 로만 사용할 장기 게임을 만든다면, DTO 자체를 도입하지 않을 듯 합니다.
규모가 작으니까요. 제가 루덴스의 생각을 잘 파악했다면 아마 루덴스도 동의할거라 생각합니다.
정말 이렇게 하지 않으면 불편하다., 확실히 문제가 있는 방식이다.라는 것이 체감되지는 않았습니다.
당연하죠. 앞서 말했듯, 해당 프로젝트는 DTO의 필요성을 느끼지 못할 만큼 작은 규모의 프로젝트입니다.
뷰 또는 외부로 노출되는 API가 많지도 않고요.
지금 크루들이 너나 할 것 없이 DTO를 도입해보는 이유는
- 해당 애플리케이션이 추후 다른 view에 호환이 되어야 해서 여러 view가 생기고
- view 또는 api 관련 로직이 생겼을 때 이를 도메인과 분리해야 할 필요성이 생기며
- 예를 들어 엔터프라이즈 레벨 웹서비스는 http 프로토콜 관련 처리, json 처리, 같은 도메인이라도 다른 방식으로 정보를 서빙하는 등, 도메인과는 관련 없는 controller 관련 로직이 생기기 마련입니다.
- 도메인 또는 뷰에 변경이 자주 일어나서 유지보수 복잡도가 올라가는
이 상황을 대비하는 연습을 하는거라고 저는 생각합니다.
따라서 저는 어떤 상황을 가정하고 판단하진 않습니다.
당장 필요 없다면, 안 만들어요. DTO 말고도 신경 쓸 게 얼마나 많은데요.
나중에 필요할 때 도입하는게, 필요하지 않은데 미리 도입하는 것보다 덜 힘듭니다.
해당 미션은 연습을 해보는 단계니까 한 번쯤 다들 도입해보는거라고 보고요.
하지만 이왕 만든다면 잘 만들 것 같습니다. 그렇기 때문에,
이 경우 처럼 값을 DTO 생성자로 리턴하면서 만들어지는 DTO도 문제가 되는가? 하는 궁금증이 있습니다!
DTO가 직접 자신이 전달하고 싶은 정보를 골라 담거나, 원하는 형태로 변형하는건 문제가 아니라고 생각합니다.
하지만 도메인이 DTO에 의존하는건 경계 해야 할 부분이에요.
앞서 같은 도메인이라도 다른 방식으로 정보를 서빙 할 수 있다고 했죠?
그러면 같은 도메인일지라도 다양한 DTO에 엮이게 됩니다. 그 도메인 말고도 수많은 도메인이 같이 쓰일 수도 있고요.
그럴 때 도메인에서 DTO 생성자를 활용하여 이를 관리하게 되면,
도메인이 애플리케이션 로직을 담당해야하는데 DTO 관련 로직 및 메서드만 무수히 담당하게 되는 일이 발생합니다.
그리고 그 수가 많아지고, 관리하는 개발자가 많아지면... 지금은 당연하게 보이는 해당 메서드의 위치도 그 때 누군가는 당연하게 생각하지 않아요. 그러면 안그래도 DTO 관련 로직이 많은데 중복까지 발생합니다. DTO 시그니처가 변경되거나, 패키지가 바뀌거나, 삭제될 때 도메인에도 변경사항이 생기고요.
이런 구조에서 도메인(Game)에서 controller로 반환하면서 DTO를 넘겨주는 것은 잘못된 설계인가요?
앞서 말했듯, 이 규모의 애플리케이션에서는 DTO 자체를 도입하지 않을 듯 하여 어떻게 만들어도 잘못되었다고 보진 않을 것 같습니다. 루덴스의 이 방식이 개발을 너무 어렵게 만든다던가, 앞서 말한 것처럼 도메인 복잡도를 엄청 크게 늘리는게 아니니까요.
하지만 이왕 만드는거 제대로 해본다면, 앞서 길게 말한 이유들 때문에 지양할 듯 합니다.
그냥 controller에서 domain -> dto 변환 작업을 하고 간단하게 유지해볼 것 같네요.
코멘트가 굉장히 길어졌는데, 이해가 안가거나 동의하지 않는 부분이 있다면 편하게 추가 코멘트 남겨주세요 :)
There was a problem hiding this comment.
긴 답변 너무 감사합니다 피케이!!
해주신 말씀이 제게 너무 위로가 되었습니다 🥹
저는 우테코에 올 때 객체 지향 = 자바 정도로 알고 있었어요.
블랙잭 미션을 하면서 처음 객체지향 설계에 대해서 고민했지만, 그 과정이 제게는 정말 힘들고 혼란스웠어요.
이 정도 과제에 객체 지향 방법론을 도입해야할 이유 자체를 찾지 못했었거든요.
제가 그나마 공부해본 분야가 알고리즘이었기에, 더 그렇게 느꼈던 것 같아요.
객체 지향에 대해 아는 것은 없고, 크루들은 내가 뜻도 모르는 단어들을 대며 설명하는데 그 뜻을 알게되더라도 이유를 납득하는 것은 더 어렵고..
제 의견만을 강하게 관철하자니 배움으로부터 멀어지는 것 같고, 다른 사람들의 말을 무조건적으로 수용하자니 객체지향을 위한 객체지향 코드가 만들어지는 것 같았습니다.
제가 아직 깨닫지 못한 무언가를 계속 찾으려고 애썼고, 그 과정에서 정답이 없을 수도 있겠다는 생각이 조금씩 들었던 것 같아요.
그런 생각들이 피케이의 답변을 읽은 지금에서야 정리되는 느낌입니다.
스스로 깊게 고민하고, 결정하여 만들어낸 코드를 남들에게 설명하고 타협하는 과정 자체가 객체 지향을 다루는 올바른 자세이고, 그 과정에서 부딪히며 배우는 것들이 진정으로 값진게 아닐까 생각하게 됐습니다. (물론 그 과정에서 새로운 지식들을 최대한 받아들이려고 애써야겠지만요 ㅎㅎ)
There was a problem hiding this comment.
public Map<Point, Intersection> boardStatus() {
return Map.copyOf(intersections);
}View로 보드의 상황을 보낼 때 도메인 (JanggiBoard)이 DTO를 직접 참조하지 않도록 수정했습니다!
보드의 현재 상태를 담는 BoardStatus 객체를 만들까 고민했지만, intersections는 행위도 하지 않고, 그저 DTO까지 전달될 뿐이며 Map불변, Point불변, Intersection불변이므로 래핑 객체를 따로 만들지 않고 copyOf로 전달만 하도록 했습니다.
(원래는 만드는게 좋아보이니 일단 만들고 생각했을 것이지만 피케이의 조언대로 용기 냈습니다 😊)
There was a problem hiding this comment.
** 수정
문제점을 발견하여 Map.copyOf로 Map을 리턴하지 않고 BoardState / IntersectionState 객체를 생성하여 view로 안전하게 값을 전달하도록 수정했습니다!
문제점:
public class Intersection {
private final Point point;
private Piece piece;Intersection이 가지는 Piece는 가변이며, Map.copyOf로 리턴하더라도 Point로 얻어낸(get) Piece에서 값을 조작할 수 있다는 점에서 view에서 도메인을 조작할 수 있는 위험성이 존재한다고 판단했습니다.
IntersectionsState는 Point, PieceType, Team을 불변으로 가집니다.
public record IntersectionState(
Point point,
PieceType pieceType,
Team team
) {
}| return point; | ||
| } |
There was a problem hiding this comment.
과제 요구 조건에 게터/세터/프로퍼티를 쓰지 않는다.라는 항목이 존재합니다.
저는 Getter를 쓰지 않는다는 항목을 지키지 못했습니다.
이에 대해 어떻게 하면 getter를 삭제할 수 있을까 고민하고 정보를 찾아보았지만, 그나마 제가 알게된 답은 State를 꺼내 쓰지 말고, 행동을 시키는 방식을 사용해야 한다. 였습니다.
하지만, 저는 내 코드에서 그것이 가능한가? 하는 의문이 들었습니다.
예로, Point라는 객체는 불변 객체이며 단지 좌표를 나타내는 상태로서 존재합니다.
그런 Point에게 Behavior을 담당하게 하는 것이 가능한가? 하는 의문이 듭니다.
설계 단계부터 잘못되었다면 바로잡을 수 있지만, 현재 제 코드를 기준으로 Getter가 없는 것은 어떤 방식으로 실현될 수 있는지 알고 싶습니다.
There was a problem hiding this comment.
Q. getter 를 사용하지 않는 방법
getter 는 정말 할 얘기가 많은데,
이 부분만큼은 루덴스가 해당 미션 뿐만 아니라 앞으로 다른 미션에서도 계속해서 직접 경험해봤으면 합니다.
getter 를 줄이는 경험을 해보면서 설계 방식을 발전시켜보는게 정말 중요한 것 같거든요.
관련해서 크루들과 토론하며 서로의 구현 방식을 보는 것도 다 도움이 될 겁니다.
할 얘기가 많은 만큼 말보단 경험으로 느껴보는게 더 값질거라 생각해요.
관련해서 제 생각을 간단히 말해보자면 : 모든 getter를 어떻게든 없애는 게 목표가 아니에요. "이걸 꺼내서 내가 하고 있는데, 해당 객체가 직접 하면 안 되나?" 한 번씩 생각해보는 게 이 요구사항의 핵심이라고 봐요. 물론 요구사항을 최대한 지키는게 정말 중요하다고도 생각하지만, 적어도 이 요구사항의 진짜 의미는 이거라고 봅니다. (개인적인 의견이에요)
There was a problem hiding this comment.
소중한 조언 너무 감사합니다 피케이!
먼저, 저의 결론은 다음과 같습니다.
- View를 사용하는 구조에서 getter를 완전히 없애기는 힘드며, getter의 사용이 꼭 나쁜 것은 아니다. (Immutable 데이터를 읽는 경우)
- 그럼에도 책임이 잘 분리된 좋은 설계에서는 getter를 최소한으로 사용할 수 있으며, 요구사항에서의 getter 사용 금지는 이러한 의도로 설정되었을 것
그리고 저의 질문은 이것입니다.
- 그렇다면 getter 사용에 대한 기준 또한 트레이드 오프인가?
피케이가 말씀 주신
이걸 꺼내서 내가 하고 있는데, 해당 객체가 직접 하면 안 되나?"
에 대해 깊게 생각해보고 제 코드에서 getter가 사용되는 이유를 분석하니 Intersection객체에 좌표와 기물이라는 성격이 다른 객체가 속해있기 때문에 Intersection을 원하는 방향으로 가져다 쓰기 위해 getter가 생긴다는 것을 알았습니다.
그래서 getter를 줄이는 방식을 고민했지만, 이를 위해 MoveRule에서는 더 많은 필드를 필요로 했고, 경로 탐색 방식도 훨씬 복잡해진다는 것을 알았습니다.
저는 그런 복잡한 코드가 getter보다 나쁜가? 하는 의심이 들었는데, 이 정도의 getter(불변 VO이며 다른 데이터 오염이 불가능한)는 편의를 위해 사용하는게 괜찮을지 궁금합니다!
| public List<Point> findPoints(Point from, Point to) { | ||
| for (Direction direction : directions) { | ||
| if (direction.canReach(from, to)) { | ||
| return direction.getPoints(from); | ||
| } | ||
| } | ||
| throw new IllegalArgumentException(); | ||
| } |
There was a problem hiding this comment.
코드를 확인하다 보니 방금 Depth가 1을 넘는 부분을 확인했습니다.
이는 제가 리뷰 요청을 올린 뒤 바로 수정해보겠습니다.
++질문
저는 이 과제의 조건들이 상당히 빡빡하다고 느낍니다.
우테코는 왜 이런 조건을 설정했는가? 하는 질문에 저는 짐작할 뿐이지만 다음과 같이 생각합니다.
- 어려운 조건을 충족하는 코드를 작성하는 사람은 조건 없는 환경에서도 좋은 코드를 작성할 수 있기에
- 회사에서도 위와같은 조건들이 실제로 적용되기에
혹시 피케이는 이에 대한 답을 알고 있으신가요? 혹은 이에 대해 피케이만의 생각을 가지고 계신가요?
There was a problem hiding this comment.
Q. 왜 요구사항이 이렇게 빡빡한가?!
맞아요. 요구사항이 꽤 빡빡합니다.
과연 나는 우테코 이후에 이 요구사항을 다 지키는가? 하면 아니라고 당당하게 말할 수 있습니다 ㅎ
왜 이렇게까지 하는가에 대한 루덴스의 생각을 말해주셨는데, 제 생각은 간단해요 :
'객체지향'이라는 개념을 직접 경험하고 학습하기엔 꽤나 좋은 방식같다.
따라서 미션 중에는 이 요구사항을 최대한 지키려고 하는게 본인에게 큰 도움이 될 겁니다.
마치 모래주머니 다리에 묶고 달리기 연습을 하는거라고 보면 어떨까 싶어요 ㅋㅋㅋ
그런 의미에서 루덴스가 depth 1 넘는 부분을 확인하고 수정하려는 모습이 정말 보기 좋습니다 👍
저라면 적어도 미션 기간동안에는 어떻게든 지키려고 노력할 것 같아요.
There was a problem hiding this comment.
강해지기 위함이군요,,, 👍
해당 코드는 Stream 사용하여 depth를 낮추고 가독성을 높였습니다!
public List<Point> findPoints(Point from, Point to) {
return directions.stream()
.filter(direction -> direction.canReach(from, to))
.findFirst()
.map(direction -> direction.getPoints(from))
.orElseThrow(IllegalArgumentException::new);
}
pkeugine
left a comment
pkeugine
left a comment
There was a problem hiding this comment.
안녕하세요 루덴스, PK입니다.
리뷰하는데 시간이 좀 오래 걸렸죠?
장기 애플리케이션이 유독 뭔가 확인하는데 시간소요가 좀 있더라고요.
루덴스의 기존 활동을 확인하는데 시간을 좀 쓰기도 했구요.
기다려 주셔서 감사합니다.
이번 리뷰는 다음과 같은 것들을 중점으로 봤어요 :
- 루덴스의 질문에 대한 답변 (여기에 시간이 가장 많이 투자됐습니다)
- 루덴스가 가감없이 의도를 묻고, 엄격하게 지적해달라고 한 것 정리
코멘트 확인하시면 제가 몇 가지 의도를 물어본 것들이 있을겁니다.
한 번 확인해주세요.
그리고 질문이 꽤 흥미로워서 저도 재밌게 작성했던 것 같아요.
제 답변에서 그런 모습이 드러났으면 하네요.
그리고 일단 코드 가독성을 신경써주셔서, 읽기 정말 편했습니다.
다만! 직설적이고 날카로운 피드백 (당신이 요청한 것) 을 하나 해보자면,
공백, 접근제어자, 테스트명, 메서드명, modifier 순서, final keyword 유무 등,
이런 기본 중 기본 요소들을 조금 더 신경 써보면 어떨까합니다.
어느 개발자가 아무리 코드가 좋고, 완성도가 높더라도
modifier 순서를 틀린다던가 (private final static), 불필요한 공백이 있다던가, 테스트명이 제각각이면, 마치 연애편지에서 '되' / '돼' 맞춤법 틀리는 것처럼 뭔가 팍 식거든요. 😂
지금부터 이런 것들을 잘 챙겨보는 습관을 가져보면 좋을 듯 해요.
앞으로의 사이클에서도 제가 매의 눈으로 찾아보겠습니다. ㅎ
그래도 질문도 재밌고, 잘 만들어주셔서 즐겁게 리뷰했습니다.
질문이나 추가로 얘기해보고 싶은게 있다면 코멘트나 DM 편하게 남겨주세요.
앞으로 남은 장기 미션 함께 잘 진행해봅시다.
해피해킹하세요! 🏇 🏇 🏇
| public class TestIntersectionGenerator implements IntersectionGenerator { | ||
| List<Intersection> intersections; |
There was a problem hiding this comment.
IntersectionGenerator 인터페이스를 테스트 전용으로 구현한 거 좋습니다.
이렇게 하면 프로덕션 코드에 테스트용 메서드를 추가하지 않아도 되고,
원하는 보드 상태를 자유롭게 만들 수 있네요.
인터페이스를 활용한 테스트 설계의 좋은 예시 같아요 👍
There was a problem hiding this comment.
이 부분은 페어의 의견으로 생겨난 설계입니다.
처음에는 인터페이스를 만들어 메인 코드와 테스트 코드에 모두 활용한다는 것이 신기했습니다.
이런 지식은 경험이라기보다는 책을 읽어야 알 수 있는게 아닐까 하는 생각이 들었는데요,
혹시 피케이가 추천하는 책이 있는지 궁금합니다!
(현재는 객체지향의 사실과 오해 / 자바의 정석 2개의 책을 읽고 있습니다!)
| public abstract class MoveRule { | ||
|
|
||
| protected final PieceType pieceType; | ||
| protected final Directions directions; |
There was a problem hiding this comment.
MoveRule이 위치 같은 상태를 갖지 않는 순수한 규칙 객체인 점이 좋아요.
이동 전략이 현재 위치를 내부에 들고 다니면 매 이동마다 상태를 갱신해야 하고,
그 과정에서 동기화 문제가 생기기 쉽거든요.
규칙과 상태를 분리한 건 좋은 판단이라고 생각합니다 👍
한 가지 :
abstract class의 생성자가 public인데,
어차피 서브클래스에서만 super()로 호출하니까 protected가 의도에 더 맞을 듯 하네요.
There was a problem hiding this comment.
MoveRule의 접근제어자를 protected로 수정했습니다.
추상 클래스는 외부 확장을 전제로 하는 것(프레임워크 등)이 아니라면 protected를 사용하는 게 좋다는 것을 알게 되었습니다!
| public BoardStatusDTO boardStatus() { | ||
| return janggiBoard.boardStatus(); | ||
| } |
There was a problem hiding this comment.
Q. DTO 는 어디서 어떻게 만들고, 필요하기는 할까?
찰형(찰리형 ㅎ)과 재밌는 대화를 나누셨군요. 한참을 읽었습니다. ㅋㅋㅋ
우선 저는 우테코 당시 해당 미션을 진행 할 때, DTO가 뭔지도 몰랐습니다.
사람들마다 약간씩 말하는게 다 다르고 추상적이라 혼란스럽기도 했고요. (안타깝게도 그 땐 물어볼 AI 조차 없었습니다)
자 이제 루덴스의 질문을 하나씩 뜯어서 다뤄보죠. 마지막 부분부터 짚고 넘어가면 좋을 듯 해서 순서를 좀 바꿨습니다 :
'어떤 상황을 가정하여 판단한다.' 혹은 '어떤 것을 기준으로 장단점을 판단해야 한다'하는 조언을 주세요
다른 리뷰어와 치열한 토론을 하고 난 후 제게 남기는 꽤나 당돌한(?) 부탁이네요.
우선 저는 쉽고, 단순하고, 편안하며, 게으른 방식을 가장 좋아합니다.
그것이 유지보수에 가장 좋은 사고방식이라고 봐요.
그런 의미에서 지금처럼 TUI 로만 사용할 장기 게임을 만든다면, DTO 자체를 도입하지 않을 듯 합니다.
규모가 작으니까요. 제가 루덴스의 생각을 잘 파악했다면 아마 루덴스도 동의할거라 생각합니다.
정말 이렇게 하지 않으면 불편하다., 확실히 문제가 있는 방식이다.라는 것이 체감되지는 않았습니다.
당연하죠. 앞서 말했듯, 해당 프로젝트는 DTO의 필요성을 느끼지 못할 만큼 작은 규모의 프로젝트입니다.
뷰 또는 외부로 노출되는 API가 많지도 않고요.
지금 크루들이 너나 할 것 없이 DTO를 도입해보는 이유는
- 해당 애플리케이션이 추후 다른 view에 호환이 되어야 해서 여러 view가 생기고
- view 또는 api 관련 로직이 생겼을 때 이를 도메인과 분리해야 할 필요성이 생기며
- 예를 들어 엔터프라이즈 레벨 웹서비스는 http 프로토콜 관련 처리, json 처리, 같은 도메인이라도 다른 방식으로 정보를 서빙하는 등, 도메인과는 관련 없는 controller 관련 로직이 생기기 마련입니다.
- 도메인 또는 뷰에 변경이 자주 일어나서 유지보수 복잡도가 올라가는
이 상황을 대비하는 연습을 하는거라고 저는 생각합니다.
따라서 저는 어떤 상황을 가정하고 판단하진 않습니다.
당장 필요 없다면, 안 만들어요. DTO 말고도 신경 쓸 게 얼마나 많은데요.
나중에 필요할 때 도입하는게, 필요하지 않은데 미리 도입하는 것보다 덜 힘듭니다.
해당 미션은 연습을 해보는 단계니까 한 번쯤 다들 도입해보는거라고 보고요.
하지만 이왕 만든다면 잘 만들 것 같습니다. 그렇기 때문에,
이 경우 처럼 값을 DTO 생성자로 리턴하면서 만들어지는 DTO도 문제가 되는가? 하는 궁금증이 있습니다!
DTO가 직접 자신이 전달하고 싶은 정보를 골라 담거나, 원하는 형태로 변형하는건 문제가 아니라고 생각합니다.
하지만 도메인이 DTO에 의존하는건 경계 해야 할 부분이에요.
앞서 같은 도메인이라도 다른 방식으로 정보를 서빙 할 수 있다고 했죠?
그러면 같은 도메인일지라도 다양한 DTO에 엮이게 됩니다. 그 도메인 말고도 수많은 도메인이 같이 쓰일 수도 있고요.
그럴 때 도메인에서 DTO 생성자를 활용하여 이를 관리하게 되면,
도메인이 애플리케이션 로직을 담당해야하는데 DTO 관련 로직 및 메서드만 무수히 담당하게 되는 일이 발생합니다.
그리고 그 수가 많아지고, 관리하는 개발자가 많아지면... 지금은 당연하게 보이는 해당 메서드의 위치도 그 때 누군가는 당연하게 생각하지 않아요. 그러면 안그래도 DTO 관련 로직이 많은데 중복까지 발생합니다. DTO 시그니처가 변경되거나, 패키지가 바뀌거나, 삭제될 때 도메인에도 변경사항이 생기고요.
이런 구조에서 도메인(Game)에서 controller로 반환하면서 DTO를 넘겨주는 것은 잘못된 설계인가요?
앞서 말했듯, 이 규모의 애플리케이션에서는 DTO 자체를 도입하지 않을 듯 하여 어떻게 만들어도 잘못되었다고 보진 않을 것 같습니다. 루덴스의 이 방식이 개발을 너무 어렵게 만든다던가, 앞서 말한 것처럼 도메인 복잡도를 엄청 크게 늘리는게 아니니까요.
하지만 이왕 만드는거 제대로 해본다면, 앞서 길게 말한 이유들 때문에 지양할 듯 합니다.
그냥 controller에서 domain -> dto 변환 작업을 하고 간단하게 유지해볼 것 같네요.
코멘트가 굉장히 길어졌는데, 이해가 안가거나 동의하지 않는 부분이 있다면 편하게 추가 코멘트 남겨주세요 :)
| private Point parsePoint(String input) { | ||
| String[] tokens = input.trim().split("\\s+"); | ||
| int integerY = Integer.parseInt(tokens[0]); | ||
| int integerX = Integer.parseInt(tokens[1]); | ||
|
|
||
| return new Point(integerY, integerX); | ||
| } |
There was a problem hiding this comment.
domain이 DTO에 의존하는 것에 대해 얘기해봤는데, 반대로 이 부분도 생각해보면 재밌을 듯 합니다.
현재 InputPointDTO가 내부에서 직접 new Point(y, x)를 호출해서 도메인 객체를 생성하고 있어요.
파싱, 검증, 도메인 객체 생성까지 DTO가 전부 하고 있는 건데, DTO의 역할이 맞을까요?
DTO가 domain에 의존하는 것에 대해 어떻게 생각하는지도 알려주시면 좋을 듯 하고요 :)
질문의 이유는, 저는 이 역시도 의존하면 안된다고 생각하기 때문이에요.
DTO는 데이터를 담아서 옮기는 역할이지, 도메인 객체를 만드는 팩토리가 아니거든요.
|
|
||
|
|
There was a problem hiding this comment.
불필요한 빈 줄이 있어요. 사소하지만 이 역시 코드를 깔끔하지 않게 보이게 하니 정리하고 신경 써보면 좋을 듯 해요.
| private Map<Point, Intersection> fillEmptyIntersections() { | ||
| return getAllPoints() | ||
| .collect(Collectors.toMap(point -> point, Intersection::empty)); | ||
| } |
There was a problem hiding this comment.
일부 클래스의 메서드 순서가 조금 뒤섞여 있어요.
public -> private, 또는 호출 순서대로 배치하는 등 본인만의 컨벤션을 만들어보면 어떨까요?
파일이 길어질수록 일관된 순서가 가독성에 큰 차이를 만들어요.
관리하는 개발자가 많아질 때 특히 유용하고요!
There was a problem hiding this comment.
컨벤션을 확인하고 필요한 부분에 반영했습니다!
우선순위:
1순위 - 필드
2순위 - 생성자
3순위 - 생성자 전용 private
4순위 - public 메서드
클린코드에서 신문 기사처럼 작성하라는 지침이 있어서 읽어보고 참고했습니다!
| GENERAL("楚", "漢"), | ||
| CHARIOT("車", "車"), | ||
| CANNON("包", "包"), | ||
| HORSE("馬", "馬"), | ||
| ELEPHANT("象", "象"), | ||
| GUARD("士", "士"), | ||
| SOLDIER("卒", "兵"), | ||
| NONE("＋", "＋"), |
There was a problem hiding this comment.
한자 표현은 view 관련 요소가 아닐까요?
만약 다른 view를 사용한다면 한자가 아니라 이미지나 다른 형태를 쓸 수도 있으니까요.
실제로 이 값은 BoardStatusDTO에서 사용되고 있어서,
더더욱 도메인보다는 view 쪽 관심사라는 생각이 들어요.
루덴스의 생각은 어떤가요?
There was a problem hiding this comment.
한자와 한글(한, 초)은 도메인 로직에서 전혀 사용되지 않고, view로 전달하기 위해서만 존재하므로 view로 책임을 넘기는게 맞다고 생각했습니다!
view에서 한자와 한글 출력을 담당하도록 수정했습니다.
private static final Map<PieceType, String> CHO_PIECE_CHINESE_CHARACTER_MAP = Map.of(
PieceType.GENERAL, "楚",
PieceType.CHARIOT, "車",
PieceType.CANNON, "包",
PieceType.HORSE, "馬",
PieceType.ELEPHANT, "象",
PieceType.GUARD, "士",
PieceType.SOLDIER, "卒",
PieceType.NONE, "＋"
);
private static final Map<PieceType, String> HAN_PIECE_CHINESE_CHARACTER_MAP = Map.of(
PieceType.GENERAL, "漢",
PieceType.CHARIOT, "車",
PieceType.CANNON, "包",
PieceType.HORSE, "馬",
PieceType.ELEPHANT, "象",
PieceType.GUARD, "士",
PieceType.SOLDIER, "兵",
PieceType.NONE, "＋"
);
private String getTeamName(Team team){
if(team == Team.CHO){
return "초(楚)";
}
return "한(漢)";
}|
|
||
| private final List<PointInfoDTO> boardStatus; | ||
|
|
||
| public BoardStatusDTO(final Map<Point, Intersection> intersections) { |
There was a problem hiding this comment.
생성자 파라미터에 final을 붙이셨네요.
다른 곳에서는 안 붙인 곳도 있는 것 같은데,
final을 붙이는 루덴스의 기준이 무엇인지 궁금합니다.
There was a problem hiding this comment.
다른 코드는 페어와 함께 빨리 작성하느라 신경쓰지 못했었는데, 혼자 코드를 작성할 때 추가한 파일이라서 파라미터에 final을 붙이게 되었습니다.
본래 final을 붙이면 좋다는 정도로만 알고 있었지만, 미션을 진행하면서 상태가 변경되지 않는 것의 장점에 대해 알게되어 최대한 final을 사용해보려고 했습니다.
(하지만 페어와 함께 진행했던 코드들은 아직 손대지 못했네요...)
추가로, 제가 생각하게 된 가능하면 final을 붙여야 하는 이유에 대해서도 한 번 읽고 의견 주시면 감사할 것 같습니다!
불변 객체를 사용하는 이유 -> 데이터의 조작을 생성 시점으로만 제한하여 잘못된 동작을 최대한 줄인다.
지역변수와 매개변수에 final을 붙이는 이유 -> 동작(메서드)가 진행되는 중에
잘못된 동작(개발자의 실수)이 추가되지 않도록 하기 위해.
마치 getter를 줄이라는 요구사항의 의도와 유사하다고 생각
Getter가 존재한다.
-> 객체가 스스로 알고있는 것 만으로 동작을 해결하지 못하고 의존하고 있으므로 책임을 분리해야 한다.
final을 사용할 수 없는 코드가 존재한다.
-> 재할당이 필요한 데이터가 많다는 것이고, 개발자의 실수로 잘못된 동작이 발생될 확률이 존재하므로
재할당의 방법보다 새로운 객체로 표현하는 방식을 고민할 수 있다.
3 participants
체크 리스트
test를 실행했을 때, 모든 테스트가 정상적으로 통과했나요?리뷰어에게 전하고 싶은 말
안녕하세요, 피케이! 이번 미션을 함께하게 된 루덴스입니다. 잘 부탁드려요 ❤️🔥
먼저, 이번 미션을 진행하며 스스로의 부족함을 많이 깨달았습니다.
특히 페어와 설계 방향을 논의하는 과정에서, 페어가 제시하는 디자인 패턴이나 문제 해결 방법론들에 대한 지식이 다소 부족하여 동등한 위치에서 토론하기보다 이를 이해하고 따라가는 데 많은 에너지를 썼던 것 같습니다.
즉, 로직은 구현해냈지만, '이 설계가 최선인가?' 혹은 '왜 이 패턴이어야만 했는가?'라는 근본적인 질문에 대해서는 아직 스스로 답을 찾아가는 과정에 있습니다.
그렇기에 이번 리뷰에서 제가 작성한 코드의 의도를 가감 없이 묻고, 설계상의 허점이나 개선점이 있다면 엄격하게 지적해 주셨으면 합니다.
직설적이고 날카로운 피드백일수록 저에게 많은 배움의 기회가 생길 것 같습니다. 저는 장기 미션에서 많은 것들을 배워가고 싶습니다.
코멘트로 코드에 대해 설명하고 싶은 점들을 정리해놓겠습니다. 바쁘시겠지만 꼼꼼한 리뷰 부탁드립니다. 정말 감사합니다!