[Spring MVC] 서현진 3,4단게 미션제출합니다. #519
Conversation
dooboocookie
left a comment
dooboocookie
left a comment
There was a problem hiding this comment.
안녕하세요, 현진.
리뷰어 루카입니다.
이번 리뷰는 어떤 방식으로 리뷰를 드릴지 조금 고민이되네요.
질문에 대한 답부터 먼저 드리겠습니다.
스프링의 컨테이너가 String constant pool 같은건가요?
@service로 만든 서비스 클래스는 어디서나 같은 static 같다고 느껴졌습니다. 그래서 Java 미션을 하며 string constant pool 이라는 곳에서 String이 관리 된다고 공부한 적이 있어 비슷한 개념인가 궁금합니다.
다른 개념입니다.
테스트를 위한 코드
삼단계 테스트를 통과하기 위해 지금 Service를 보시면 clear()을 만들어 reservations/3을 초기화 하는 방식으로 테스트를 통과하였습니다. claer()를 사용하지 않고 요구사항을 만족 시킬려면 어떤 방식으로 리팩토링 해야 하는지 궁금합니다.
지금 프로덕트 코드에서는 딱히 방법이 떠오르지 않습니다.
구체적인 내용은 해당부분에 코멘트로 달아놨습니다.
추가적인 요청
- Service의 책임을 고려해보셨으면 좋겠습니다.
- 개행, 들여쓰기 같은 기본 코드 포맷을 잘 맞춰주세요.
- 질문은 조금 더 명확히 해주시면 좋겠습니다.
아무래도 스프링 MVC 미션을 할 때 모두들 조금 어려워하는 경향이 있는데,
조금 더 본인이 이 미션을 통해서 어떤것을 학습해야할지 고민해보셨으면 좋겠습니다.
그를 혼자 파보고 궁금증이 생겼을 때 저한테 질문을 주시면 더 좋은 리뷰가 될 수 있을 것 같습니다.
There was a problem hiding this comment.
👀 Comment
Reservation과 ReservationResponse는 어떤 개념적인 차이가있을까요?
필드과 완전히 동일한데, 굳이 따로있어야하는 이유는 어떤것일까요?
There was a problem hiding this comment.
모델과 dto의 차이라고 생각합니다.
Reservation : 모델
ReservationResponse : dto
생각해보면 dto로 넘겨줘야 하는 인자들은 변할 수 있습니다. 그에 맞춰 dto를 만들어서 넘겨주면 되지만 만약 Reservation만 코드 상에 존재하게 되어 dispatcherservlet에게 넘겨 줘야 하는데 그에 맞춰 메서드를 만들거나 하면서 모델의 역할인 내부 비즈니스 로직과 DB 상태를 처리하는 것 보다 더 많은 책임을 가지는 것 같습니다.
There was a problem hiding this comment.
🔥 Request Change
좋습니다.
DTO로 넘겨줘야하는 인자들이 변할 수 있군요.
맞습니다. 이 DTO는 Response이기 때문에 이 응답을 받는 클라이언트에게 적절한 데이터 형식으로 전달해주는 것이 좋겠죠.
리뷰 남겨 주신 자료형인 LocalTime와 LocalDate로 리팩토링 후 실행 시켜 보니 post 가 안되게 되었습니다. 그래서 이유를 조사해보니 LocalTime와 LocalDate는 jacson이 "YYYY-MM-DD"와 "HH:MM"와 같은 문자열이 아니라 다른 방식으로 넘겨온 데이터를 직렬화를 할 수 없어서 라고 해서 다시 String으로 바꿨습니다.
앞선 리뷰에 대해서 반영시도를 해보고 이렇게 답변주셨는데요.
타당한 타협이라고 생각합니다.
Response나 Request는 controller에서 어떤 요청을 받을지 응답을 내릴지 결정하는 객체이니까요.
(Reservation가 PR범위에 빠져있어 여기에 리뷰를 달아보자면)
Reservation의 경우는 조금 다르죠.
이 어플리케이션의 가장 핵심이 되는 class를 뽑으라하면 저는 Reservation이라고 대답할건데요.
핵심 도메인이기 때문입니다.
그 경우에는 많은 비즈니스 로직에 호출돼서 여러 역할을 수행하겠죠.
그래서 다른 Service나 Controller에 의존하지 않고 조금 더 순수하게 존재할필요가 있습니다.
클라이언트에서 날짜와 시간 데이터가 어떤 형식으로 오는지는 Reservation의 관심 밖입니다.
String이 아닌 도메인이 이해할 수 있는 자료형으로 바꿔보시죠!
There was a problem hiding this comment.
🔥 Request Change
String은 너무 자유도가 높은 타입입니다.
누군가는 hh시 mm분, 누군가는 hh:mm이라고 입력할수 있겠죠.
java에는 LocalDate와 같은 날짜나 시간을 나타내는 자료형이 있습니다.
조금 더 명환한 타입을 찾아서 선택해주세요
There was a problem hiding this comment.
LocalTime과 LocalDate로 자료형 바꿨습니다.
There was a problem hiding this comment.
👀 Comment
ID 자동증가생성을 위해 counter를 1씩 올리면서 사용중인 것 같은데요.
지금은 Memory에 데이터를 저장하고 있는데요.
만약에 DB와 같은 저장소에 데이터를 저장하는 방식으로 바꿔야한다는 요구사항이 생기면,
이 부분에 코드 변화가 생기겠네요.
심지어 이를 사용하는 비즈니스 로직에도요.
이는 이 부분이 과연 현진이 PR에 서비스에 핵심 비즈니스 로직을 구현했다고 말씀하셨는데요.
ID를 생성하는 것이 핵심 비즈니스로직이라고 할 수 있을까요?
There was a problem hiding this comment.
리뷰어님의 말씀을 들어보니 단순 id를 생성이 비즈니스 로직보단 DB에서 자동적으로 일어나는 과정이라는 생각이 듭니다. repository로 따로 관리하도록 코드를 리팩토링해보겠습니다.
There was a problem hiding this comment.
👀 Commente
테스트를 위한 코드
삼단계 테스트를 통과하기 위해 지금 Service를 보시면 clear()을 만들어 reservations/3을 초기화 하는 방식으로 테스트를 통과하였습니다. claer()를 사용하지 않고 요구사항을 만족 시킬려면 어떤 방식으로 리팩토링 해야 하는지 궁금합니다.
글쎄요.
저는 service에 서비스 정책과 무관한 로직이 있는 것이 적절치않다고 생각합니다.
지금 현재 프로덕트 코드에서 이를 해결할 수 있는 방법이 명확하게 떠오르지도 않구요.
저는 그래서 이 질문에 대해서는 2가지 의견입니다.
- 테스트가 매회 동일한 결과를 반환하려면 clear는 필요할 수 있다, 하지만 그것이 service에 있어야할 내용인지는 모르겠다.
- 테스트에서 아이디를 정적으로 1인지 2인지 확인하는 방식은 좋지않은 테스트 같다.
There was a problem hiding this comment.
- 테스트가 매회 동일한 결과를 반환하려면 clear는 필요할 수 있다, 하지만 그것이 service에 있어야할 내용인지는 모르겠다.
Service가 가지고 있던 arrayList를 repository로 분리를 하면서 clear()를 repository가 하도록 코드를 리팩토링 하였습니다.
- 테스트에서 아이디를 정적으로 1인지 2인지 확인하는 방식은 좋지않은 테스트 같다.
리뷰어님이 말씀하신 내용이 테스트 코드의 존재 목적이 뭔지 생각을 해보게 하는 것 같습니다.
테스트 코드는 핵심 비즈니스 로직을 확인해본다고 생각합니다. 그에 반한 지금의 테스트는 id를 잘 구성해놓았나를 확인해본다는 관점에서 말씀하신 것 같습니다. 그런 방면에서 본다면 좋지 않은 테스트 코드 인 것 같습니다. (다만 위 테스트코드가 제가 작성한 것이 아닌 초록스터디에서 주어진 것 수정해도 되는 지 잘 모르겠습니다.)
말씀해주신 리뷰를 읽다 보니 Spring mvc에 대한 이해가 낮은 거 같아 조금 더 공부해보게 되었습니다. 서비스의 책임이란 비즈니스 '규칙'을 결정하고 '처리'한다 라고 생각합니다. 서비스는 컨트롤러와 레포지토리 사이에서 요청이 들어오면 어떤 데이터를 가지고 어떻게 사용자가 원하는 결과에 도달할 수 있는지를 결정하는 계층이라고 생각합니다. 그래서 요번 리뷰 반영 전 코드에서 위 책임을 생각해보면 서비스가 배열을 가지고 있는 것은 사실 아주 큰 위반사항입니다. 실제론 db가 되겠지만 지금은 배열로 생성해서 사용하는 코드를 서비스가 가지지 않고 레포지토리가 가지도록 리팩토링 해보았습니다. ( 여기서 사실 db가 데이터를 가지고 있고 서비스에서 어떤 데이터에 대한 요청을 서비스가 하면 이에 맞는 데이터를 레포지토리가 db에서 찾아서 넘겨준다는 것은 알지만 지금 풍부한 도메인 모델 vs 빈약한 도메인 모델풍부한 도메인 모델이 좋은 패턴이고 빈약한 도메인 패턴이 안티 패턴이라고 설명을 하고 있었습니다. 여기서 두 패턴을 나누는 기준은 비즈니스 로직의 위치였습니다. 풍부한 도메인 모델은 비즈니스 로직이 도메인 모델에 존재하고 빈약한 도메인 모델은 서비스에 비즈니스 로직이 존재하였습니다. 그래서 서비스는 모델에 있는 비즈니스 로직을 사용하면 되고 oop관점에서도 좋고 캡슐화도 잘된다 라고 설명했습니다. 그럼 서비스 계층이 비즈니스 로직을 가지면 안된다는 건가?저는 여기서 혼란에 빠지기 시작했습니다. 어떤 자료에선 비즈니스 로직이 도메인 모델에 있어야 한다고 하고, 다른 곳에서는 서비스 계층의 목적은 컨트롤러에서 요청을 받아서 비즈니스 로직을 수행하는 것 이라고 하니 혼돈이 있었습니다. 그래서 왜 서비스 계층에 비즈니스 로직을 설계 해야 하나를 고민해보니 저번 스터디에서 배운 @transactional 같은 기능이 서비스에 존재하지 않는다면 동시에 데이터를 바꾸게 되어 원하는 비즈니스 로직을 수행하지 못할 수 도 있겠구나 생각했습니다. 이에 대해 리뷰어님은 왜 도메인 모델이 아닌 서비스 계층에서 비즈니스 로직을 처리해야하는지 묻고 싶습니다.
처음에 질문했던 저의 의도는 다음과 같습니다. 자바 미션에서
다음엔 한번 더 확인하고 푸쉬 하겠습니다.
제 질문이 다소 추상적이게 느껴졌을 수 도 있다고 생각했습니다. 최대한 풀어서 작성해보겠습니다. LocalDate,LocalTime이 왜 적용이 되지 않을까?리뷰 남겨 주신 자료형인 |
dooboocookie
left a comment
dooboocookie
left a comment
There was a problem hiding this comment.
안녕하세요, 현진.
리뷰어 루카입니다.
리뷰가 생각보다 많이 늦었네요. 죄송합니다.
🎯 리뷰 포인트
1️⃣ CleanCode
아마 그리디 스터디에서도 클린코드에 중요성을 많이 다뤘을 것이라 생각하는데요.
지난번 개행이나 들여쓰기를 잘 지켜달라한 리뷰도 클린코드 관점에서 드린 리뷰입니다.
클린코드는 변수명이나 메서드명을 잘 주고 그 로직을 잘 분리해서 응집도를 높이는 코드 품질에 밀접한 내용도 있지만,
코드를 읽는 자의 이해를 돕는 것도 큰 목적이라 생각합니다.
그래서 이번에는 사소할 수 있지만, 주석 부분에 코멘트를 달아봤습니다.
2️⃣ Service, Domain
현진도 앞서 서비스의 책임은 비즈니스 로직이다라고 말씀해주셨는데요.
이어
저는 여기서 혼란에 빠지기 시작했습니다. 어떤 자료에선 비즈니스 로직이 도메인 모델에 있어야 한다고 하고, 다른 곳에서는 서비스 계층의 목적은 컨트롤러에서 요청을 받아서 비즈니스 로직을 수행하는 것 이라고 하니 혼돈이 있었습니다. 그래서 왜 서비스 계층에 비즈니스 로직을 설계 해야 하나를 고민해보니 저번 스터디에서 배운 @transactional 같은 기능이 서비스에 존재하지 않는다면 동시에 데이터를 바꾸게 되어 원하는 비즈니스 로직을 수행하지 못할 수 도 있겠구나 생각했습니다. 이에 대해 리뷰어님은 왜 도메인 모델이 아닌 서비스 계층에서 비즈니스 로직을 처리해야하는지 묻고 싶습니다.
라는 고민과 질문도 남겨주셨습니다.
정말 좋은 고민이라고 생각해요.
결론부터 이야기를 하면 보통 Service에는 비즈니스로직, 즉 더 정책적인 내용이 들어간다고 설명할 수 있을 것 같아요.
지금 구조에서도 Reservation과 ReservationService는 확실히 다른 책임을 가지고 있어야합니다.
예를들어 새로운 예약을 만든다면,
- User가 실제하는지 조회하고,
- 새 Reservation을 만들 수 있는지 검증하고,
- User에게 포인트를 적립해주고,
- ...
비즈니스 관점으로 보면 정책이고
프로그래밍적으로 보면 (보통 하나의 트랜잭션에서 일어나야할) 일련의 로직입니다.
이렇게 여러 도메인들을 협업시켜 비즈니스 로직을 완성 시키는 것은 필연적으로 ReservationService에 존재하겠죠.
반면 Reservation은 조금 더 Reservation이라는 도메인 자체의 책임을 갖게 됩니다.
예를들면, 시간은 LocalDateTime 타입이어야한다던지, 시간은 빈값일 수 없다던지, 이름은 몇글자를 넘을 수 없다던지, 이런 Reservation자체의 규칙을 갖게되는 것이지요.
비즈니스로직vs서비스로직
유튜브 영상이 있는데, 이전 그리디 기수분들은 이걸 보고 좀 이해를 하시는것 같더라구요.
참고해보시면 좋겠고, 충분히 헷갈리고 고민해봐야할 개념이니 고민해보시고 더 궁금한게 있으면 말씀해주세요.
😅 다시한번 Request Change
아무래도 지난번에 조금 리뷰가 딱딱하게 느껴졌을 수 있다는 것을 이번에 리뷰를 다시 작성하면서 했습니다.
혹시나 리뷰에 답답함을 느꼈거나 상처를 받으셨다면 죄송합니다.
스프링같은 처음 보는 개념을 학습할 때 모두들 답답함을 느끼는데요.
리뷰는 받아라하는데, 뭘 질문해야될지도 모르겠는 상황이니까요.
망망대해를 혼자 표류하고 있는 느낌이실텐데요.
리뷰라는 과정이 이렇게 명쾌하지 않은 상황의 연속임을 이해해주시면 감사하겠습니다.
부디 저와 리뷰를 나누는 과정이 도움이되었으면 좋겠네요.
그래도그래도..
사용하지 않는 코드도 있고, 지금 Service와 Repository 분리 시도하신 걸 조금 잘 마무리 지으시는 게 좋을 것 같아서 다시한번 Request Chnage 드리겠습니다.
| @@ -0,0 +1,49 @@ | |||
| package roomescape.repository; // (새 패키지) | |||
There was a problem hiding this comment.
🔥 Request Change
이 코멘트의 목적은 불필요한 주석은 지워주세요 입니다.
아무래도 학습목적으로 진행되는 미션이고,
맥락이 많이 다른 리뷰어와 소통을 진행하다 보니,
설명적인 PR을 만들고 싶어서 여러 주석을 달고 싶다는 마음을 이해합니다.
그런 불안감과는 별개로 코드리뷰 과정을 앞으로 진행할 개발에 대한 예행 연습이라고 생각해주시면 좋겠습니다.
그래서 이 코드 라인을 빌려 클린코드에 대해서 조금 이야기를 나눠보면 어떨까합니다.
리뷰어를 읽는 사람의 입장에선 현진님이 올려준 질문, 코드, 코드의 주석, 요구사항정리문서, commit내역, 테스트 내역, 실제 동작여부 등 고려할게 정말 많습니다.
그렇다 보니, 협업과 개발을 잘하는 현진이 올린 PR은 좋은 코드가 되기 위해서는 코드 자체가 자기설명적이 되어야합니다.
현실 세계에선 여러 변경이력이나 도저히 정리가 안되는 복잡한 요구사항에 의해 주석을 많이 쓰긴 하지만,
미션처럼 극한으로 통제되어있는 요구사항에선 repository패키지를 보고 "아! 레파지토리 레이어를 분리하기 위해 새로운 패키지를 만들었구나!"라고 충분히 이해할테 주석은 필요없는 부분이라고 생각합니다.
사실 이 주석 하나가 얼마나 리뷰어를 피로하게 하겠냐라고 생각하실 수 있고 사실 저도 그렇게 생각해요 별로 안피로합니다.
하지만, 아마 n개월 뒤 현진도 이 코드의 수정이 필요해서 이 라인을 보면 '음? 왜 주석을달았지?'라고 생각할 수 있습니다.
이런 코드를 이해하고 유지보수해나가는 과정에서 불필요한 것을 덜어내고 응집도 있는 코드를 작성하는것은 매우 중요하다는 것을 타인(미래의 나를 포함한)을 위해 이해해주시면 좋겠습니다.
There was a problem hiding this comment.
넵 좋은 프로그래머는 코드로 말한다고 하는게 떠오르는 리뷰인 것 같습니다. 주석을 달지 않아도 코드로서 저의 생각을 말할 수 있도록 노력해보겠습니다.
| import java.util.concurrent.atomic.AtomicLong; | ||
|
|
||
| @Repository | ||
| public class ReservationRepository { |
| @@ -1,3 +1,3 @@ | |||
| package roomescape.dto; | |||
|
|
|||
| public record ReservationDelete ( | |||
There was a problem hiding this comment.
🔥 Request Change
이 코드또한 사용되고 있지 않습니다.
There was a problem hiding this comment.
🔥 Request Change
좋습니다.
DTO로 넘겨줘야하는 인자들이 변할 수 있군요.
맞습니다. 이 DTO는 Response이기 때문에 이 응답을 받는 클라이언트에게 적절한 데이터 형식으로 전달해주는 것이 좋겠죠.
리뷰 남겨 주신 자료형인 LocalTime와 LocalDate로 리팩토링 후 실행 시켜 보니 post 가 안되게 되었습니다. 그래서 이유를 조사해보니 LocalTime와 LocalDate는 jacson이 "YYYY-MM-DD"와 "HH:MM"와 같은 문자열이 아니라 다른 방식으로 넘겨온 데이터를 직렬화를 할 수 없어서 라고 해서 다시 String으로 바꿨습니다.
앞선 리뷰에 대해서 반영시도를 해보고 이렇게 답변주셨는데요.
타당한 타협이라고 생각합니다.
Response나 Request는 controller에서 어떤 요청을 받을지 응답을 내릴지 결정하는 객체이니까요.
(Reservation가 PR범위에 빠져있어 여기에 리뷰를 달아보자면)
Reservation의 경우는 조금 다르죠.
이 어플리케이션의 가장 핵심이 되는 class를 뽑으라하면 저는 Reservation이라고 대답할건데요.
핵심 도메인이기 때문입니다.
그 경우에는 많은 비즈니스 로직에 호출돼서 여러 역할을 수행하겠죠.
그래서 다른 Service나 Controller에 의존하지 않고 조금 더 순수하게 존재할필요가 있습니다.
클라이언트에서 날짜와 시간 데이터가 어떤 형식으로 오는지는 Reservation의 관심 밖입니다.
String이 아닌 도메인이 이해할 수 있는 자료형으로 바꿔보시죠!
위 리뷰를 받고 도메인을
아주 좋은 감명 답변인 것 같습니다. 읽어보며 도메인과 서비스의 로직 차이에 대해 확실히 이해가 되는 것 같습니다.
아유 아닙니다. 요번 리뷰에 처음부터 제가 따끔하게 부탁드린다고 했었고 리뷰어님의 핵심적인 질문들 덕분에 좋은 공부가 된 것 같습니다. |
2 participants
안녕하세요! 백경환 리뷰어님
스프링은 처음이라 많이 서툴었습니다. 따끔하게 리뷰해주시면 열심히 배워보겠습니다!
일단 전반적인 코드 흐름을 설명해보겠습니다.
컨트롤러 (Controller)
AdminController:@Controller를 사용하여 웹 페이지를 반환합니다. 사용자가 특정 경로로 접속했을 때, 해당하는 HTML 파일을 렌더링하여 보여줍니다./:home.html(홈 페이지)/reservation:reservation.html(예약 관리 페이지)ReservationController:@RestController를 사용합니다. HTTP 요청을 받아ReservationService를 호출하고, 그 결과를 JSON 형태로 반환합니다.GET /reservations: 모든 예약 목록을 조회합니다.POST /reservations: 새로운 예약을 생성합니다.DELETE /reservations/{id}: 특정 예약을 삭제합니다.서비스 (Service)
ReservationService: 핵심 비즈니스 로직을 담당합니다.List<Reservation>형태로 메모리에 저장하고 관리합니다.AtomicLong을 사용하여 예약 ID를 순차적으로 생성합니다.getAllReservations(): 전체 예약 목록을 반환합니다.addReservation(): 새로운 예약을 리스트에 추가하고, 생성된 예약 객체를 반환합니다.deleteReservation(): ID를 기준으로 예약을 찾아 리스트에서 삭제합니다. 존재하지 않는 ID일 경우IllegalArgumentException을 발생시킵니다.궁금한점
@service로 만든 서비스 클래스는 어디서나 같은 static 같다고 느껴졌습니다. 그래서 Java 미션을 하며 string constant pool 이라는 곳에서 String이 관리 된다고 공부한 적이 있어 비슷한 개념인가 궁금합니다.삼단계테스트를 통과하기 위해 지금Service를 보시면clear()을 만들어 reservations/3을 초기화 하는 방식으로 테스트를 통과하였습니다.claer()를 사용하지 않고 요구사항을 만족 시킬려면 어떤 방식으로 리팩토링 해야 하는지 궁금합니다.