-
Notifications
You must be signed in to change notification settings - Fork 39
[1주차] 영화 서비스 초기 구성 및 상영 중인 영화 조회 API 개발 #2
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
base: main
Are you sure you want to change the base?
Conversation
|
안녕하세요 찬엽님! 1주차 리뷰어를 맡게된 권순한입니다! |
core/build.gradle
Outdated
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.
현재 core 모듈과 다른 모듈에 공통적인 dependency (e.g jpa)가 있습니다.
- 아키텍처 의도로 보면 공통적인 부분은 코어모듈에 의존성을 가지도록 설계하신것 같은데 중복되는 의존성은 제거해도 좋을 것 같습니다 :)
- 루트 프로젝트의 build.gradle에서 all-project, sub-project의 종속성들을 한 번에 관리해주는 방법도 있습니다.
- 이 방법은 프로젝트를 진행하는 팀들의 컨벤션에따라 달라질 수도 있겠네요 :)
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.
아래처럼 수정했습니다! allprojects, subprojects 차이점이 뭘까요? 둘 다 모든 프로젝트에 영향을 주는 설정 같아서요.
root / build.gradle
plugins {
id 'java'
id 'org.springframework.boot' version '3.3.2'
id 'io.spring.dependency-management' version '1.1.6'
}
java {
toolchain {
languageVersion = JavaLanguageVersion.of(21)
}
}
allprojects {
group = 'com.example'
version = '0.0.1-SNAPSHOT'
repositories {
mavenCentral()
}
}
subprojects {
apply plugin: 'java'
apply plugin: 'org.springframework.boot'
apply plugin: 'io.spring.dependency-management'
dependencies {
implementation 'org.springframework.boot:spring-boot-starter-data-jpa'
implementation 'org.springframework.boot:spring-boot-starter-validation'
implementation 'org.springframework.boot:spring-boot-starter-web'
compileOnly 'org.projectlombok:lombok'
annotationProcessor 'org.projectlombok:lombok'
developmentOnly 'org.springframework.boot:spring-boot-devtools'
testImplementation 'org.springframework.boot:spring-boot-starter-test'
testRuntimeOnly 'org.junit.platform:junit-platform-launcher'
}
}
movie-api / build.gradle
dependencies {
implementation project(':core')
implementation project(':infrastructure')
}
bootJar {
enabled = true
}
tasks.named('test') {
useJUnitPlatform()
}
core / build.gradle
dependencies {
implementation 'com.fasterxml.jackson.core:jackson-annotations:2.17.2'
}
bootJar {
enabled = false
}
tasks.named('test') {
useJUnitPlatform()
}
infrastructure / build.gradle
dependencies {
implementation project(':core')
runtimeOnly 'com.mysql:mysql-connector-j'
}
bootJar {
enabled = false
}
tasks.named('test') {
useJUnitPlatform()
}
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.
- allprojects는 루트 프로젝트와 모든 하위 프로젝트에 공통 설정 적용
- subprojects는 모든 하위 프로젝트들에만 적용할 설정
루트에서 application을 구동시키는 경우도 있고, 다른 상황도 있을 수 있어 이 경우에 allprojects도 사용됩니다.
현재 찬엽님 프로젝트에서는 root 모듈의 subprojects들에 모든 모듈이 포함됨으로 subprojects만 사용해도 관리 가능합니다 :)
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.
[좋았던 점]
- 아키텍처 :
- core 모듈에 불필요한 모듈 의존성이 없다.
- DIP를 통해 DB, JPA와의 결합도를 최대한 약화시키려고 노력한 점이 매우 좋았다.
- 테이블 :
- 비즈니스 로직 구현에 무리가 없도록 정확하게 설계했고 데이터 타입또한 잘 정의했다.
- DDL에서 디테일하지만 기본적인 charset, collate를 설정한 점이 좋았다.
- API 구현 :
- 엔티티가 API 스펙으로 노출 and 종속되지 않도록 잘 구현했다.
[아쉬운 점]
- 아키텍처 :
- DB와의 의존성을 더 개선할 수 있는 방법이 존재한다.
- Domain에 대한 생각을 조금 더 해보면 좋을것 같습니다.
- 테이블 :
- N : N 관계를 풀어줄 중간 테이블을 설계했지만 구현되지 않음
[다음 과제에서 신경쓰면 좋을 부분]
- "도메인"에 대한 의존성을 개선시키는 방향으로 생각해보면 좋을듯 합니다.
- 문제해결에 앞서(eg OneToMany) 특정 기능, 애노테이션이 "구조적, 비즈니스적"으로 꼭 필요한 것인지 이유가 있으면 좋겠습니다.
- 특정 애너테이션을 습관처럼 쓰지말고 어떠한 이유에서 사용하는지 파악하면 불필요한 부분이 없어져 좋을듯합니다.
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.
System Properties를 BaseEntity로 정의해서 사용을 잘하셨습니다!
- 다만, Data 애노테이션의 모든 기능이 필요한 건지? 최대한 필요한 메서드만 캡슐화의 관점에서 사용하시면 더 좋을듯 합니다.
- BaseEntity 클래스는 직접 생성되지 않고 상속을 통해서만 사용되기 때문에, abstract로 선언하면 더 설계 의도가 명확해질 것 같습니다.
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.
Getter 어노테이션으로 변경했고 의견주신대로 abstract으로 선언하였습니다!
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.
- 불변성을 보장하는 record 사용 적절히 잘 하셨습니다 👍
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.
현재 MovieDto가 Api 스펙(http response)라고 판단됩니다.
- 단순히 API 스펙에 맞게 변환하는 로직만 들어있어서 책임과 역할이 좋다고 생각합니다 👍
- record를 사용하면 불필요한 애노테이션을 제거할 수 있습니다.
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.
record + @builder 조합으로 변경했습니다!
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.
API 요구사항에 맞게 Movie 테이블을 설계하고 엔티티를 만들어주셨습니다 👍
다만 몇 가지 걸리는 점을 보자면,
-
현재 Setter의 필요성?
- 당장 사용되지 않는 코드이며, Setter를 사용해서 외부에서 "상태변경"할 가능성이 열려있습니다.
- 기본적으로 객체 내부에서 상태를 변경하도록 구현하는 것이 코드 재사용성, 캡슐화에 좋다고 생각합니다.
-
OneToMany, ManyToMany가 요구사항을 구현하기 위해 "꼭 필요한가?" 궁금합니다.
- 저는 해당 애노테이션들은 득보다 실이 훨씬 많다고 생각하는 편입니다.
- 경험상 해당 애노테이션을 쓰지 않고도 모든 비즈니스 로직을 구현할 수 있습니다.(e.g 중간테이블, join etc)
- 위 애노테이션(연관관계 특히 양방향)을 사용함으로써 "N+1문제"를 세심하게 고려해줘야하는 가능성이 만들어집니다.
- entity graph, fetch join, batch size, querydsl 등 방법으로 해결할 수 있지만, 이러한 구조를 애초에 만들지 않는 것도 하나의 방법입니다.
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.
현재 Setter의 필요성?
OOP 관점에서 말씀해주시는게 좋은거 같습니다.
Setter보다는 도메인 행위(?)로 상태를 수정하는게 더 좋을거라 생각해 Setter 제거했습니다.
경험상 해당 애노테이션을 쓰지 않고도 모든 비즈니스 로직을 구현할 수 있습니다.(e.g 중간테이블, join etc)
일단 N:N 경우 중간테이블 Entity로 만들었습니다. 이걸 어떻게 활용해볼지 고민해보겠습니다.
위 애노테이션(연관관계 특히 양방향)을 사용함으로써 "N+1문제"를 세심하게 고려해줘야하는 가능성이 만들어집니다.
네 맞습니다. 사실 해당 문제 때문에 실무에서도 Entity만 만들고 모든 연관 관계는 querydsl로 해결하고 있습니다.
entity graph, fetch join, batch size, querydsl 등 방법으로 해결할 수 있지만, 이러한 구조를 애초에 만들지 않는 것도 하나의 방법입니다.
👍
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.
start, end에 String을 사용하신 이유가 특별히 있으신가요? 저는 LocalTime을 사용하는것이 비즈니스 로직 구현 및 컬럼 타입에 적합해보입니다 :)
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.
맞네요 ㅎㅎ LocalTime으로 수정했습니다.
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.
Restful 하게 잘 url을 만들어주셨네요, 하지만 많은 곳에서 컨벤션으로 백엔드 API는 api/v1/movies 패턴을 사용합니다.
- 특히 v1(버전관리)는 현업에서 생각보다 중요하다고 경험한 점이었습니다. 이유를 한 번 생각해주셔도 좋을듯합니다.
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.
/v1 추가했습니다. 이유는 아래처럼 생각해봤습니다.
- 히스토리 추적 가능. e.g) v3부터 해당 값이 추가되었구나
- 오류 최소화. 갑자기 응답 구조 변경하면 해당 API에 의존하는곳까지 장애가 전파되기 때문.
이건 개인적인 생각인데, 버전을 적용하면 관리 포인트도 함께 증가한다 생각하는데요.
e.g) 새로운 응답 구조를 위해 추가 dto가 생기고 관리해야하는 endpoint 역시 증가.
저는 어쩔 수 없는 상황아니면 버전 관리는 최소화 시키는게 좋다고 생각합니다 🤔
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.
이유 맞습니다 👍
- 보통 API 스펙에서 값이 추가되는 경우는 문제가 없지만, 삭제될때 해당 API를 사용하고 있는 경우 장애가 전파가 됩니다
- 모바일 애플리케이션을 예를 들어보면 버전관리를 안해주면 특정 버전(앱 업데이트)으로 넘어오지 않은 유저는 기능 장애를 겪게됩니다.
- 말씀해주신 것처럼 최대한 관리 포인트를 줄이는 것이 좋은데, 조직 내부에서 특정 시점에는 불필요하다고 생각되는 엔드 포인트를 삭제할 수 있도록 잘 조율하는 것이 중요합니다 :)
|
|
||
| private final MovieRepoService movieRepoService; | ||
|
|
||
| public List<MovieDto> getMovies(final SearchMovies searchMovies) { |
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.
SearchMovies은 record인데 final 키워드는 큰 의미가 없다고 생각이 듭니다.
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.
맞네요. final 지웠습니다!
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.
아키텍처를 그려주셔서 한 눈에 파악할 수 있어 너무 좋네요 👍
-
DB에 최대한 의존하지 않도록 아키텍처를 설계하신것 좋습니다.
-
나중에 Mock 프레임워크에 의존하지 않고도 테스트 코드를 짤 수 있는 구조라 좋았습니다!
-
궁금한 점은 Entity == Domain으로 생각하고 설계하신건지 궁금합니다.
-
DB에 최대한 의존하지 않도록 한다면 "영속성 엔티티"와 "도메인 객체"(비즈니스 그 자체)가 분리되어야 좀 더 의존성을 분리할 수 있습니다(설계적으로)
-
현재와 같은 구조에서 DB가 바뀐다면?
- JPA도 못쓰는 경우 -> JPA에 의존한 Entity 모두 사용불가 변화해야함 -> Entity에 핵심 비즈니스 로직들이 있다면?
-
이런 경우도 한 번 생각해보시면 좋을 듯 합니다(물론 현재 구조로도 모든 서비스는 잘! 구현할 수 있습니다!)
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.
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.
ERD를 보면 중간 테이블을 넣어주셨는데 코드 구현은 되어있진 않네요.
- N : N 관계를 중간 테이블로 잘 풀어주시면 많은 문제가 구조적으로 해결될 것 같습니다.
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.
Entity를 만들어도 활용을 못하겠어서 구현을 안했습니다. 일단 Entity 추가했고 어떻게 활용해볼지 고민해보겠습니다 🔥
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.
module 설정 잘 해주셨네요 Good!

[1주차] 영화 서비스 초기 구성 및 상영 중인 영화 조회 API 개발
작업 내용
showDate(상영 날짜)query param으로 개봉일과 비교발생했던 문제와 해결 과정을 남겨 주세요.
문제 1
또한 양방향 OneToMany 이슈가 발생했었습니다.
해결 방법 1
@JoinTable어노테이션 사용했습니다.이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.
showtimes가져올때 Join 조회 한번,theaters가져올때 Join 조회 다시 한번, 총 2번 조회합니다. 완벽히 N+1 해결하려면 QueryDSL 밖에는 생각이 안나네요.리뷰 포인트
기타 질문