Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[1, 2단계 - 체스] 져니(이지원) 미션 제출합니다. #485
[1, 2단계 - 체스] 져니(이지원) 미션 제출합니다. #485
Changes from 88 commits
5274392
03ee6d5
21c2ca1
0d3d52b
70d1806
b74cd59
dc81448
1a837b3
71a8a9b
248d295
03c9a07
8c8c198
d340443
a55dd78
f073c49
fdff963
af16004
6ae4743
0d03670
1b8fd8c
4472050
2ea85ee
aecdcf9
a7d2f08
0d8d1c0
2063c36
62f5fb5
26a7030
0bbac7f
f2d3668
ed9ac2b
fd4667c
144ce4e
7f0ebd0
ed54a8c
c07d5eb
1f137d1
0f01ec7
22beb9e
d556de0
f2bb1c1
edb495e
be1433f
f07c41d
4e4f5b0
689228d
547e146
27ca651
045aca6
7605501
77a5101
19f8ede
7bcec7b
11ce918
45b6bf4
7578073
3250f4f
b7c8649
c7b3c4c
1e46715
ee5169c
f8876f1
921b7ee
dc5b490
d4f94a0
95a7c9e
5e405a6
334312a
48511d1
062a2d4
e19dabf
bc1d707
320e7cf
6872471
4fcc154
50dcdb3
972fb93
e3e4fab
69ded27
c0a18ac
b8b123a
947f6a0
5a2d334
671085f
f857825
3807a70
9a26d3d
fb354ed
5d34902
64c1803
a1438cb
fc3e731
1b2d7e9
02cbed3
aa3786a
cf6475c
36ee143
eb36fae
26e3db4
8814a1a
d75f190
1753cf5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적으로는 dto로 변경하는 로직 조차도 엄격하게 도메인을 모르게만들고싶은데,,ㅎ 요거는 제 욕심이라 의견으로만 남겨둡니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서비스 레이어를 두면 좋을 것 같은데, 그런 의도로 말씀 주신 거 맞을까요~?! 생성해두겠습니다 ㅎㅎㅎ 👍
+) 그런데 혹시, dto 역시 도메인을 모르게 하는 이유가 있을까요? 사실 dto는 도메인의 필드값들과 밀접하게 관련이 있을 수밖에 없기 때문에 도메인에 대한 업데이트가 발생하면 dto도 변경이 발생하는 게 당연하다고 생각이 들었거든요!
아니면, 이런 이유일까요?
[dto가 도메인을 알고 있는 경우]
[dto가 도메인을 모를 경우 - 필드로 각 요소를 주입받음]
제가 제대로 이해한 게 맞는지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 의도한거는 서비스를 만들라는 것은 아니였고, controller 에서 직접 new BoradDto(..) 선언해서 사용하는 것을 말씀드린거에요!
아니면 Mapper class를 만들어서 해당 클레스에서 Domain을 받고 Dto를 리턴해주는 형태로사용하던가요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그리고, 지금 서비스를 잘못사용하고 계신것 같습니다. 현재 서비스를 dto를 만드는 factory처럼 사용하고 계시는데요ㅎㅎ.. 서비스의 역할은 이게 아닙니다.
서비스의 역할을 찾아보시면 좋을 것 같아요.
서비스는 비지니스 로직의 흐름을 담당하는 클래스입니다. dto를 만드는 factory 역할을 하는게 아닙니다!
원래는 서비스가 없다면 Controller에서 비지니스로직을 호출하여 사용하게 될텐데요. 애플리케이션이 커지면, 복잡해지기 때문에 이런 흩어진 로직을 하나로 묶어서 고수준의 형태로 추상화해서 사용하는 것이 서비스입니다.
따라서 비지니스 로직의 흐름을 고수준으로 묶기 때문에 서비스의 메서드는 하나의 applicaiton 의 기능을 하게 되는것이죠.
따라서 해당 구현들은 지금 잘못된것 같아요. 네이밍을 그냥 Mapper 기능을 하고 있어서 네이밍만 변경해서 사용하시면될 것 같아요.
그리고 Mapper의 utils 성 클래스이기 때문에, 인스턴습변수도 필요 없어질 것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dto 는 view <-> controller 사이에서 도메인의 의존을 끊기 위해서 사용하는 것인데, dto가 도메인을 알면 사용하는 의미 자체가 퇴색되는것 같아요.
이런 의존성을 끊어주는 이유는 서로의 변경사항을 직접적으로 영향받지 않도록 하기 위함입니다.
domain이 변경돼서 view가 직접적으로 영향을 받거나, view의 변경으로 domain이 직접적으로 영향을 받거나 하는 상황이요.
도메인이 변경돼서 view가 아니라 dto가 변경이되는거면 의존성을 잘 끊어준것이 되겠죠?
만약 만약에
이동욱님 블로그 글을 참고로 달아둘게요.
이런 이유들은 지금같은 콘솔프로그램같은 작은 서비스에선 와닿지 않을거에요!.. 이해가 안되더라도 경험하시면서 이해하도 좋을 것 같아요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서브웨이의 의견을 읽고 제가 이해한대로 말씀을 드려도 괜찮을까요?
1. 서비스의 역할에 대해
서비스는 “비즈니스 로직”을 담는다.
이유 -> 컨트롤러에서 호출하기 위한 비즈니스 로직들을 모아서 관리하기 위해 + 추상화 목적
여기서 말씀해 주신 추상화의 목적은 다음과 같을까요?
ex) 주문 관리 서비스
이런 식으로 생각하면 될까요?
2. dto에서 도메인을 알면 안 되는 이유
아래에서 설명해주신 이유는 뷰가 도메인 자체를 의존하면 안 되는 이유라고 생각해요. 그래서 dto를 사용하는 것까지는 저도 동의하는 바예요. (특히 웹 서비스로 가게 된다면 당연히 엔티티의 모든 내용을 내려주는 건 안 되니까 dto를 통해서 반환하는 것이겠구요. 순환 참조의 문제도 있을 것이구요.)
다만, 제가 궁금한 건 dto로 변환하는 로직에서, dto가 도메인을 자체를 가지고 변환 작업을 하면 안 되는 이유가 궁금했어요. 서브웨이는 dto도 일종의 뷰의 영역으로 보고 말씀하시는 것일까요?…
그렇다면 도메인의 변화가 생겼을 때 뷰와 밀접하게 관련이 있는 dto가 바로 영향을 받도록 하지 않고, 한 단계를 두어서 변화를 최대한 받지 않도록 하기 위해서라고 보면 될까요??…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우선 서비스가 dto를 변환하는 건 옳지 않다는 것까지는 이해했어요! 다만... 별도의 매핑 클래스까지 두는 게 맞을까? 라는 의문이 들었습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1번
네 맞습니다.
서비스가 없으면 컨트롤러에서 세부 구현들이 들어가겠죠!
이런 행위들이 분산될 거고, 이걸 하나의 기능으로 추상화해서 사용하기 위해 서비스를 사용합니다. 따라서 서비스 하나의 애플리케이션 기능을 나타낸다고 할 수 있고, 비지니스 로직의 흐름을 담당한다고 할 수 있습니다.
2번
public static BoardDto from(final Map<Position, Piece> board)
이처럼 변환하는 로직 자체를 왜 알지 못하게 해야하는가? 에대한 질문인가요?저는 dto에 대해서 이러한 생각을 갖고 있습니다.
domain의 영향이 dto에 영향을 끼쳐서라기보단, 위와 같은 이유로dto가 domain을 알아야 하나 라는 생각을 했습니다.
현재 변환로직이 단순 생성자로 변환하는것 말고는 따로 없고, 변환로직이 들어가는 것에 대해 어떻게 생각하느냐에 따라 달라질 것 같아서 그냥 의견으로 드렸던 것입니다!
만약 변환하는 메서드가 로직을 갖고 있었다면, 변경해달라고 리뷰를 하긴했을 것 같네요!