Conversation
|
안녕하세요 형재님! 1주차 리뷰어를 맡게된 권순한입니다! |
|
|
||
| private final MovieService movieService; | ||
|
|
||
| @GetMapping("/") |
There was a problem hiding this comment.
API url이 restful하게 작성되면 좋을 것 같습니다 :)
| private final MovieService movieService; | ||
|
|
||
| @GetMapping("/") | ||
| public List<MovieResponse> getMovieList() { |
There was a problem hiding this comment.
리턴 타입이 List인데 메서드 이름에 또 List를 붙이는 것은 개인적으로는 불필요하다고 생각하는 편입니다 :)
|
|
||
| private static List<String> createTheaters(Movie movie) { | ||
| return movie.getMovieTheaters().stream() | ||
| .map(movieTheater -> movieTheater.getTheater().getName()) |
There was a problem hiding this comment.
map을 쓸 때 메서드 레퍼런스를 활용한 방법도 소개해드립니다.
.map(MovieTheater::getTheater)
.map(Theater::getName)|
|
||
| @Getter | ||
| public class MoviesServiceResponse { | ||
| public static List<MovieResponse> of(List<Movie> movies) { |
There was a problem hiding this comment.
불필요한 변수 a, forEach 와 같은 부분은 제거하고 바로 stream을 통해서 리턴해주는게 가독성 면에서 좋아보입니다 :)
There was a problem hiding this comment.
급하게 개발하다보니까 나중에 수정한다는걸 깜빡했습니다
stream을 이용해서 코드를 개선했습니다
| my-db: | ||
| container_name: theater_mysql | ||
| image: mysql | ||
| environment: |
There was a problem hiding this comment.
Timezone, Charset 에 대한 부분이 없어서 조금 아쉽습니다. 현업에서 많은 버그를 만들어내는 부분이기도 해서요 :)
There was a problem hiding this comment.
mysql 초기설정시 Timezone, Charset 부분을 알아보고 중요성을 알게되었습니다
docker-compose.yml에 설정추가했습니다!
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object object) { |
There was a problem hiding this comment.
equals and hashcode 재정의한 이유가 특별하게 있으셨나요?
There was a problem hiding this comment.
Movie 클래스에서 Screening, MovieTheater Set 자료구조를 사용하고있습니다.
Set 자료구조를 오류없이 사용하려먼 equals and hashcode를 재정의해서 사용해야하는걸로 알고있어서 사용했습니다
| import java.util.List; | ||
|
|
||
| public interface MovieRepository extends JpaRepository<Movie, Long> { | ||
| List<Movie> findAllByOrderByReleaseDateDesc(); |
There was a problem hiding this comment.
OrderByReleaseDateDesc 대신 Sort를 사용하면 가독성, 메서드 재사용성, 유연성이 개선될것 같습니다 😃
There was a problem hiding this comment.
Sort를 사용하여 외부에서 값을 받아 정렬할 수 있도록 수정했습니다!
| import static org.assertj.core.api.Assertions.*; | ||
|
|
||
| @Transactional | ||
| @SpringBootTest |
There was a problem hiding this comment.
DataJpaTest 를 사용해서 불필요한 부분은 로드하지 않도록 테스트를 만드는게 좋을것 같다고 생각합니다.
결국 테스트라는게 자주 돌려야하고 속도가 중요해지는 시점이 현업에서 꼭 옵니다.(eg ci/cd시 테스트 프로세스가 들어가있다면 빌드속도 영향)
- 지극히 개인적인 관점으로 의견을 드리면 JPA repository에 대한 테스트는 JPA에서 이미 보증하고 있다고 생각합니다.
- 도메인, 서비스에 대한 테스트 그리고 통합테스트(integration test)가 훨씬 중요하다고 생각하는 편입니다.
There was a problem hiding this comment.
피드백 감사합니다!
앞으로 테스트 작성 시 이러한 관점을 참고해서 더 개선된 방향으로 진행하도록 하겠습니다.
init/01-schema.sql
Outdated
| @@ -0,0 +1,52 @@ | |||
| -- Movie 테이블 생성 | |||
| CREATE TABLE movie ( | |||
| id BIGINT AUTO_INCREMENT PRIMARY KEY, | |||
There was a problem hiding this comment.
BIGINT 말고 unsigned int형을 사용하면 좋았을 것 같습니다. 불필요한 DB 리소스 낭비라고 생각됩니다.
There was a problem hiding this comment.
unsigned int을 사용하기위해 entity에 @column(columnDefinition = "INT UNSIGNED") 사용했습니다!
init/01-schema.sql
Outdated
| updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, | ||
| movie_id BIGINT NOT NULL, | ||
| theater_id BIGINT NOT NULL, | ||
| FOREIGN KEY (movie_id) REFERENCES movie(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
실무에서는 FK 참조 무결성으로 인한 성능, 스키마 변경, 데이터 이관 및 데이터 클렌징에 대한 편의성 때문에 FK 를 걸지 않는 경우가 많습니다 :)
- 데이터의 무결성이 아주 중요한 서비스는 아니므로 FK constraint를 삭제하는 것도 고려해주시면 좋겠습니다.
There was a problem hiding this comment.
JPA를 사용하면 연관관계를 설정할 경우 기본적으로 외래 키(FK)가 자동으로 생성됩니다.
그렇다면, 스키마 생성 시 외래 키 제약 조건(FK)이 적용되지 않도록 설정할 방법이 있을까요?
There was a problem hiding this comment.
jpa hibernate ddl-auto 를 validate 나 none으로 설정하시고 DDL은 직접 데이터베이스에서 실행하셔도 됩니다 :)
리뷰 포인트Readme에 구성과 설명을 잘 정리해주셔서 좋았습니다! :)
기타 질문
|
There was a problem hiding this comment.
불필요한 래퍼클래스라고 보이는데 특별하게 여기서 변환하는 이유가 있을까요?
[ 2주차 잔여 ] Feature/caching 캐싱 테스트 완료
3 participants
[1주차] 멀티모듈 구성 및 영화목록 조회 API 개발
작업 내용
발생했던 문제와 해결 과정을 남겨 주세요.
문제 1
해결방법
문제 2
해결방법
이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.
리뷰 포인트
기타 질문