feat: 추억 목록 정렬, 필터링 API 구현 #546 #583
Conversation
🌻Test Coverage Report
|
| public class MemoryMembers { | ||
| private final List<MemoryMember> memoryMembers; | ||
|
|
||
| public List<Memory> operate(List<String> filters, String sort) { |
There was a problem hiding this comment.
필터링과 정렬 연산을 수행하는 역할을 합니다. 도메인일까요, 서비스일까요?
There was a problem hiding this comment.
음 명확하게 구분하는 것이 애매할 수 있지만 지금 구성에서는 MemoryFilter와 MemroySort를 통해 결과를 반환하는 역할이기에 stream과 굉장히 유사한 역할을 하고있다고 생각합니다 따라서 도메인보다는 서비스의 성격에 가깝지 않나라는 생각을 했습니다
There was a problem hiding this comment.
계속 네이밍이 아쉽다는 생각이 드네요🤔
좋은 네이밍 추천 받아요 ㅎㅎ
There was a problem hiding this comment.
지금은 List<Memory> memories를 관리하고 있으며 Memories가 되었네요.
도메인의 리스트를 일급 컬렉션으로 묶는 경우는 리스트의 연산을 처리해야 하는 경우가 많아질 때라고 생각합니다. 하지만 막상 memories.stream()....과 같이 연산을 묶어 수행하는 메서드가 보이질 않아서 도메인으로 보기는 힘들다고 생각합니다. MemoryFilter의 apply가 non-static이 된다면 그런 로직이 생길 수도 있겠지만, Memories를 사용하지 않고 List<Memory> 형태로 타입을 다루는 코드가 많기 때문에, 여전히 일급 컬렉션으로 보기에는 부족하다는 생각이 듭니다.
MemoryMember를 Memories로 만드는건 그냥 MemoryService에서 하게 하고, 아예 operate 연산을 수행하는 클래스로 만드는 것에 대해서는 어떻게 생각하시나요? 아래처럼 구현해봤습니다.
// MemoryOperator.java
public class MemoryOperator {
private final List<MemoryFilter> memoryFilters;
private final MemorySort memorySort;
public MemoryOperator() {
this.memoryFilters = List.of();
this.memorySort = MemorySort.UPDATED;
}
public MemoryOperator(List<MemoryFilter> memoryFilters, MemorySort memorySort) {
this.memoryFilters = memoryFilters;
this.memorySort = memorySort;
}
public List<Memory> operate(List<Memory> memories) {
List<Memory> filteredMemories = MemoryFilter.apply(memoryFilters, memories);
return memorySort.apply(filteredMemories);
}
}// MemoryService.java
public class MemoryService {
public MemoryResponses readAllMemories(Member member, MemoryReadRequest memoryReadRequest) {
List<Memory> memories = toMemories(memoryMemberRepository.findAllByMemberId(member.getId()));
MemoryOperator memoryOperator = new MemoryOperator(memoryReadRequest.getFilters(), memoryReadRequest.getSort());
List<Memory> filteredMemories = memoryOperator.operate(memories);
return MemoryResponses.from(filteredMemories);
}
public MemoryNameResponses readAllMemoriesByDate(Member member, LocalDate currentDate) {
List<Memory> memories = toMemories(memoryMemberRepository.findAllByMemberIdAndDate(member.getId(), currentDate));
MemoryOperator memoryOperator = new MemoryOperator();
List<Memory> filteredMemories = memoryOperator.operate(memories);
return MemoryNameResponses.from(filteredMemories);
}
private List<Memory> toMemories(List<MemoryMember> memoryMembers) {
return memoryMembers.stream()
.map(MemoryMember::getMemory)
.collect(Collectors.toList());
}
}There was a problem hiding this comment.
Memories를 Service 내부에서 생성하도록 하는 것도 좋은 방법이라고 생각해요!
하지만, MemoryOperator라는 클래스는 객체라기보다는 유틸성에 가깝다는 생각이 들어요.
MemoryOperator가 MemorySort와 MemoryFilter를 상태값으로 들고 있는 객체로 만들었을 때의 이점이 와닿지 않는 것 같아요.
현재의 MemoryOperator는 내부적으로 결국 MemoryFilter와 MemorySort의 메서드를 호출할 뿐이니까요!
두 개의 필드를 상태로서 가지고 어떤 행위를 수행한다고 보기 힘들 것 같아요!
차라리, Service에서 Filter 메서드와 Sort 메서드를 직접 호출하는 편이 가독성 측면에서도 좋을 것 같은데 어떻게 생각하시나요?
public MemoryResponses readAllMemories(Member member, MemoryReadRequest memoryReadRequest) {
List<Memory> memories = toMemories(memoryMemberRepository.findAllByMemberId(member.getId()));
List<Memory> filteredMemories = filterAndSortMemories(memories, memoryReadRequest.getFilters(), memoryReadRequest.getSort());
return MemoryResponses.from(filteredMemories);
}
private List<Memory> filterAndSortMemories(List<Memory> memories, List<MemoryFilter> filters, MemorySort sort) {
List<Memory> filteredMemories = MemoryFilter.apply(filters, memories);
return sort.apply(filteredMemories);
}There was a problem hiding this comment.
수행하는 로직이 결국에는 하나라서 객체로 만들었을 때의 큰 이점이 있지는 않네요!
확실히 리니가 올려주신 코드처럼 Service에서 하나의 메서드로 관리하는 편이 더 좋아보입니다.
There was a problem hiding this comment.
아무런 필터 정렬을 집어넣지 않는 경우는 아래와 같은 형식이 되겠네요.
List<Memory> filteredMemories = filterAndSortMemories(List.of(), MemorySort.UPDATED);Memories에서 했던 것처럼 MemoryService에서 해당 값들을 상수로 가지면 뭔가 애매한 느낌이 들어서 더 생각해봤는데,
Memory filtering&sorting의 역할을 담당하고 있는 MemoryService가 그 기능의 상수를 갖고 있다고 생각하면 어색하지 않은 것 같습니다.
결론적으로, 상수로 정의하든 그렇지 않든 큰 상관 없는 것 같아요.
private static final List<MemoryFilter> DEFAULT_MEMORY_FILTER = List.of();
private static final MemorySort DEFAULT_MEMORY_SORT = MemorySort.UPDATED;혹시나 정렬&필터링 기능이 지금보다 더 무거워진다면, 그때 클래스 분리를 생각해도 좋을 것 같습니다!
There was a problem hiding this comment.
클래스가 뚱뚱해지고 있네요 😂 후에 더 뚱뚱해지면 리팩터링을 고려해보시죠!
| sortByCreatedAtDescending(memoryMembers); | ||
| public MemoryNameResponses readAllMemoriesByDate(Member member, LocalDate currentDate) { | ||
| MemoryMembers memoryMembers = new MemoryMembers(memoryMemberRepository.findAllByMemberIdAndDate(member.getId(), currentDate)); | ||
| List<Memory> memories = memoryMembers.operate(); |
There was a problem hiding this comment.
default용 operate() 메서드를 오버로딩했습니다. 내부적으로 operate(Filter, Sort)를 호출합니다.
There was a problem hiding this comment.
위의 코멘트와 유사하게 만약 현재의 MemoryMembers가 Memories로 변화하면서 필드로 List<Memory>를 가지게 된다면 해당 메서드는 없어질 수 있을 것 같아요!
There was a problem hiding this comment.
Memories로 리팩터링 했습니다.
다만 해당 메서드는 아무런 정렬,필터링 값을 사용하지 않는 경우를 위해 오버로드 한 메서드였기 때문에 제거할 수 없는 부분이라고 생각합니다. 호티는 어떻게 생각하시나요?
There was a problem hiding this comment.
이렇게 한다면, 생성자 오버로딩의 개념으로도 볼 수 있을 것 같습니다.
There was a problem hiding this comment.
최신 수정순 정렬이 기본으로 제공되어야 하기 때문에... 동의합니다
| @Schema(hidden = true) | ||
| public List<String> getFilters() { | ||
| List<String> filters = new ArrayList<>(); | ||
| if (isActive(term)) { |
There was a problem hiding this comment.
아주........마음에 안듭니다...... 이상태로는 필터링 조건이 늘어날 때마다 분기가 늘어날텐데 분기 처리를 줄일 수 있는 방법이 떠오르지 않아요..
There was a problem hiding this comment.
위의 코멘트에 달아놓은 것 처럼 List<Enum>으로 받는다면 해결 가능할 것 같네요
There was a problem hiding this comment.
해당 방식은 기존에 의논했던 uri에서는 불가능 할 것 같아요 ㅜㅜ
저번 회의에서 말씀 드렸듯 다른 uri 형식으로 가져갔을 때에 대한 코드를 함께 코멘트로 남겨놓았습니다!
| @@ -42,18 +39,18 @@ private boolean isOnlyOneDatePresent(LocalDate startAt, LocalDate endAt) { | |||
| } | |||
|
|
|||
| private boolean isInvalidTerm(LocalDate startAt, LocalDate endAt) { | |||
| return isExist(startAt, endAt) && endAt.isBefore(startAt); | |||
| return Objects.nonNull(startAt) && Objects.nonNull(endAt) && endAt.isBefore(startAt); | |||
There was a problem hiding this comment.
밑에서 사용하는 isExist()를 사용하지 않으신 이유가 있을까요?
There was a problem hiding this comment.
아래 isExist() 메서드는 인스턴스의 속성 값에 대한 메서드입니다! 현재는 속성 값이 아닌 외부에서 들어온 값에 대한 검증이기 때문에 이 메서드에서는 재사용할 수 없어요!
private으로 메서드 오버로드 하는 방향으로 리팩터링 했습니다!
| private List<Memory> getMemories() { | ||
| return memoryMembers.stream() | ||
| .map(MemoryMember::getMemory) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
불변한다면 toList()를 사용하는게 더 나을 것 같아요!
There was a problem hiding this comment.
필터링과 정렬 작업으로 인해 unmodifiable하지 않은 컬렉션이 필요했어요!
| @Schema(description = "추억 목록 조회시 정렬과 필터링 조건을 위한 요청 형식입니다.") | ||
| public record MemoryReadRequest( | ||
| @Schema(description = "기간 필터 사용 여부 (대소문자 구분 X)", example = "true") | ||
| String term, |
There was a problem hiding this comment.
기간 필터의 사용 유무라면 boolean으로 표현하는 방법도 있을 것 같은데 String으로 표현하신 이유가 있을가용?
There was a problem hiding this comment.
우선은 추후 필터링 조건이 늘어날 수 있기에 String으로 표현하신걸로 알고 넘어갈게요!
There was a problem hiding this comment.
필터링 조건이 중복되어 있을 수 있기에 처음 받을때 List<Enum>으로 받는다면 더 괜찮을 것 같아요!
public static class MemoryReadRequest {
private List<Status> statuses;
// Status 는 Enum
GET /example?statuses=ACTIVE&statuses=INACTIVE&statuses=PENDINGThere was a problem hiding this comment.
기간 필터의 사용 유무라면 boolean으로 표현하는 방법도 있을 것 같은데 String으로 표현하신 이유가 있을가용?
boolean -> null 값 수용 불가
Boolean -> null 값 수용 가능 but 그 외의 문자열(true/false)에 대해서는 400 에러 발생
잘못된 문자열이 들어왔을 때에는 예외 처리보다는 필터링이 활성화되지 않도록 구현하고 싶었어요! 따라서 위의 문제들로 인해 String 값을 받는 방식으로 했습니다. 하지만, 저도 코드는 마음에 들지 않네요😂
회의에서 말씀드렸듯 다른 uri 방식을 채택했을 때의 코드도 코멘트로 남겼습니다!
| List<MemoryMember> memoryMembers = memoryMemberRepository.findAllByMemberId(member.getId()); | ||
| sortByCreatedAtDescending(memoryMembers); | ||
| public MemoryResponses readAllMemories(Member member, MemoryReadRequest memoryReadRequest) { | ||
| MemoryMembers memoryMembers = new MemoryMembers(memoryMemberRepository.findAllByMemberId(member.getId())); |
There was a problem hiding this comment.
현재 인자로 MemoryMember의 List 형을 받지만 내부적으로는 Memory 의 List만을 사용하는 것으로 보입니다.
클래스 명을 Memories로 변경하고 필드로 List<Memory>만 받는 것으로 바꾸는 건 어떨까요?
(memoryMembers.operate 가 조금은 어색하게 다가왔습니다)
There was a problem hiding this comment.
부생성자 만드는 방식으로 하고 싶었지만, List형 인자이다보니 오버로드가 안되어서 정팩메로 만들었습니다!
| memoryList.stream() | ||
| .filter(Memory::hasTerm) | ||
| .collect(Collectors.toList()) |
There was a problem hiding this comment.
enum 내부에 함수형 인터페이스를 구현하는 것과 외부의 메서드로 메서드를 구성해놓은 것 중 함수형 인터페이스를 내부에 구현하는 방법을 선택하신 이유가 궁금합니다!
There was a problem hiding this comment.
각 상수(RECENTLY_UPDATED, NEWEST, OLDEST)가 각각의 정렬 방식에 대한 로직을 포함하므로, 어떤 정렬 방식이 사용되는지를 명확하게 알 수 있습니다. 새로운 정렬 조건이 추가되어도, 해당 클래스에 enum만 추가하면 되기 때문에 유지보수 측면에서도 더 편리하다고 판단했습니다.
| sortByCreatedAtDescending(memoryMembers); | ||
| public MemoryNameResponses readAllMemoriesByDate(Member member, LocalDate currentDate) { | ||
| MemoryMembers memoryMembers = new MemoryMembers(memoryMemberRepository.findAllByMemberIdAndDate(member.getId(), currentDate)); | ||
| List<Memory> memories = memoryMembers.operate(); |
There was a problem hiding this comment.
위의 코멘트와 유사하게 만약 현재의 MemoryMembers가 Memories로 변화하면서 필드로 List<Memory>를 가지게 된다면 해당 메서드는 없어질 수 있을 것 같아요!
| private final Function<List<Memory>, List<Memory>> operation; | ||
|
|
||
| public static List<Memory> apply(String sortValue, List<Memory> memories) { | ||
| return Stream.of(MemorySort.values()) |
| @Schema(hidden = true) | ||
| public List<String> getFilters() { | ||
| List<String> filters = new ArrayList<>(); | ||
| if (isActive(term)) { |
There was a problem hiding this comment.
위의 코멘트에 달아놓은 것 처럼 List<Enum>으로 받는다면 해결 가능할 것 같네요
| @Setter | ||
| public abstract class BaseEntity { | ||
| @CreatedDate | ||
| private LocalDateTime createdAt; |
There was a problem hiding this comment.
createdAt까지 setter의 영향을 받을 수 있다 생각되어 새로운 updatedAt 수정 메서드만 구성하는게 좋을 것 같아요!
There was a problem hiding this comment.
좋은 것 같아요! 반영했습니다:)
| @@ -25,6 +27,7 @@ | |||
|
|
|||
| @Entity | |||
| @Getter | |||
| @EntityListeners({MomentEntityListener.class}) | |||
There was a problem hiding this comment.
해당 Moment 내부에서 메서드를 구현한 후 EntityListener를 구성하는 방법도 있었는데 새롭게 클래스를 만드신 이유가 궁금해요!
There was a problem hiding this comment.
명시적으로 엔티티 생명주기 이벤트와 관련하여 특정 작업을 수행함을 나타내고 싶었어요. 하지만, MomentEntityListener는 여러 엔티티에서 공용으로 사용되는 로직이 아니기 때문에, 어떤 코드 재사용성 증가의 이점을 누릴 수는 없을 것 같네요!
특정 엔티티에 특화된 로직이기 때문에 굳이 클래스를 분리할 필요 없이 내부에서 콜백 메서드를 등록해도 될 것 같아요. 수정했습니다👊
There was a problem hiding this comment.
/memories?term=true&sort=updated와 /memories?filters=term&sort=updated 2가지 방식의 코드가 공존합니다. 따라서, 다음 링크를 통해서 코드 리뷰 부탁드립니다.
/memories?term=true&sort=updated 방식
/memories?filters=term&sort=updated 방식
어떤 방식이 유지보수, 확장, !가독성! 측면에서 좋은 것 같은지 의견 부탁드립니다:)
| @@ -25,6 +27,7 @@ | |||
|
|
|||
| @Entity | |||
| @Getter | |||
| @EntityListeners({MomentEntityListener.class}) | |||
There was a problem hiding this comment.
명시적으로 엔티티 생명주기 이벤트와 관련하여 특정 작업을 수행함을 나타내고 싶었어요. 하지만, MomentEntityListener는 여러 엔티티에서 공용으로 사용되는 로직이 아니기 때문에, 어떤 코드 재사용성 증가의 이점을 누릴 수는 없을 것 같네요!
특정 엔티티에 특화된 로직이기 때문에 굳이 클래스를 분리할 필요 없이 내부에서 콜백 메서드를 등록해도 될 것 같아요. 수정했습니다👊
| public class MemoryMembers { | ||
| private final List<MemoryMember> memoryMembers; | ||
|
|
||
| public List<Memory> operate(List<String> filters, String sort) { |
There was a problem hiding this comment.
계속 네이밍이 아쉽다는 생각이 드네요🤔
좋은 네이밍 추천 받아요 ㅎㅎ
| sortByCreatedAtDescending(memoryMembers); | ||
| public MemoryNameResponses readAllMemoriesByDate(Member member, LocalDate currentDate) { | ||
| MemoryMembers memoryMembers = new MemoryMembers(memoryMemberRepository.findAllByMemberIdAndDate(member.getId(), currentDate)); | ||
| List<Memory> memories = memoryMembers.operate(); |
There was a problem hiding this comment.
Memories로 리팩터링 했습니다.
다만 해당 메서드는 아무런 정렬,필터링 값을 사용하지 않는 경우를 위해 오버로드 한 메서드였기 때문에 제거할 수 없는 부분이라고 생각합니다. 호티는 어떻게 생각하시나요?
| @Schema(hidden = true) | ||
| public List<String> getFilters() { | ||
| List<String> filters = new ArrayList<>(); | ||
| if (isActive(term)) { |
There was a problem hiding this comment.
해당 방식은 기존에 의논했던 uri에서는 불가능 할 것 같아요 ㅜㅜ
저번 회의에서 말씀 드렸듯 다른 uri 형식으로 가져갔을 때에 대한 코드를 함께 코멘트로 남겨놓았습니다!
| @@ -42,18 +39,18 @@ private boolean isOnlyOneDatePresent(LocalDate startAt, LocalDate endAt) { | |||
| } | |||
|
|
|||
| private boolean isInvalidTerm(LocalDate startAt, LocalDate endAt) { | |||
| return isExist(startAt, endAt) && endAt.isBefore(startAt); | |||
| return Objects.nonNull(startAt) && Objects.nonNull(endAt) && endAt.isBefore(startAt); | |||
There was a problem hiding this comment.
아래 isExist() 메서드는 인스턴스의 속성 값에 대한 메서드입니다! 현재는 속성 값이 아닌 외부에서 들어온 값에 대한 검증이기 때문에 이 메서드에서는 재사용할 수 없어요!
private으로 메서드 오버로드 하는 방향으로 리팩터링 했습니다!
| List<MemoryMember> memoryMembers = memoryMemberRepository.findAllByMemberId(member.getId()); | ||
| sortByCreatedAtDescending(memoryMembers); | ||
| public MemoryResponses readAllMemories(Member member, MemoryReadRequest memoryReadRequest) { | ||
| MemoryMembers memoryMembers = new MemoryMembers(memoryMemberRepository.findAllByMemberId(member.getId())); |
| List<MemoryMember> memoryMembers = memoryMemberRepository.findAllByMemberId(member.getId()); | ||
| sortByCreatedAtDescending(memoryMembers); | ||
| public MemoryResponses readAllMemories(Member member, MemoryReadRequest memoryReadRequest) { | ||
| MemoryMembers memoryMembers = new MemoryMembers(memoryMemberRepository.findAllByMemberId(member.getId())); |
There was a problem hiding this comment.
부생성자 만드는 방식으로 하고 싶었지만, List형 인자이다보니 오버로드가 안되어서 정팩메로 만들었습니다!
| @Setter | ||
| public abstract class BaseEntity { | ||
| @CreatedDate | ||
| private LocalDateTime createdAt; |
There was a problem hiding this comment.
좋은 것 같아요! 반영했습니다:)
| memoryList.stream() | ||
| .filter(Memory::hasTerm) | ||
| .collect(Collectors.toList()) |
There was a problem hiding this comment.
각 상수(RECENTLY_UPDATED, NEWEST, OLDEST)가 각각의 정렬 방식에 대한 로직을 포함하므로, 어떤 정렬 방식이 사용되는지를 명확하게 알 수 있습니다. 새로운 정렬 조건이 추가되어도, 해당 클래스에 enum만 추가하면 되기 때문에 유지보수 측면에서도 더 편리하다고 판단했습니다.
| public static List<Memory> apply(List<String> filters, List<Memory> memories) { | ||
| List<MemoryFilter> applicableFilters = Stream.of(MemoryFilter.values()) | ||
| .filter(filter -> filters.contains(filter.name)) | ||
| .toList(); | ||
|
|
||
| List<Memory> filteredMemories = memories; | ||
| for (MemoryFilter filter : applicableFilters) { | ||
| filteredMemories = filter.operation.apply(filteredMemories); | ||
| } | ||
| return filteredMemories; | ||
| } |
There was a problem hiding this comment.
리니가 의도하신 부분은 Filter와 관련된 부분이 현재는 기간이 있는 경우 없는 경우 하나이지만 추가적으로 다른 요구사항들이 생길 가능성이 있기에 MemoryFilter를 구성하신것 같습니다. 맞을까요?
맞습니다.
지금 잘 구성해 주셨으니 이대로 유지는 하되 차라리 Filter의 종류를 의미하는 Enum을 새롭게 구성해 해당 Enum에게 특정 역할을 부여해 해당 메서드를 조금은 단순하게 풀어갈 수도 있을 것 같아요!
동의합니다!!
추가로 26번째 줄 부터 30번째 줄까지의 코드에서 filter적용이 되지 않은 memories를 새로운 filteredMemories 와 같은 메모리 주소를 공유하게 설정한 다음, 적용가능한 filter를 순회하면서 해당 filteredMemories 를 반환하는 형태로 구성한 부분에서 26번째에 같은 메모리 주소를 공유하게 설정하신 이유가 궁금해요!
코드가 복잡한 것 같아, 직관적으로 필터링 작업을 나타내고자 변수를 분리했어요. 딱히 다른 이유는 없었습니다! 오히려 가독성이 떨어진다면 제거하는 편이 나을 것 같네요😂
| @MockBean | ||
| private AuthService authService; | ||
|
|
||
| class MomentControllerTest extends ControllerTest { |
There was a problem hiding this comment.
컨텍스트 재사용 구조를 만들어서 코드 중복성을 감소시켰네요! 훨씬 깔끔하고 좋아보입니다.
도메인 변경하면서 새로 만든 CategoryControllerTest와 StaccatoControllerTest에도 반영하도록 하겠습니다.
| public static Memory create(LocalDate startAt, LocalDate endAt) { | ||
| return Memory.builder() | ||
| .thumbnailUrl("https://example.com/memorys/geumohrm.jpg") | ||
| .title("2024 여름 휴가") | ||
| .description("친구들과 함께한 여름 휴가 추억") | ||
| .startAt(startAt) | ||
| .endAt(endAt) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
도메인 변경하면서 예시들을 변경해주려면 이런 Fixture 들도 전부 수정해야 하더라구요. 충돌을 최소화하기 위해 원래 코드를 아예 안건드리는 방향으로 구현했기 때문에, '추억'이라는 표현이 있어도 클라이언트에 주는 직접적인 영향이 없는 부분은 그대로 놔뒀었습니다.
리니 코드가 머지되고 난 후에 전반적으로 Fixture에 있는 네이밍까지 수정하겠습니다.
There was a problem hiding this comment.
넵-! 현재 작업하던 부분 외에 나머지는 다음주 오프 만남 때 같이 하시죠👊
| assertThat(result).isTrue(); | ||
| } | ||
|
|
||
| @DisplayName("추억이 기간을 가지고 있으면 참을 반환한다.") |
There was a problem hiding this comment.
사소사소한 오타~
| @DisplayName("추억이 기간을 가지고 있으면 참을 반환한다.") | |
| @DisplayName("추억이 기간을 가지고 있지 않으면 거짓을 반환한다.") |
| LocalDateTime beforeUpdate = memory.getUpdatedAt(); | ||
|
|
||
| // when | ||
| moment.changeFeeling(Feeling.ANGRY); |
There was a problem hiding this comment.
@PostMapping("/{momentId}/feeling")->moment.changeFeeling사용@PutMapping(path = "/{momentId}")->moment.update()사용
스타카토 수정 API에서 사용하는 메서드는 moment.update()이므로, 이 메서드를 사용해서도 테스트를 해보았는데 잘 수행되는 것을 확인할 수 있었습니다.
@DisplayName("Moment 수정 시 Memory의 updatedAt이 갱신된다.")
@Test
void updateMemoryUpdatedDateWhenMomentUpdated() {
// given
Member member = memberRepository.save(MemberFixture.create());
Memory memory = memoryRepository.save(MemoryFixture.createWithMember(member));
Moment moment = momentRepository.save(MomentFixture.create(memory));
Moment updatedMoment = MomentFixture.create(memory, LocalDateTime.of(2024, 7, 1, 10, 1));
LocalDateTime beforeUpdate = memory.getUpdatedAt();
// when
moment.update(updatedMoment);
entityManager.flush();
entityManager.refresh(memory);
LocalDateTime afterUpdate = memory.getUpdatedAt();
// then
assertThat(afterUpdate).isAfter(beforeUpdate);
}There was a problem hiding this comment.
Moment에 수정 사항이 발생하기만 하면 된다고 생각하여 간단한 메서드로 테스트를 하고자 했습니다! (기분 업데이트 API도 Moment에 수정사항을 만드는 API이니까요~😁)
| public List<Memory> operate(List<MemoryFilter> filters, MemorySort sort) { | ||
| List<Memory> filteredMemories = MemoryFilter.apply(filters, memories); | ||
| return sort.apply(filteredMemories); | ||
| } |
There was a problem hiding this comment.
MemoryFilter의 apply는 static으로, MemorySort의 apply는 non-static으로 관리하는 이유가 있으신가요?
There was a problem hiding this comment.
MemoryFilter는 중복 필터링 적용으로, 하나의 인스턴스에 대해 apply를 수행하는 것이 아닌 List<MemoryFilter>의 모든 필터링에 대한 apply를 호출하여 클래스 메서드로 생성하였습니다.
반면, 정렬 조건은 중복 적용이 아니기 때문에 인스턴스 메서드로 호출하도록 구현했어요!
만약, 가독성 측면에서 통일시키는게 좋을 것 같다고 하면, 정적 메서드 방식으로 통일 시키는 방향으로 리팩터링 하겠습니다.
폭포는 어떤 것이 더 좋다고 생각하세요?
There was a problem hiding this comment.
처음에 operate 메서드를 보았을 때, 비슷한 기능을 하는 클래스임에도 불구하고 적용방식이 달라서 코드를 이해하는데 약간의 시간이 걸렸습니다. 통일시키면 좋을 것 같다는 생각이 들지만, 그렇다면 non-static으로 통일시켜야할지 static으로 통일시켜야할지에 대해서는 아직 명확한 이유가 생각나지 않네요.
There was a problem hiding this comment.
만약, 통일시킨다면 저는 객체에게 메세지를 전달하는 방식으로 통일시키는게 더 좋을 것 같아요.
MemoryFilter.apply 내부에서 filters의 operation을 모두 적용시키는 대신, 외부에서 for문을 돌면서 인스턴스 메서드를 호출하는 편이 좋을 것 같아서 리팩터링 해보았습니다.
|
이 부분 코멘트 한번 확인부탁드립니다! |
api uri만 생각했을 때는
바뀐 방식 |
| import com.staccato.memory.domain.Memory; | ||
| import lombok.RequiredArgsConstructor; | ||
|
|
||
| @RequiredArgsConstructor |
There was a problem hiding this comment.
enum에 @RequiredArgsConstructor 쓸 생각은 못해봤는데 👍
| @@ -7,6 +7,6 @@ | |||
|
|
|||
| @ExtendWith(DatabaseCleanerExtension.class) | |||
| @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.NONE) | |||
| @Import({TestConfig.class}) | |||
| @Import({ServiceTestConfig.class}) | |||
| @WebMvcTest | ||
| public abstract class ControllerTest { | ||
| @Autowired | ||
| protected MockMvc mockMvc; | ||
| @Autowired | ||
| protected ObjectMapper objectMapper; | ||
| @MockBean | ||
| protected MemberService memberService; | ||
| @MockBean | ||
| protected MemoryService memoryService; | ||
| @MockBean | ||
| protected MomentService momentService; | ||
| @MockBean | ||
| protected ImageService imageService; | ||
| @MockBean | ||
| protected AuthService authService; | ||
| @MockBean | ||
| protected CommentService commentService; | ||
| } |
There was a problem hiding this comment.
ControllerTest에서 특정 MockBean이 필요하지 않은 경우에도 MockBean으로 등록하는 것으로 보이는데, 이 부분은 사소할까요! 잘 몰라서 질문드립니다 🤔
There was a problem hiding this comment.
맞아요! 사용하지 않는 경우에도 MockBean으로 등록됩니다. 대신, Context를 재사용할 수 있게 됩니다!
@MockBean이나 SpyBean 등의 어노테이션을 사용하면 대리 객체가 생성되어 Context가 오렴되었다고 판단하기 때문에 Controller 테스트마다 Context가 새롭게 띄워집니다.
따라서, 처음부터 필요한 대리 객체가 무엇인지 파악하고 Mock, Spy 한번에 선언하여 Context 에 올려두어 Test Class 들이 상속 받아 재사용하도록 구현하여 컨텍스트 초기화를 한번만 하도록 하고자 위와 같이 구성하게 되었습니다.
| sortByCreatedAtDescending(memoryMembers); | ||
| public MemoryNameResponses readAllMemoriesByDate(Member member, LocalDate currentDate) { | ||
| MemoryMembers memoryMembers = new MemoryMembers(memoryMemberRepository.findAllByMemberIdAndDate(member.getId(), currentDate)); | ||
| List<Memory> memories = memoryMembers.operate(); |
There was a problem hiding this comment.
최신 수정순 정렬이 기본으로 제공되어야 하기 때문에... 동의합니다
땅땅땅!
맞아요.! 우선은 추억의 개수는 스타카토와 달리 많아지는 부분이 아니라고 생각하여 애플리케이션 레벨에서 해결했습니다! |
⭐️ Issue Number
🚩 Summary
/memories?term=true&sort=NEWEST와 같은 형식으로 필터링과 정렬 조건을 query string으로 전달하여 적용합니다.🛠️ Technical Concerns
MomentEntityListener를 만들고Moment엔티티에 등록하였습니다. 해당 리스너에서는 Moment에 대한 Persist, Remove, Update 작업이 발생하였을 때 Memory의 UpdatedAt을 갱신합니다. 이때 트랜잭션 내부에서 작업하도록 PreXXX 콜백을 사용하였습니다.🙂 To Reviewer
추가로, 갑자기-> 해결: 의존성 문제였슴다momentRepositoryTest.findAllByMemoryIdOrderByCreatedAt()가 터지는데, 왜인지 모르겠습니다. 그 외에도 어떤 생성 날짜/수정 날짜 갱신에 대한 테스트가 랜덤하게 터지고 있습니다.....!주목!
/memories?term=true&sort=updated와/memories?filters=term&sort=updated2가지 방식의 코드가 공존합니다. 따라서, 다음 링크를 통해서 코드 리뷰 부탁드립니다./memories?term=true&sort=updated방식/memories?filters=term&sort=updated방식어떤 방식이 유지보수, 확장, !가독성! 측면에서 좋은 것 같은지 의견 부탁드립니다:)
📋 To Do