Conversation
|
@yeonjookang 안녕하세요 연주님! 열심히 참여 해주셔서 감사합니다! 1주차 리뷰 진행하도록 하겠습니다 :) |
| public record ScreeningsDetail ( | ||
| String theater, | ||
| List<String>screeningTime | ||
| ) { |
There was a problem hiding this comment.
데이터 전달 객체로 record 를 사용해주셨네요 👍🏿
코드 전반적으로 살펴봤을때 깔끔하게 잘 작성해주셨는데, List<String>screeningTime 띄어쓰기, 괄호, 중괄호 등이 일관되지 않은 부분들이 보이는 것 같아요 :) 이런 부분들까지 신경 써주시면 더 좋을 것 같습니다 !
|
|
||
| @EntityListeners(AuditingEntityListener.class) | ||
| @MappedSuperclass | ||
| public class BaseEntity { |
There was a problem hiding this comment.
System Properties 를 정의하는 BaseEntity 는 추상 클래스로 제공해도 좋을 것 같습니다 :)
| package com.example.entity.movie; | ||
|
|
||
| public enum Genre { | ||
| 액션, 로맨스, 호러, SF |
There was a problem hiding this comment.
요구 사항을 정말 충실하게 잘 따라주셨네요 !! 한글 보다 영어로 통일해주셔도 좋을 것 같습니다.
1주차 시나리오에 그려진 그림은 클라이언트 사이드에서의 화면이기 때문에, 서버사이드에서는 Enum 을 영어로 관리해도 문제 없을 것 같아요 !
|
|
||
| private Long theaterId; | ||
|
|
||
| private LocalDateTime startTime; |
There was a problem hiding this comment.
저는 개인적으로 LocalDate 타입의 경우에는 xxxDate 와 같은 네이밍을 적용해도 된다고 생각하는데,
LocalDateTime 의 경우에는 네이밍을 xxxDate or xxxTime 으로 하기에는 타입과 네이밍의 불일치가 존재한다고 생각해서, at 이라는 네이밍을 주로 사용하곤 합니다 !
There was a problem hiding this comment.
요구 사항에 맞게 Screening 테이블을 너무 잘 설계 해주셨습니다 👍🏿
There was a problem hiding this comment.
와! 네이밍에 대한 좋은 방법 하나 배워갑니다 :)
| private Long seatCol; | ||
|
|
||
| private Boolean isReservation; | ||
|
|
There was a problem hiding this comment.
- isReservation 이 현재 요구사항을 기준으로 꼭 필요한 지 고민해주시면 좋을 것 같습니다.
- seatRow, seatCol 을 추가해주셨는데, cgv 등의 영화관에서 좌석은 각자 A1, A2 와 같이 좌석 이름이 존재하는데요, 여기서 알파벳이 row 에 해당되고, 숫자가 col 에 해당됩니다 ! 따라서 두 컬럼을 합쳐서, 하나의 좌석 이름으로 제공하는 것은 어떨까요 ?
There was a problem hiding this comment.
현재 요구사항은 모든 상영관이 A1~E5로 통일되어있지만, 실제로는 상영관별로 좌석 행과 열 정보가 다른 것을 고려하여 예매 여부 속성을 넣었습니다. 만약 이 정보가 없다면 프론트엔드가 좌석 정보를 출력할 때 예매된 좌석 정보만 알게 됨으로써 모든 좌석을 출력할 수 없다고 생각했습니다!
하지만 현재 요구사항에서는 A1~E5로 통일되어있어 딱히 필요없는 속성인 것 같습니다. 제가 생각한 변경 가능성에 대해서 멘토님은 어떻게 생각하시나요?
There was a problem hiding this comment.
행과 열의 정보는 하나로 합치는 게 더 나아보이네요 👍 수정하겠습니다!
There was a problem hiding this comment.
실제로는 상영관별로 좌석 행과 열 정보가 다른 것을 고려하여 예매 여부 속성을 넣었습니다. 만약 이 정보가 없다면 프론트엔드가 좌석 정보를 출력할 때 예매된 좌석 정보만 알게 됨으로써 모든 좌석을 출력할 수 없다고 생각했습니다!
@yeonjookang 변경 가능성을 생각하고 코딩을하는 것은 좋은 태도라고 생각됩니다. 다만, 현재 설계된 Seat 는 수정이 필요하긴 합니다.
- 예매 여부가 Seat 테이블에 들어가는 것은 좋지 않다고 생각됩니다. 가장 큰 이유는 예매라는 행위가 공유 자원 인 seat 에 결합된다는 것인데, 이 경우 관리가 더 힘들어집니다. 상품-주문-결제 도메인에서 저희가 주문을하면 상품테이블에 주문과 관련된 컬럼등을 반영을 할지 생각하면 이해가 잘 되실 것 같아요.
프론트엔드가 좌석 정보를 출력할 때 예매된 좌석 정보만 알게 됨
이 경우는 예매와 관련된 테이블을 통해서 특정 좌석이 예매되었는지 알 수 있을 것 같아요.
There was a problem hiding this comment.
행위와 공유 자원이 결합되면 관리가 어려워지는군요! 상품-주문-결제 도메인을 예시로 들어주셔서 이해가 더 잘됐습니다 ! 수정하겠습니다!
module-infra/build.gradle
Outdated
| jar.enabled = true | ||
|
|
||
| dependencies { | ||
| api("org.springframework.boot:spring-boot-starter-data-jpa") |
There was a problem hiding this comment.
JPA 의 의존성 전파를 위해서 api 를 사용해주셨는데, 이 경우 infra 모듈을 의존하는 다른 상위 모듈들에서는 jpa 의존성을 다시 정의할 필요가 없을 것 같습니다 !
There was a problem hiding this comment.
api는 추이 의존성을 허용하는군요! 수정하겠습니다 :)
| "JOIN Screening s ON m.id = s.movieId " + | ||
| "JOIN Theater t ON s.theaterId = t.id " + | ||
| "WHERE s.startTime >= CURRENT_DATE " ) | ||
| List<MoviesNowShowingDbDto> findNowShowing(LocalDate today); |
There was a problem hiding this comment.
today 매개변수가 불필요한 것 같습니다 :)
There was a problem hiding this comment.
테스트 용이성을 위해 CURREND_DATE 대신 today 를 사용하겠습니다!
|
|
||
| @Getter | ||
| @AllArgsConstructor | ||
| public class MoviesNowShowingDbDto { |
There was a problem hiding this comment.
클래스 이름에 Db 와 같은 특정 기술에 종속적인 용어가 포함되면, 해당 클래스가 특정 기술에 강하게 결합된 것처럼 보이며, 다른 기술로의 전환이 필요할 때 유연성이 떨어진다는 단점이 있습니다. 기술에 종속적이지 않은 이름을 사용하면 다른 기술로 전환 할 때 유연성을 유지할 수 있습니다 !
There was a problem hiding this comment.
필요한 데이터만 조회해서 사용하는 방식은 사실 2주차 구현 요구사항 중 하나였는데 ㅎㅎ
미리 고민하셔서 적용한 부분이 좋은 것 같습니다 👍🏿
There was a problem hiding this comment.
MoviesNowShowingDetailDto로 변경하도록 하겠습니다!
module-presentation/build.gradle
Outdated
| @@ -0,0 +1,12 @@ | |||
| bootJar.enabled = false | |||
There was a problem hiding this comment.
애플리케이션의 진입점이 되는 presentation 모듈에서는 bootJar 가 true 로 되어야할 것 같아요 :)
There was a problem hiding this comment.
이해했습니다! Application이 실행되는 presentation 모듈은, bootJar를 true로, 그 외는 bootJar를 false로 설정하겠습니다.
| String theaterName = theaterEntry.getKey(); | ||
| List<String> screeningTimes = theaterEntry.getValue().stream() | ||
| .sorted(Comparator.comparing(MoviesNowShowingDbDto::getStartTime)) | ||
| .map(dto -> dto.getStartTime() + " ~ " + dto.getEndTime()) |
There was a problem hiding this comment.
map(dto -> dto.getStartTime() + " ~ " + dto.getEndTime()) 해당 코드에서
요구사항을 진짜 너무 충실하게 잘 반영하려는 노력이 많이 보여서 좋습니다 !!
다만, 화면에 표기되어야 하는 데이터 형식을 서버에서 꼭 만들어주기보다, 필요한 데이터만 응답을 하고 화면을 구성하는 것은 클라이언트의 역할로 위임하는 것도 방법입니다 !
위 처럼 서버에서 클라이언트 요구사항 을 직접 처리하게 되면, 화면과 관련된 비지니스 요구사항이 변경될 때, 서버도 영향이 있을 것 같습니다.
settings.gradle
Outdated
| include 'module-application' | ||
| include 'module-domain' | ||
| include 'module-infra' | ||
| include 'module-core' |
There was a problem hiding this comment.
module prefix 는 꼭 필요한 것 같지 않습니다 !
물론 팀, 회사 컨벤션에 따라서 달라질 수 있는 부분이긴하나 이름을 생략해도 의미 전달이 충분히 된다면, 제거해도 괜찮다고 생각합니다 :)
| private final MovieService movieService; | ||
|
|
||
| @GetMapping("/list/now-showing") | ||
| public BaseResponse<List<MoviesNowShowingDetail>> getCategories() { |
There was a problem hiding this comment.
API 이름에 list 를 명시하는 것은 중복일 것 같아요. 일반적으로 RESTful API 설계에서는 리소스 이름만으로 충분히 목록 반환이라는 의미를 전달할 수 있습니다. (e.g /movies)
추가적으로 실무에서는 API Enpoint 에 버전을 명시하기도 합니다 :) 이런 부분들까지 반영되면 좋을 것 같아요.
There was a problem hiding this comment.
API Endpoint 와 Handler Method 네이밍에 대한 불일치가 있는 것 같습니다.
| compileOnly 'org.projectlombok:lombok' | ||
| annotationProcessor 'org.projectlombok:lombok' | ||
| testImplementation 'org.springframework.boot:spring-boot-starter-test' | ||
| testImplementation 'org.junit.jupiter:junit-jupiter-api:5.8.1' |
There was a problem hiding this comment.
5.8.1 와 같이 버전을 의존성 마다 직접 명시하는 방법대신, 의존성 버전들을 한곳에서 관리하는 방법도 있습니다 !
There was a problem hiding this comment.
dependencyManagement 말씀하시는 걸까요?
There was a problem hiding this comment.
@yeonjookang 네 dependencyManagement 도 있지만, 명시적인 의존성이 필요한 경우도 있는데요,
이 경우 gradle.properties 에 아래와 같이 정의해서 가져다 사용할 수 있습니다.
springBootVersion=3.4.0
README.md
Outdated
| - module-application: 사용자가 요청한 기능을 처리한다. | ||
| - module-domain: 시스템이 제공할 도메인 규칙을 구현한다. | ||
| - module-infra: 데이터베이스 같은 외부 시스템과의 연동을 처리한다. | ||
| - module-core: Convertor, Error-Response 와 같이 특정 layer에 종속되지 않는 기능을 처리한다. |
There was a problem hiding this comment.
Layer 를 기준으로 모듈을 잘 구성해주셨네요 :) 많은 고민의 흔적이 보이는 것 같아서 좋습니다 ㅎㅎ
추가로 몇가지 리뷰를 드리면,
Layered Architecture 를 사용한다고 해서 모듈도 완전 동일한 형태의 Layer 로 나눌 필요는 없습니다 :)
presentation + application 을 하나의 모듈로 구성할 수도 있을 것 같아요.
그리고 추후에 분리가 필요하면 그때 분리해도 될 것 같습니다.
단, 지금은 현재 모듈 형태를 그대로 유지하고 2주차로 넘어가주세요 ! Layer 를 많이 쪼갠 형태의 모듈 구조를 사용하는 경우 프로젝트가 점진적으로 발전하면서 어떤 영향이 있을지, 직접 느껴보는 것도 많은 인사이트를 느낄 수 있을 것 같습니다. 진행 하시면서 presentation or application 이 통합되어도 괜찮겠다고 판단되는 지점이 올 수 있고, 혹은 분리해도 충분히 괜찮다고 느끼실 수도 있을 것 같아요.
core 모듈은 common 역할을 하는 모듈로 보여지는데, 경우에 따라서 해당 모듈을 anti-pattern 으로 보기도 합니다. (팀 by 팀)
공통 기능을 위한 하나의 모듈을 만드는 경우, 개발자들이 클래스들을 설계 하면서 (실제로 공통 역할이 아니지만) core(or common) 모듈에 패키지를 생성해서 정의하는 경우도 있습니다 !
전체적으로 모듈을 잘 구성해주셨습니다 !
There was a problem hiding this comment.
모듈을 구성하면서 너무 많은 모듈로 쪼갠 건 아닐까 라는 고민이 많이 되었습니다! 멘토님 말씀대로 직접 경험하면서 느껴봐야겠습니다! core 모듈은 제 프로젝트에서는 없애는 게 맞는 것 같아요 수정하겠습니다!
There was a problem hiding this comment.
| AS-IS
infra -> domain <- application <- presentation
infra <- application <- presentation
| TO-BE : infra -> domain <- application <- presentation
에서, MoviesNowShowingDbDto를 domain 모듈로 위치를 변경해도 application이 여전히 JpaRepository를 참조하고 있기 때문에 TO-BE 관계로 변경은 되지 않습니다.
그럼에도, MoviesNowShowingDbDto를 domain 모듈로 위치하는 것이 좋을까요?
There was a problem hiding this comment.
TO-BE 로 만들려면 MovieRepository 또한 domain 으로 이동시켜야 합니다.
그러면 현재 설계 기준으로 infra 가 모듈이 없어지겠죠?, 즉, 의존성 방향을 TO-BE 처럼� 고수준(domain) 으로 흐르게끔 만드는 것이 clean architecture 입니다.
앞으로 남은 주차를 진행하면서 Presentation > Application > Persistence > Database 와 같이 단방향으로 흐르는 Layered Architecture 를 끝까지 가져가면서 개선해볼지, 혹은 진행하면서 다른 아키텍처로 변경할 것인지 자신만의 trade-off 기준을 세워서 선택해주시면 됩니다 :)
| private Long theaterId; | ||
|
|
||
| @Nullable | ||
| private Long userId; |
There was a problem hiding this comment.
좌석 테이블이 유저 정보를 포함하고 있는 이유가 궁금합니다.
아마도 '예약' 과 관련된 비지니스 요구 사항을 미리 가늠하여서 설계를 해주신 것 같습니다 :)
좌석 테이블이 유저 정보를 갖게 된다면, 해당 테이블은 좌석보다는 '예약' 에 가까운 테이블일 것 같아요 !
실무에서는 부족한 비지니스 요구사항을 기반으로 프로젝트를 진행하는 경우, 확장, 추가될 기능에 대해서 미리 가늠하고 설계하는 것은 좋지만 ! (이 부분은 정말 칭찬드립니다 💯 )
현재 설계된 seat 테이블은 수정이 필요한 설계로 보여집니다.
There was a problem hiding this comment.
예매 정보와 좌석 정보를 함께 조회하는 API가 많이 요청될 것이라고 생각했기 때문입니다. 예매 정보를 조회할 때도 좌석 정보를, 좌석 정보를 조회할 때도 예매 정보를 같이 조회할 것이라고 예상됩니다. 이런 상황에서 좌석 테이블과 예약 테이블을 분리한다면 예매 정보를 조회할 때 Join이 필수적으로 필요해짐에 따라 성능이 낮아질 것이라 생각했습니다. 따라서 좌석과 예약은 1:1 관계를 테이블 역정규화로 하나의 테이블로 합쳤습니다.
제가 한 테이블 역정규화에 대해 어떻게 생각하시는지 궁금합니다!
There was a problem hiding this comment.
예매 정보를 조회할 때도 좌석 정보를, 좌석 정보를 조회할 때도 예매 정보를 같이 조회할 것이라고 예상됩니다. 이런 상황에서 좌석 테이블과 예약 테이블을 분리한다면 예매 정보를 조회할 때 Join이 필수적으로 필요해짐에 따라 성능이 낮아질 것이라 생각했습니다. 따라서 좌석과 예약은 1:1 관계를 테이블 역정규화로 하나의 테이블로 합쳤습니다.
성능을 사전에 고려하여 역정규화를 하는 것은 잘못되었습니다. 성능 최적화는 성능 문제가 발생했을때 해도 충분합니다.
역정규화로 인해 아래와 같은 단점이 발생할텐데, 적절한 trade-off 가 맞을지 고민해주시면 좋을 것 같아요.
- 데이터의 중복으로 인한 저장공간 증가
- 데이터의 무결성 문제
- 상영관1 의 좌석 정보가 X1~Z5 까지로 변경되는 경우, 상영관 1에 해당되는 (이미 예약(주문)된) 좌석들의 정보도 변경해야 하므로 쓰기 복잡성 증가. 따라서 데이터베이스에 대한 유지보수성이 감소
결과적으론 조회속도 vs 데이터 중복 제거 및 무결성 유지 중에서 더 중요하다고 생각되는 걸 기준으로 선택해야 합니다.
There was a problem hiding this comment.
조회 속도를 측정해보고, 성능 최적화를 통해 향상된 조회 속도가 데이터 중복 제거 및 무결성 유지보다 더 중요하다고 생각되면, 테이블 역정규화를 진행하도록 해야겠네요! 감사합니다 :)
| public class DtoConvertor { | ||
| public List<MoviesNowShowingDetail> moviesNowScreening(List<MoviesNowShowingDbDto> dbResults){ | ||
| return dbResults.stream() | ||
| .collect(Collectors.groupingBy(MoviesNowShowingDbDto::getMovieName)) |
There was a problem hiding this comment.
같은 이름의 영화가 존재하더라도 Id 가 다르면 다른 영화로 취급되니, 영화 이름보다는 Id 가 적합할 것 같아요 :)
There was a problem hiding this comment.
이해했습니다! 추가적으로 응답에 영화 id를 추가하고, id를 기준으로 그룹핑하도록 수정하겠습니다.
좋았던 점
아쉬웠던 점
리뷰 포인트 및 추가 질문에 대한 답변
이건 설계를 어떻게 했냐에 따라 다를 것 같아요. 현재 구조에서는 MoviesNowShowingDbDto 가 infra 모듈에 속하고 있으며, infra 가 domain 모듈을 의존하고 있으니,
현재 구조는 application 에서 infra 모듈에 존재하는 MoviesNowShowingDbDto 코드를 참조하기 때문에, 만약, MoviesNowShowingDbDto 를 infra 에 정의하고 싶은 경우에는 모듈, Layer 간 의존성 방향을 To-be 처럼 명확하게 하기 위해 Domain 모듈에 중간 객체를 하나 두는 방법이 있습니다.
이 경우 단점은, Layer 간 객체 변환이 많이 일어난다는 점이 있습니다. 여기서 Trade-off 를 해야 합니다. 객체 변환이 많이 일어나는게 싫은 경우에는 위에서 언급한 것 처럼 Projection 용 Dto 를 domain 모듈에 정의한다. 아니면 domain 모듈에 별도의 클래스를 하나 더 만들어서 Layer 간 의존 방향을 명확히 한다. 현재 별도 Domain Model 을 두고 있지 않고, Entity 를 Domain 모듈에 정의하고 Domain Model 로 사용중인 것을 보아,
이건 팀 by 팀일 것 같아요. 즉, 컨벤션 차이일 것 같습니다. 팀원들이 JPA 를 충분히 잘 쓴다면, 직접 참조를 사용한 객체 그래프 탐색 방식으로 설계해도 무방합니다. 만약 JPA 를 사용하면서 간접 참조를 사용하고, querydsl 과 같은 동적 쿼리 작성 도구를 사용하여 N+1 문제를 해결하는 경우 JPA 대신 다른 기술을 고려하는 것도 방법입니다 :)
이것은 상황에 따라 다를 것 같습니다 :) 오버엔지니어링 을 고려하시는 마인드는 정말 좋은 것 같습니다. 저는 비지니스 논리가 복잡하고, 사전에 어떤 모델들이 필요한지 알고 있으며, 해당 모델들이 다뤄야하는 메서드들이 충분히 복잡하다면 Entity 와 별개의 Domain Model 을 둘 것 같습니다. 그게 아니라면 말씀해주신 것 처럼 프로젝트 초기부터 JPA 기술을 다른 기술로 변경하는 걸 예측하여 POJO 객체를 생성하는 것은 과도한 설계 로 생각됩니다.
(없어도 됩니다.) 개인 취향이라고 봐주시면 될 것 같아요 ㅎㅎ 저는 코드 내에서 명시적으로 테이블의 구조를 보고 싶은 경우에 쓰곤 합니다. JPA 만으로 테이블 구조를 명확하게 파악하기 위해서는 테이블 명세에 정확하게 대응되는 구조를 클래스에 반영해야 하는데, 아무래도 JPA 를 보는 것보단 ddl 을 보는게 더 직관적이고 빨라서요 ! 그리고 JPA 를 사용해서 자동생성이 되려면 create 와 같은 설정을 해줘야하는데, 모든 환경(운영, 개발 등)을 통틀어서, 저는 none or validate 를 사용하는 것을 선호합니다 ! |
| public class MovieService { | ||
|
|
||
| private final MovieRepository movieRepository; | ||
| private DtoConvertor dtoConvertor = new DtoConvertor(); |
There was a problem hiding this comment.
Converter 도 빈으로 만들어서 제공하면 좋을 것 같아요. 테스트에서는 외부 의존성이나 복잡한 로직을 포함한 객체를 실제로 사용하는 대신, Mock 등을 사용하여 테스트를 독립적으로 진행할 수 있는데 new 키워드로 객체를 생성하는 방식은 직접 객체를 생성하기 때문에 테스트에서 의존성을 주입하기 힘들 것 같습니다.
There was a problem hiding this comment.
new 키워드를 지양해야하는 이유를 새롭게 알아갑니다! 감사합니다 :)
| @ResponseStatus(HttpStatus.BAD_REQUEST) | ||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| public BaseErrorResponse handle_IllegalArgumentException(IllegalArgumentException e) { | ||
| log.error("[BaseExceptionControllerAdvice: handle_IllegalArgumentException 호출]", e); |
There was a problem hiding this comment.
모든 코드에 log.error 를 사용중인데 실무에서는 error 로그의 비율에 따라서 알람(Alert)을 구성하기도 합니다.
즉, 클라이언트 사이드의 오류인데도 불구하고 log.error 를 찍는다면, 심각한 에러가 아님에도 불구하고 많은 알람을 받을 것 같아요 :)
There was a problem hiding this comment.
클라이언트 사이드의 오류인 경우, error 가 아닌 info 정도가 적합해 보이네요 :) 수정하겠습니다!
|
|
||
| @Bean | ||
| public AuditorAware<String> auditorProvider(){ | ||
| return () -> Optional.of(UUID.randomUUID().toString()); |
There was a problem hiding this comment.
UUID 가 갖는 특별한 의미가 있을까요? 보통 System Properties 의 작성자, 수정자 들은 SYSTEM 과 같은 이름을 고정적으로 사용하기도 합니다.
|
@BAEKJungHo 꼼꼼히 리뷰해주셔서 정말 감사드립니다 :) 수고하셨습니다 👍 |
|
@yeonjookang 수고하셨습니다. 남은 주차도 화이팅입니다 :) |
제목(title)
작업 내용
발생했던 문제와 해결 과정을 남겨 주세요.
이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.
리뷰 포인트
기타 질문