Conversation
|
안녕하세요 근태님! 1주차 리뷰어를 맡게된 권순한입니다! |
| * 도커컴포즈 변수 파일 | ||
| * `.env` | ||
| * 보안을 위해 `.gitignore`에 추가해야 하지만 교육과제므로 github에 올림 | ||
| * `.env.dev`, `.env.prod` 나누어 환경별로 다르게 적용도 가능 |
| > cinema-domain | ||
| > cinema-infrastructure | ||
|
|
||
| 모듈은 헥사고날 아키텍처의 계층에 따라 나누었습니다. |
There was a problem hiding this comment.
헥사고날 아키텍처를 너무 잘 설계해주신듯 합니다 👍
- 아키텍처에 대한 설명도 이해하기 쉽게 너무 잘해주셨습니다!
| developmentOnly("org.springframework.boot:spring-boot-devtools") | ||
| } | ||
|
|
||
| subprojects { |
There was a problem hiding this comment.
서브 프로젝트의 공통 의존성, 서브 프로젝트 별 필요한 의존성을 잘 설정해주셨네요 👍
cinema-adapter/src/main/java/com/hanghae/adapter/web/MovieController.java
Outdated
Show resolved
Hide resolved
| public class MovieController { | ||
| private final MovieScheduleService movieScheduleService; | ||
|
|
||
| public MovieController(MovieScheduleService movieScheduleService) { |
There was a problem hiding this comment.
프로젝트 전반에 Lombok을 사용하는 만큼 적절하게 RequiredArgsConstructor 와 같은 애노테이션을 사용해서 보일러 플레이트 코드를 없애면 좋겠다고 생각이 듭니다 :)
| @Getter | ||
| @AllArgsConstructor | ||
| public class ErrorResponse { | ||
| private int status; |
There was a problem hiding this comment.
단순한 response는 final 키워드를 사용하면 좋을것 같습니다 :)
- 불변성과 불필요한 애노테이션을 사용하지 않기위해 record 사용도 고려하면 좋겠습니다.
There was a problem hiding this comment.
넵 record 를 좀 더 활용해 간단하게 표현하도록 하겠습니다!
| # ?? ?? ?? (???: 8080) | ||
| server.port=8080 | ||
|
|
||
| # MySQL ?????? ?? ?? |
There was a problem hiding this comment.
Datasource의 timezone, charset, collation에 대한 기본적인 설정이 없어서 아쉽습니다.
| String rating, //영상물 등급 | ||
| LocalDate releaseDate, //개봉일 | ||
| String thumbnailPath, // 썸네일 경로 | ||
| Long runningTime, // 러닝타임(분) |
There was a problem hiding this comment.
주석으로 잘 달아주셨지만, runningTime 변수명만으로 시간단위가 "분"이라는 것을 파악하기 어렵습니다 :)
| public List<MovieScheduleDto> getMovieSchedules() { | ||
| List<ScreeningSchedule> schedules = repositoryPort.findAll(); | ||
|
|
||
| return schedules.stream().map(schedule -> { |
There was a problem hiding this comment.
해당 로직 흐름의 가독성이 많이 떨어집니다. return 내부에 또 return이 존재해서 코드를 자세히 읽어봐야 알 수 있네요.
- 코드를 구조화시켜서 가독성을 개선시키면 좋을듯 합니다 :)
|
|
||
| @Getter | ||
| @AllArgsConstructor | ||
| public class Movie { |
There was a problem hiding this comment.
도메인 객체로 보이는데 "상태 변화"를 하지 않는 객체라는것에 조금 의문이 듭니다.
- 불변성을 유지하는데는 장, 단점이 있는데 이 경우 과하게 객체가 생성될 것으로 예상됩니다.
- 로직에 의해서 상태가 변하면? -> 재생성해서 -> 엔티티로 변환 -> 저장 이런식으로 흘러가게 될까요?
- 도메인 객체라면 상태가 변화하는 핵심 비즈니스 로직이 생기게될텐데 어떻게 생각하셨는지 궁금하네요 :)
There was a problem hiding this comment.
객체가 중간에 어디서 수정되었는지 모를 문제를 막기위해 getter만 넣어 설계했습니다!
코치님 말씀처럼 과하게 객체가 생성되는 문제가 발생할 수 있을것 같습니다.
해당 부분 좀 더 유연하게 흘러가도록 고민해 보겠습니다!
| this.updatedAt = updatedAt; | ||
|
|
||
| // 개봉일 검증 로직 | ||
| validateReleaseDate(movie.getReleaseDate()); |
|
|
||
| @Getter | ||
| @RequiredArgsConstructor | ||
| public enum MovieGenre { |
There was a problem hiding this comment.
ENUM으로 접근하신 것은 좋은 선택이라고 생각합니다. 👍
- 한 가지 소개해드릴 방법은 Converter를 사용해서 데이터베이스에 'ACTION'이면 -> 'A' 이렇게 저장해서 데이터베이스 리소스를 절약하는 방법도 있습니다.
There was a problem hiding this comment.
오 데이터베이스 리소스를 절약하는 방법 정말 좋은것 같습니다!!
적용 하도록 하겠습니다!
| @Override | ||
| public List<ScreeningSchedule> findAll() { | ||
| // jpa 엔티티 > 도메인 모델로 변환하여 리턴 | ||
| return repository.findAll().stream().map(entity -> new ScreeningSchedule( |
There was a problem hiding this comment.
이 부분도 가독성이 많이 떨어지네요. 해당부분은 빌더패턴을 적절히 사용하면 가독성 및 실수할 여지가 많이 줄어들듯 합니다 :)
| @Entity | ||
| @Getter | ||
| @DynamicInsert | ||
| @DynamicUpdate |
There was a problem hiding this comment.
DynamicInsert, Update를 사용할 만큼의 데이터를 염두해 두시고 사용하신건지 궁금합니다 :)
There was a problem hiding this comment.
모든 필드를 수정하는 일이 적을 것 같아 작성하였습니다!
| @Column | ||
| private Long createdBy; // 작성자 ID | ||
|
|
||
| @CreationTimestamp |
There was a problem hiding this comment.
System Properties는 BaseEntity로 정의해서 사용하면 어떨까요?
- Domain 객체를 따로 사용하기 때문에 테스트시에도 큰 문제는 없을듯 합니다.
There was a problem hiding this comment.
넵 BaseEntity 를 사용해서 중복되는 코드를 줄이도록 하겠습니다!
| import com.hanghae.infrastructure.entity.MovieEntity; | ||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| public interface MovieRepositoryJpa extends JpaRepository<MovieEntity, Long> { |
There was a problem hiding this comment.
MovieJpaRepository, MovieRepositoryJpa 왜 동일한 레포지토리가 2개가 있는지 궁금합니다 :)
There was a problem hiding this comment.
처음에 MovieJpaRepository 이런식으로 메서드명을 작성하였는데 jpa가 중간에 있어 port에 Repository와 햇갈렸습니다!
그래서 Jpa를 맨뒤에 주어 보기 쉽게 하려고했는데 이름이 변경된 소스가 지워지지 않은것 같습니다 ㅠ
| @DataJpaTest | ||
| @ActiveProfiles("test") // application-test.properties 사용 | ||
| @AutoConfigureTestDatabase(replace = AutoConfigureTestDatabase.Replace.NONE) | ||
| public class ScreeningScheduleRepositoryTest { |
There was a problem hiding this comment.
DataJpaTest 를 사용해서 테스트 진행해주신 부분 좋습니다! Medium Test 또는 Slice Test 라고 할 수 있겠네요. 다만 좀 더 생각해볼 점은
- 결국 테스트라는게 자주 돌려야하고 속도가 중요해지는 시점이 현업에서 꼭 옵니다.(eg ci/cd시 테스트 프로세스가 들어가있다면 빌드속도 영향)
- 지극히 개인적인 관점으로 의견을 드리면 JPA repository에 대한 테스트는 JPA에서 이미 보증하고 있다고 생각합니다.
- 도메인, 서비스에 대한 테스트 그리고 통합테스트(integration test)가 훨씬 중요하다고 생각하는 편이어서 해당 테스트가 있으면 더욱 좋을듯 합니다.
There was a problem hiding this comment.
ci/cd시 테스트 프로세스에 대해 생각하지 못했습니다 ㄷㄷ
비즈니스 로직에 대한 테스트 코드에 집중하도록 하겠습니다!
| image: mysql:8.0 # 사용할 도커 이미지, 버전을 명시하는 것이 좋다고함 | ||
| # restart: always # 컨테이너가 종료되면 자동 재시작 | ||
| container_name: mysql_cinema # 생성될 컨테이너 이름, 작성안하면 임의로 지정됨 | ||
| environment: # 컨테이너에 전달할 환경 변수 설정, |
There was a problem hiding this comment.
Docker Compose 파일을 잘 작성해주셨네요 👍
- Charset, Timezone, Collation에 대한 설정이 없는 것이 아쉽습니다.
There was a problem hiding this comment.
넵 해당 설정에 Charset, Timezone, Collation 정보 추가하도록 하겠습니다!
docs/cineam_create.sql
Outdated
| @@ -0,0 +1,73 @@ | |||
| CREATE TABLE movie ( | |||
| movie_id int AUTO_INCREMENT PRIMARY KEY COMMENT '영화 ID', | |||
There was a problem hiding this comment.
int 말고 unsigned int형을 사용하면 좋았을 것 같습니다. 불필요한 DB 리소스 낭비라고 생각됩니다.
There was a problem hiding this comment.
좋은 정보 감사드립니다!
unsigned int로 수정하도록 하겠습니다!
|
내용이 많아서 쫌 넓은 시선으로 리뷰했습니다. PR 사이즈가 작으면 좀 더 깊게 리뷰할텐데, 아쉽습니다. 부족한 부분 생각나면 보충해드리겠습니다 :) 근태님이 아키텍처에 대한 설명을 간결하게 잘 써주셔서 이해하기 좋았습니다 👍 좋았던 점
아쉬웠던 점
리뷰 포인트
|
|
PR을 늦게 올렸는데도 자세한 리뷰 정말 감사드립니다!! |
다른 저장소에 PR을 잘못올려 다시 올립니다 ㅠㅠ
늦어서 정말 죄송합니다 ㅠㅠ
상영중인 영화 조회 API는 시간이 없어 완성하지 못했습니다.
DB도 시간부족으로 도커컴포즈 없이 로컬로 작업 하였습니다.
도커컴포즈 적용등 추가적인 작업이 필요합니다 ㄷㄷ
작업 내용
발생했던 문제와 해결 과정을 남겨 주세요.
문제 : 멀티모듈 구성 의존성 문제가 발생 (다른 모듈 클래스
import불가)build.gradle.kts파일에 의존성을 각각 주어 해결문제 : 다른 모듈의 빈을 찾지 못해 서버가 실행되지 안는 문제 발생
AdapterApplication클래스에@EnableJpaRepositories,@EntityScan,@ComponentScan어노테이션을 추가하여 해결문제 : DB에 접근되지 되지 않는 문제 발생
SpringApplication.run()가 위치한 모듈에 있는application.properties파일에 작성하여 해결이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.
리뷰 포인트
감사합니다~!