Redis 단기 스킬업 1주차 리팩토링
리팩토링 목록
- Domain 클래스에 id 필드 추가
- Dto 변환 로직 Dto 내부로 이동
- Cinema, Theater 도메인 개념 추가
- 좌석 일급 컬렉션 생성 시 entity to domain 변환 쉽게 메서드 추가
- 상영 중인 영화 조회 로직 리팩토링
- 응답 형식 수정
- 필드명, 메서드명 수정
Domain 클래스에 id 필드 추가
1주차 요구 사항을 구현하면서 domain과 entity를 분리하였다.
이때 domain의 의존성을 최소화 하기 위해 id 필드를 넣지 않았다.
그렇게 했더니 entity를 가져와서 domain으로 변환할 때 번거로운 점들이 있었다.
또 entity들을 조회하거나 update, delete 할 때에도 번거로웠다.
코드 리뷰를 받으며 질문해보니 튜터님도 id 필드는 필요할 것 같다고 말씀하셨다.
따라서 Domain 클래스에 id 필드를 추가하였다.
그에 따라 entity to domain 역할을 하는 mapper 클래스들의 메서드에도 변화가 생겼다.
Dto 변환 로직 Dto 내부로 이동
이전에 마지막에 정신 없이 코드를 작성하다 보니 Dto로 변환하는 로직을 Service 클래스에 두었다.
아래 코드처럼 마지막에 메서드로 두었다.
@Service
@RequiredArgsConstructor
public class MovieService {
private final MovieAdapter movieAdapter;
private final ScreeningAdapter screeningAdapter;
/*
중간 코드 생략
*/
public NowPlayMovieDto makeNowPlayMovieDto(Movie movie, String cinemaName, List<Screening> screenings) {
return NowPlayMovieDto.builder()
.movieName(movie.getTitle())
.movieRate(movie.getRating().getMovieRateDescription())
.movieReleaseDate(movie.getReleasedAt())
.movieThumbnailImage(movie.getThumbnail())
.movieRunningTime(movie.getDuration())
.movieGenre(movie.getGenre().getMovieGenreDescription())
.cinemaName(cinemaName)
.screenings(screenings)
.build();
}
}
코드 리뷰를 받으면서 이런 변환 로직은 Dto 내부에 있는 것이 좋다고 하셔서 반영하였다.
@Getter
public class NowPlayMovieDto {
private String movieGenre;
private String cinemaName;
private List<Screening> screenings;
public static NowPlayMovieDto of(Movie movie, String cinemaName, List<Screening> screenings) {
return NowPlayMovieDto.builder()
.movieName(movie.getTitle())
.movieRate(movie.getRating().getMovieRateDescription())
.movieReleaseDate(movie.getReleasedAt())
.movieThumbnailImage(movie.getThumbnail())
.movieRunningTime(movie.getDuration())
.movieGenre(movie.getGenre().getMovieGenreDescription())
.cinemaName(cinemaName)
.screenings(screenings)
.build();
}
}
그리고 Service 클래스의 메서드에서 정적 팩터리 메서드를 사용하는 것으로 코드가 수정되었다.
이렇게 리팩토링하니 더 책임이 잘 분리되고 코드도 깔끔해지고 가독성이 올라간 것 같다.
Cinema, Theater 도메인 개념 추가
이전에는 영화관을 의미하는 Cinema라는 개념만 존재하였다.
하지만 영화관 안에 상영관이 있고 각 상영관마다 좌석이 존재하는 것이다.
그리고 각 상영관에 대해 상영관에서 상영하는 상영 정보가 존재한다.
따라서 상영관을 의미하는 Theater라는 도메인 개념을 추가하였다.
이전 ERD와 수정된 ERD는 아래와 같다.
이전 ERD
수정된 ERD
이렇게 하니 조금 더 복잡해지기는 한 것 같다.
상영 정보를 가져올 때 영화관 이름과 상영관 이름을 다 가져오게 하는 등 조금 더 복잡해졌다.
하지만 복잡할수록 해결해가는 과정에서 고민을 많이 하기 때문에 도메인 개념을 추가해봤다.
좌석 일급 컬렉션 생성 시 entity to domain 변환 쉽게 메서드 추가
이전 1주차를 구현하면서 굉장히 머리 아팠던 부분이다.
엔티티와 도메인을 분리하면서 상영관 도메인 클래스에 좌석들을 의미하는 필드를 만들어놨다.
이때 아무 생각 없이 'n*n이니까 List<List<Seat>>으로 해야지' 했다가 더 복잡해졌다.
엔티티에서는 List<List<Seat>> 타입을 사용할 수 없어서 List<Seat>으로 했다.
이때라도 도메인도 List<Seat>으로 수정해야 했다.
그러나 도메인을 엔티티와 분리한 이유가 List<List<Seat>>으로 표현해 n*n 좌석임을 잘 보여주도록 하는게 아닌가 이런 생각을 했다.
생각해보면 일급 컬렉션을 적용할 것이면 어차피 List<List<Seat>>은 잘 보이지 않는데 굳이 List<List<Seat>>로 한 것 같다.
이렇게 하니 상영관을 entity to domain을 할 때 굉장히 복잡했다.
우선 List<Seat>을 List<List<Seat>>으로 변환해야 하니 row와 col을 비교해서 넣고 정렬하고 해야 했다.
그리고 더 문제는 상영관 Domain 클래스를 만들 때 아래처럼 좌석 일급 컬렉션을 바로 생성하도록 생성자를 지정해놨다.
@AllArgsConstructor(access = AccessLevel.PRIVATE)
public class Cinema {
public static final int ROWS = 5;
public static final int COLUMNS = 5;
private String cinemaName;
private CinemaSeats cinemaSeats;
public static Cinema of(String cinemaName) {
return new Cinema(cinemaName, CinemaSeats.create());
}
}
상영관을 생성할 때에는 당연히 좌석이 생성되는게 보장되어야 하니 저렇게 구현을 하였다.
하지만 entity to domain을 생각하지 않고 구현해서 문제가 된 것이다.
CinemaSeats.create()는 List<List<Seat>>을 늘 새로 생성하는 메서드이다.
하지만 entity to domain을 하면 이전에 좌석 정보들이 와야 하는데 Cinema 클래스를 생성할 때 좌석이 새로 생겨버린다.
이렇게 해서 정신이 나간채로 로직을 그냥 주석 처리해놓고 추후에 짜야지 했던 기억이 난다.
그래서 리팩토링한 것은 아래와 같다.
- 좌석 일급 컬렉션 List<List<Seat>> → List<Seat>으로 수정
- Cinema 클래스 및 TheaterSeats 클래스 (좌석 일급 컬렉션)에 List<Seat>을 받는 생성자 및 정적 팩터리 메서드 추가
Cinema 클래스와 TheaterSeats 클래스를 생성할 때 이전 좌석 정보를 받아서 생성할 수 있게 메서드들을 추가하였다.
그리고 List<Seat>으로 하면서 변환 로직이 매우 간단해지고 깔끔해졌다.
각 리팩토링한 commit은 아래와 같다.
좌석 정보를 받아서 생성할 수 있게 메서드들을 추가 : https://github.com/CJ-1998/redis_2nd/commit/245f12f40907792edf68726291687a533cec1906
entity to domain 변환 로직 수정 : https://github.com/CJ-1998/redis_2nd/commit/5dfb7e8b03710b714e90672052a62b347d24476d
우선 Cinema 클래스 전과 후이다.
전
@AllArgsConstructor(access = AccessLevel.PRIVATE)
public class Theater {
private Long theaterId;
@Getter
private String theaterName;
private TheaterSeats theaterSeats;
public static Theater of(Long theaterId, String theaterName) {
return new Theater(theaterId, theaterName, TheaterSeats.create());
}
}
후
@AllArgsConstructor(access = AccessLevel.PRIVATE)
public class Theater {
private Long theaterId;
@Getter
private String theaterName;
private TheaterSeats theaterSeats;
public static Theater of(Long theaterId, String theaterName) {
return new Theater(theaterId, theaterName, TheaterSeats.create());
}
public static Theater of(Long theaterId, String theaterName, List<Seat> seats) {
return new Theater(theaterId, theaterName, TheaterSeats.create(seats));
}
}
좌석 일급 컬렉션인 TheaterSeats 전과 후이다.
전
@AllArgsConstructor(access = AccessLevel.PRIVATE)
public class TheaterSeats {
public static final int ROWS = 5;
public static final int COLUMNS = 5;
private final List<List<Seat>> seats;
public static TheaterSeats create() {
return new TheaterSeats(createSeats());
}
private static List<List<Seat>> createSeats() {
List<List<Seat>> seats = new ArrayList<>();
for (int row = 0; row < ROWS; row++) {
List<Seat> seat = new ArrayList<>();
for (int col = 0; col < COLUMNS; col++) {
Seat oneSeat = createOneSeat(row, col);
seat.add(oneSeat);
}
seats.add(Collections.unmodifiableList(seat));
}
return Collections.unmodifiableList(seats);
}
private static Seat createOneSeat(int row, int col) {
String seatRow = getSeatRow(row);
Integer setColumn = col + 1;
return Seat.of(null, false, seatRow, setColumn);
}
private static String getSeatRow(int rowNumber) {
return String.valueOf((char) ('A' + rowNumber));
}
}
후
public class TheaterSeats {
public static final int ROWS = 5;
public static final int COLUMNS = 5;
private final List<Seat> seats;
private TheaterSeats() {
seats = new ArrayList<>();
for (int row = 0; row < ROWS; row++) {
for (int col = 0; col < COLUMNS; col++) {
Seat oneSeat = createOneSeat(row, col);
seats.add(oneSeat);
}
}
}
private TheaterSeats(List<Seat> seats) {
this.seats = new ArrayList<>(seats);
}
public static TheaterSeats create() {
return new TheaterSeats();
}
public static TheaterSeats create(List<Seat> seats) {
return new TheaterSeats(seats);
}
private Seat createOneSeat(int row, int col) {
String seatRow = getSeatRow(row);
Integer setColumn = col + 1;
return Seat.of(null, false, seatRow, setColumn);
}
private String getSeatRow(int rowNumber) {
return String.valueOf((char) ('A' + rowNumber));
}
}
마지막으로 entity to domain 해주는 mapper 클래스 전과 후이다.
전 (코드를 작성하다 중간에 넋이 나가버렸다)
@Component
@RequiredArgsConstructor
public class TheaterMapperImpl implements TheaterMapper {
private final SeatMapper seatMapper;
@Override
public Theater toDomain(TheaterEntity theaterEntity) {
return Theater.of(theaterEntity.getTheaterId(), theaterEntity.getTheaterName());
}
public List<Seat> convertSeatEntitiesToSeats(List<SeatEntity> seatEntities) {
return seatEntities.stream()
.map(seatMapper::toDomain)
.toList();
}
// CineamSeats를 Cinema 생성할 때마다 자동 생성하게 하니 Entity에서 domain 클래스로 변환할 때 어떻게 해야 할지 모르겠음
/*
public CinemaSeats convertSeatsToCinemaSeats(List<Seat> seats) {
List<Seat> sortedSeats = sortSeats(seats);
List<List<Seat>> cinemaSeats = new ArrayList<>();
String startRow = sortedSeats.get(0).getSeatRow();
int index = 0;
return null;
}
private List<Seat> sortSeats(List<Seat> seats) {
seats.sort((seat1, seat2) -> {
// row 비교
int rowComparison = seat1.compareRow(seat2);
if (rowComparison != 0) {
return rowComparison;
}
// col 비교
return seat1.compareCol(seat2);
});
return seats;
}
*/
}
후
@Component
@RequiredArgsConstructor
public class TheaterMapperImpl implements TheaterMapper {
private final SeatMapper seatMapper;
@Override
public Theater toDomain(TheaterEntity theaterEntity) {
List<Seat> seats = convertSeatEntitiesToSeats(theaterEntity.getSeats());
return Theater.of(theaterEntity.getTheaterId(), theaterEntity.getTheaterName(), seats);
}
public List<Seat> convertSeatEntitiesToSeats(List<SeatEntity> seatEntities) {
return seatEntities.stream()
.map(seatMapper::toDomain)
.toList();
}
}
상영 중인 영화 조회 로직 리팩토링
이전에 하나의 메서드에서 진행되던 로직들을 메서드로 분리를 해보았다.
stream을 사용하니 복잡한 로직들도 간단하게 처리가 되는 것 같다.
또 1주차 구현 때에는 구현하지 못했던 정렬 로직도 추가해서 구현하였다.
그리고 Dto 변환 로직도 Dto 내부로 들어가니 책임이 잘 분리되고 코드도 보기 좋아진 것 같다.
전
@Service
@RequiredArgsConstructor
public class MovieService {
private final MovieAdapter movieAdapter;
private final ScreeningAdapter screeningAdapter;
public List<NowPlayMovieDto> getNowPlayingMovies() {
List<Movie> movies = movieAdapter.findMovies();
List<Movie> nowPlayingMovies = findNowPlayingMovies(movies);
// Movie에는 id가 없고 MovieEntity에는 id가 있으니 Movie -> MovieEntity로 할 수 없음
// 이렇게 도메인 객체에 id가 없는 것이 맞는가?
// id가 없으니 관련된 Screening을 찾으려고 할 때 이름을 또 추출하고 그 이름으로 찾고 해야 함
List<NowPlayMovieDto> nowPlayMovieDtos = makeNowPlayingMoviesInfo(nowPlayingMovies);
return nowPlayMovieDtos;
}
public List<Movie> findNowPlayingMovies(List<Movie> movies) {
LocalDateTime today = LocalDateTime.now();
return movies.stream()
.filter(movie -> movie.isReleasedBefore(today))
.sorted(Movie::compareReleaseDate)
.collect(Collectors.toList());
}
public List<NowPlayMovieDto> makeNowPlayingMoviesInfo(List<Movie> movies) {
List<NowPlayMovieDto> nowPlayMovieDtos = new ArrayList<>();
for (Movie movie : movies) {
// 영화 이름이 같을 수도 있지 않나...?
List<Screening> movieAllScreening = screeningAdapter.findScreeningsByMovieName(movie.getMovieName());
Map<String, List<Screening>> cinemaNameScreening = movieAllScreening.stream()
.collect(Collectors.groupingBy(Screening::getCinemaName));
for (Map.Entry<String, List<Screening>> entry : cinemaNameScreening.entrySet()) {
String cinemaName = entry.getKey();
List<Screening> screenings = entry.getValue();
NowPlayMovieDto nowPlayMovieDto = makeNowPlayMovieDto(movie, cinemaName, screenings);
nowPlayMovieDtos.add(nowPlayMovieDto);
}
}
return nowPlayMovieDtos;
}
public NowPlayMovieDto makeNowPlayMovieDto(Movie movie, String cinemaName, List<Screening> screenings) {
return NowPlayMovieDto.builder()
.movieName(movie.getMovieName())
.movieRate(movie.getMovieRate().getMovieRateDescription())
.movieReleaseDate(movie.getMovieReleaseDate())
.movieThumbnailImage(movie.getMovieThumbnailImage())
.movieRunningTime(movie.getMovieRunningTime())
.movieGenre(movie.getMovieGenre().getMovieGenreDescription())
.cinemaName(cinemaName)
.screenings(screenings)
.build();
}
}
후
@Service
@RequiredArgsConstructor
public class MovieService {
private final MovieAdapter movieAdapter;
private final ScreeningAdapter screeningAdapter;
public List<NowPlayMovieDto> getNowPlayingMovies() {
List<Movie> movies = movieAdapter.findMovies();
List<Movie> nowPlayingMovies = findNowPlayingMovies(movies);
return makeNowPlayingMoviesInfo(nowPlayingMovies);
}
private List<Movie> findNowPlayingMovies(List<Movie> movies) {
LocalDate today = LocalDate.now();
return movies.stream()
.filter(movie -> movie.isReleasedBefore(today))
.sorted(Movie::compareReleaseDate)
.collect(Collectors.toList());
}
private List<NowPlayMovieDto> makeNowPlayingMoviesInfo(List<Movie> movies) {
List<NowPlayMovieDto> nowPlayMovieDtos = new ArrayList<>();
for (Movie movie : movies) {
List<Screening> movieAllScreening = screeningAdapter.findScreeningsByMovie(movie);
Map<String, List<Screening>> cinemaNameScreening = mapByCinemaName(movieAllScreening);
createNowPlayMovieDtoByMap(movie, cinemaNameScreening, nowPlayMovieDtos);
}
nowPlayMovieDtos.sort(Comparator.comparing(NowPlayMovieDto::getMovieReleaseDate).reversed());
return nowPlayMovieDtos;
}
private Map<String, List<Screening>> mapByCinemaName(List<Screening> movieAllScreening) {
return movieAllScreening.stream()
.collect(Collectors.groupingBy(Screening::fetchTheaterAndCinemaName));
}
private void createNowPlayMovieDtoByMap(Movie movie, Map<String, List<Screening>> cinemaNameScreening,
List<NowPlayMovieDto> nowPlayMovieDtos) {
for (Map.Entry<String, List<Screening>> entry : cinemaNameScreening.entrySet()) {
NowPlayMovieDto nowPlayMovieDto = createNowPlayMovieDtoByEntry(movie, entry);
nowPlayMovieDtos.add(nowPlayMovieDto);
}
}
private NowPlayMovieDto createNowPlayMovieDtoByEntry(Movie movie, Entry<String, List<Screening>> entry) {
String theaterAndCinemaName = entry.getKey();
List<Screening> screenings = entry.getValue();
List<ScreeningTimeDto> screeningTimeDtos = makeScreeningTimeDtos(screenings);
return NowPlayMovieDto.of(movie, theaterAndCinemaName, screeningTimeDtos);
}
private List<ScreeningTimeDto> makeScreeningTimeDtos(List<Screening> screenings) {
List<ScreeningTimeDto> screeningTimeDtos
= new ArrayList<>(screenings.stream().map(ScreeningTimeDto::of).toList());
screeningTimeDtos.sort(Comparator.comparing(ScreeningTimeDto::getStartTime));
return screeningTimeDtos;
}
}
가독성을 좀 높히기 위해 여러 stream 로직들을 메서드로 분리해봤다.
또 너무 메서드로 나눠져서 복잡한 감도 있는 것 같다.
그리고 메서드명들이 좀 더 잘 읽히게 하고 싶은데 어려운 것 같다.
기타
- 1주차 구현 때 하지 못했던 설정 파일 (docker-compose, ddl.sql, data.sql, .http) 등을 추가하였다.
- 클래스, 필드명에 prefix를 제거했다.
회고
1주차 때 처음이고 너무 정신이 없어서 그런지 코드를 너무 막 작성했던 것 같다.
그리고 시간도 부족하여 많은 요구사항을 충족하지 못했다.
그래도 리뷰를 받고 리팩토링을 하고 부족한 요구사항들을 모두 만족시켜 만족스러운 리팩토링이였다.
특히 id 필드에 대한 고민과 상영관-좌석 생성 및 entity to domain 변환 부분에 대해서 잘 해결한 것 같다.
추후에는 코드를 짤 때 리뷰 받은 것들과 이번에 고민하고 생각했던 것들을 잘 생각하고 녹여서 코드를 짜야겠다.