쇼핑몰을 토이 프로젝트로 만들고 틈틈히 리팩토링도 병행하는 중이었다.
평소에 되게 신경쓰였던 부분이 있어 리팩토링을 하게 됐는데 그 과정을 기록해보려 한다.
(보는 사람 관점에선 오류 수정일 수 도 있고, 리팩토링 일수도 있고 좀 애매한 부분이라고 생각한다.)
1. 계기
리팩토링을 하게된 계기는 postman을 통해 api 결과를 보다가 마음에 안드는 부분이 있어서 였다.
바로 아래 결과였다.
좌측메뉴 같은 곳에서 카테고리의 전체목록을 조회하기 위해 호출되는 GET API 였다.
카테고리 구성을 1Depth 와 2Depth 만 있다고 정의하고 개발을 진행했는데, 위 결과를 보면 하위 카테고리 정보에도 subCategories 라는 데이터가 비어있는 List로 넘어오는 것을 알 수 있다.
최하위 카테고리는 어차피 하위 카테고리가 없으므로 저 필드 자체를 출력하고 싶지가 않았다.
(별거 아닐 수 있겠지만, 빈 리스트를 가져온다는게 괜히 신경쓰였다.)
2. 리팩토링 대상 확인
가장 먼저 확인해 볼 부분은 API 결과로 return 되는 response 객체였다.
아래와 같이 작성되어 있었다.
@Data
@NoArgsConstructor(access = AccessLevel.PUBLIC)
public class CategoryResponse {
private Long id;
private String name;
private List<CategoryResponse> subCategories = new ArrayList<>();
public CategoryResponse(Long id, String name) {
this.id = id;
this.name = name;
}
public CategoryResponse(Long id, String name, List<CategoryResponse> subCategories) {
this.id = id;
this.name = name;
this.subCategories = subCategories;
}
public static CategoryResponse of(Category category) {
return new CategoryResponse(category.getId(), category.getName(),
category.getSubCategories().stream()
.map(CategoryResponse::of)
.collect(Collectors.toList()));
}
public List<CategoryResponse> getSubCategories() {
return subCategories;
}
}
response 객체를 만드는 방법이 총 3가지나 된다.(이것도 문제라면 문제인거 같다.)
위 3가지 방법 중 어떤 방법으로 response 객체를 생성해서 API 결과로 사용하는지 확인해봐야겠다.
아래 CategoryService 에서 생성하고 있었다.
@Transactional(readOnly = true)
@Service
@RequiredArgsConstructor
public class CategoryService {
private final CategoryQueryRepository categoryQueryRepository;
/* == 중략 == */
public List<CategoryResponse> findCategories() {
List<CategoryQueryDto> categories = categoryQueryRepository.findCategoryAll();
return categories.stream()
.collect(
groupingBy(categoryQueryDto -> new CategoryResponse(categoryQueryDto.getParentId(),
categoryQueryDto.getParentName()),
mapping(categoryQueryDto -> new CategoryResponse(categoryQueryDto.getChildId(),
categoryQueryDto.getChildName()), toList()
)
)).entrySet().stream()
.map(
e -> new CategoryResponse(e.getKey().getId(), e.getKey().getName(), e.getValue())
).collect(toList());
}
}
한 눈에 보이는 코드는 아니지만, 결과적으로 맨 아래에 있는 람다식으로 생성되는 객체가 List로 변환됨을 알았다.
e -> new CategoryResponse(e.getKey().getId(), e.getKey().getName(), e.getValue())
두 가지 수정 지점이 있을 수 있다고 생각했다.
- CategoryResponse 위주로 수정
- CategoryService 도 같이 수정
먼저, CategoryResponse 위주로 수정하게 되면 아래와 같은 절차가 예상되었다.
- CategoryResponse 내 생성자에서 subCategories list의 데이터 유무를 판단
- API 결과로 내보낼 때 JSON 생성 시, null 인 field 는 내보내지 않기
그 외에 CategoryService 를 같이 수정하는 목적은 딱 하나, "findCategories 에서 return 하는 로직이 복잡해서" 이다.
예전에 인프런에서 영한님의 JPA 강의를 듣고, 쿼리 1번 + dto 조회로 성능 최적화를 노리고 작성해본 코드인데,
오랜만에 보고 막상 건드려보려니 나도 정확히 내 소스를 이해하지 못하는 불상사가 발생했다.
그래서 좀 더 쉽고 직관적으로 리팩터링을 할 순 없을까 고민을 해보다가 이왕 해보는거 둘 다 해보자란 마음으로 둘 다 손보기로 했다.
3. 테스트 코드 확인하기
리팩토링을 하기 전에 해당 내용을 검증해 줄 수 있는 테스트코드가 잘 작성되어 있는지 확인이 필요했다.
아래와 같은 테스트코드가 작성되어 있었다.
@Transactional
@SpringBootTest
public class CategoryServiceTest {
@Autowired
CategoryService categoryService;
/* -- 관련없는 테스트 코드 중략 -- */
@Test
@DisplayName("모든 카테고리 조회")
void findCategoryAll(){
// given
CategoryResponse categoryResponse1 = categoryService.saveCategory(new CategoryRequest("상위 카테고리 1", null));
categoryService.saveCategory(new CategoryRequest("하위 카테고리 1-1", categoryResponse1.getId()));
categoryService.saveCategory(new CategoryRequest("하위 카테고리 1-2", categoryResponse1.getId()));
CategoryResponse categoryResponse2 = categoryService.saveCategory(new CategoryRequest("상위 카테고리 2", null));
categoryService.saveCategory(new CategoryRequest("하위 카테고리 2-1", categoryResponse2.getId()));
categoryService.saveCategory(new CategoryRequest("하위 카테고리 2-2", categoryResponse2.getId()));
categoryService.saveCategory(new CategoryRequest("하위 카테고리 2-3", categoryResponse2.getId()));
// when
List<CategoryResponse> categories = categoryService.findCategories();
// then
// 상위 카테고리 조회
assertThat(categories).hasSize(2);
List<Long> categoryIds = categories.stream()
.map(CategoryResponse::getId).collect(Collectors.toList());
assertThat(categoryIds).contains(
categoryResponse1.getId(), categoryResponse2.getId()
);
// 1번 째 카테고리의 하위 카테고리 목록 조회
CategoryResponse resultCategory1 = categories.get(0);
assertThat(resultCategory1.getSubCategories()).hasSize(2);
List<String> category1SubNames = resultCategory1.getSubCategories()
.stream()
.map(CategoryResponse::getName).collect(Collectors.toList());
assertThat(category1SubNames).containsExactly(
"하위 카테고리 1-1", "하위 카테고리 1-2"
);
// 2번 째 카테고리의 하위 카테고리 목록 조회
CategoryResponse resultCategory2 = categories.get(1);
assertThat(resultCategory2.getSubCategories()).hasSize(3);
List<String> category2SubNames = resultCategory2.getSubCategories()
.stream()
.map(CategoryResponse::getName).collect(Collectors.toList());
assertThat(category2SubNames).containsExactly(
"하위 카테고리 2-1", "하위 카테고리 2-2", "하위 카테고리 2-3"
);
}
}
하지만, 위 테스트 코드는 Service 코드에 대한 테스트이다.
실제로 리팩토링 하는 기능은 API 결과에 변화가 생기는 것이기 때문에 API를 호출하는 테스트도 작성하였다.(이후부턴 인수테스트 라고 부르도록 하겠다.)
// @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) 등 인수테스트에 필요한 설정은
// AcceptanceTest class 를 별도로 생성하여 상속받아 자동으로 적용되게 처리함.
public class CategoryAcceptanceTest extends AcceptanceTest {
/* -- 해당 기능 외 테스트 코드 중략 -- */
/**
* given: 기존에 카테고리(상위1 : 하위1-1,하위1-2 / 상위2 : 하위2-1,하위2-2,하위2-3) 가 생성되어 있고,
* when: 카테고리를 전체 조회 하면,
* then: 카테고리 전체 목록이 조회된다.
*/
@Test
@DisplayName("전체 카테고리 목록 조회")
void find_categories_test() {
// given
CategoryResponse 상위1 = 카테고리_등록("상위1", null).as(CategoryResponse.class);
카테고리_등록("하위1-1", 상위1.getId());
카테고리_등록("하위1-2", 상위1.getId());
CategoryResponse 상위2 = 카테고리_등록("상위2", null).as(CategoryResponse.class);
카테고리_등록("하위2-1", 상위2.getId());
카테고리_등록("하위2-2", 상위2.getId());
카테고리_등록("하위2-3", 상위2.getId());
// when
ExtractableResponse<Response> 전체_카테고리_조회_결과 = RestAssured.given().log().all()
.contentType(MediaType.APPLICATION_JSON_VALUE)
.accept(MediaType.APPLICATION_JSON_VALUE)
.when().get("/categories")
.then().log().all()
.extract();
// then
assertThat(전체_카테고리_조회_결과.statusCode()).isEqualTo(HttpStatus.OK.value());
List<CategoryResponse> data = 전체_카테고리_조회_결과.jsonPath().getList("data", CategoryResponse.class);
assertThat(data).hasSize(2);
List<Long> ids = data.stream()
.map(CategoryResponse::getId)
.collect(Collectors.toList());
assertThat(ids).contains(
상위1.getId(), 상위2.getId()
);
CategoryResponse 결과1 = data.stream()
.filter(categoryResponse -> categoryResponse.getId().equals(상위1.getId()))
.findAny()
.orElseThrow();
assertThat(결과1.getSubCategories()).hasSize(2);
CategoryResponse 결과2 = data.stream()
.filter(categoryResponse -> categoryResponse.getId().equals(상위2.getId()))
.findAny()
.orElseThrow();
assertThat(결과2.getSubCategories()).hasSize(3);
}
}
위 테스트를 실행하면, RestAssured 를 사용하여 실제로 GET 호출이 발생하고 그에 대한 Response 까지 받아온다.
그 받아온 Response 가 의도한 대로 응답된 것이 맞는지 검증하는 테스트코드이다.
4. 리팩토링 하기
검증에 쓰일 테스트 코드까지 완성되었으니, 이제 실제로 리팩토링 작업을 진행해보자.
리팩토링 순서를 아래와 같이 진행해보기로 했다.
- CategoryResponse 수정으로 API 결과 변경
- CategoryService 의 findCategories() 메서드 리팩토링
- 인수테스트 코드 리팩토링
4-1. CategoryResponse 수정
API의 결과로 CategoryResponse 를 생성하는 부분을 먼저 확인해보기로 했다.
메서드만 따로 가져와서 확인해보면 아래와 같았다.
public List<CategoryResponse> findCategories() {
List<CategoryQueryDto> categories = categoryQueryRepository.findCategoryAll();
return categories.stream()
.collect(
groupingBy(categoryQueryDto -> new CategoryResponse(categoryQueryDto.getParentId(),
categoryQueryDto.getParentName()),
mapping(categoryQueryDto -> new CategoryResponse(categoryQueryDto.getChildId(),
categoryQueryDto.getChildName()), toList()
)
)).entrySet().stream()
.map(
//여기서 생성한 객체가 최종적으로 list로 변환됨
e -> new CategoryResponse(e.getKey().getId(), e.getKey().getName(), e.getValue())
).collect(toList());
}
해당 부분만 발췌해서 본다면 아래와 같다.
e -> new CategoryResponse(e.getKey().getId(), e.getKey().getName(), e.getValue())
CategoryResponse class 와 비교해보면 아래처럼 확인된다.
@Data
@NoArgsConstructor(access = AccessLevel.PUBLIC)
public class CategoryResponse {
private Long id;
private String name;
private List<CategoryResponse> subCategories = new ArrayList<>();
public CategoryResponse(Long id, String name) {
this.id = id;
this.name = name;
}
// CategoryService 의 findCategories() 메서드에서 사용하는 생성자
public CategoryResponse(Long id, String name, List<CategoryResponse> subCategories) {
this.id = id;
this.name = name;
this.subCategories = subCategories;
}
public static CategoryResponse of(Category category) {
return new CategoryResponse(category.getId(), category.getName(),
category.getSubCategories().stream()
.map(CategoryResponse::of)
.collect(Collectors.toList()));
}
public List<CategoryResponse> getSubCategories() {
return subCategories;
}
}
subCategories 필드를 선택적으로 JSON으로 표현하기 위해서는 2가지가 선행되어야 한다.
- 하위 카테고리의 response 는 subCategories 가 null 이어야 한다.
- 값이 Null인 필드는 JSON으로 만들지 않는다.
위 2가지를 순차적으로 진행해보려 한다.
먼저, subCategories list에 element가 없으면, 생성자 내에서의 초기화를 안하도록 수정했다.
또한, 필드변수에서 직접 new ArrayList<>() 로 초기화 했던 부분도 제거했다.
@Data
@NoArgsConstructor(access = AccessLevel.PUBLIC)
public class CategoryResponse {
private Long id;
private String name;
// 초기화 구문 제거
//private List<CategoryResponse> subCategories = new ArrayList<>();
private List<CategoryResponse> subCategories;
public CategoryResponse(Long id, String name) {
this.id = id;
this.name = name;
}
public CategoryResponse(Long id, String name, List<CategoryResponse> subCategories) {
this.id = id;
this.name = name;
// 리스트가 비어있지 않으면, 파라미터로 넘어온 list 로 초기화
if(!subCategories.isEmpty()) {
this.subCategories = subCategories;
}
}
public static CategoryResponse of(Category category) {
return new CategoryResponse(category.getId(), category.getName(),
category.getSubCategories().stream()
.map(CategoryResponse::of)
.collect(Collectors.toList()));
}
public List<CategoryResponse> getSubCategories() {
return subCategories;
}
}
위 처럼 수정된 상태로 테스트 코드를 실행해보면 아래와 같이 성공하는 것을 볼 수 있다.
그럼 실제로 postman 의 결과는 어떻게 나오는지 확인해보자.
이제 빈 리스트 대신 Null 을 반환하게 됐습니다. 이걸 제거해보도록 하겠습니다.
제거하는 방법은 여러가지가 있겠지만, 간단하게 annotation 하나를 써서 제거하도록 하겠습니다.
CategoryResponse class 에 @JsonInclude(Include.NON_NULL) 을 추가하도록 하겠습니다.
@Data
@JsonInclude(Include.NON_NULL)
@NoArgsConstructor(access = AccessLevel.PUBLIC)
public class CategoryResponse {
private Long id;
private String name;
private List<CategoryResponse> subCategories;
public CategoryResponse(Long id, String name) {
this.id = id;
this.name = name;
}
public CategoryResponse(Long id, String name, List<CategoryResponse> subCategories) {
this.id = id;
this.name = name;
if(!subCategories.isEmpty()) {
this.subCategories = subCategories;
}
}
public static CategoryResponse of(Category category) {
return new CategoryResponse(category.getId(), category.getName(),
category.getSubCategories().stream()
.map(CategoryResponse::of)
.collect(Collectors.toList()));
}
public List<CategoryResponse> getSubCategories() {
return subCategories;
}
}
다시 postman 을 실행하여 결과를 확인해보면 아래와 같다.
이제 subCategories 가 Null 인 항목들이 아예 json 에서 빠진 것을 확인할 수 있다.
4-2. CategoryService 수정
처음 계획대로 원하는 수정은 마무리가 됐지만, CategoryService 에 복잡한 로직을 조금 알아보기 쉽게 리팩토링 해보려고 한다.
리팩토링 대상은 바로 아래 부분 입니다.
public List<CategoryResponse> findCategories() {
List<CategoryQueryDto> categories = categoryQueryRepository.findCategoryAll();
/* -- 여기부터 -- */
return categories.stream()
.collect(
groupingBy(categoryQueryDto -> new CategoryResponse(categoryQueryDto.getParentId(),
categoryQueryDto.getParentName()),
mapping(categoryQueryDto -> new CategoryResponse(categoryQueryDto.getChildId(),
categoryQueryDto.getChildName()), toList()
)
)).entrySet().stream()
.map(
e -> new CategoryResponse(e.getKey().getId(), e.getKey().getName(), e.getValue())
).collect(toList());
/* -- 여기까지 -- */
}
람다식을 너무 무분별하게(?) 사용한 느낌도 있고, 메서드를 분리하기도 쉽지 않은 구조라고 생각했다.
그래서 조금 보기쉽게 리팩토링을 도전해봤다.
먼저, 위 방식은 쿼리 1번으로 조회한 데이터를 dto 형태로 반환했다면, 변경한 방법은 쿼리 2번으로 dto 조회를 해보았다.
먼저 아래와 같이 repository 에 새로운 쿼리를 정의했다.
public interface CategoryQueryRepository {
List<CategoryQueryDto> findCategoryAll();
/* -- 신규 추가 -- */
List<CategoryDto> findParentsDto();
List<CategoryDto> findSubCategoryDto(List<Long> parentIds);
}
findParentDto 는 상위 카테고리를 전부 조회하는 쿼리를, findSubCategoryDto 는 모든 하위 카테고리를 조회하는 쿼리이다.
구현체는 아래와 같이 작성했다.
@RequiredArgsConstructor
@Repository
public class CategoryQueryRepositoryImpl implements CategoryQueryRepository {
private final EntityManager entityManager;
/* -- 코드 중략 -- */
@Override
public List<CategoryDto> findParentsDto() {
return entityManager.createQuery(
"select new springboot.shoppingmall.category.dto.CategoryDto(id, name, parent.id) "
+ "from Category "
+ "where parent is null "
+ "order by id", CategoryDto.class)
.getResultList();
}
@Override
public List<CategoryDto> findSubCategoryDto(List<Long> parentIds) {
return entityManager.createQuery(
"select new springboot.shoppingmall.category.dto.CategoryDto(id, name, parent.id) "
+ "from Category "
+ "where parent.id in :parentIds "
+ "order by id", CategoryDto.class)
.setParameter("parentIds", parentIds)
.getResultList();
}
}
JPQL 을 문자열로 작성한 형태이기에 쿼리를 제대로 작성했는지 여부를 확인하고자 테스트 코드를 아래와 같이 작성했다.
@Transactional
@SpringBootTest
class CategoryQueryRepositoryTest {
@Autowired
CategoryRepository categoryRepository;
@Autowired
CategoryQueryRepository categoryQueryRepository;
Category category1;
Category subCategory1;
Category subCategory2;
Category category2;
Category subCategory3;
Category subCategory4;
Category subCategory5;
@BeforeEach
void beforeEach() {
category1 = categoryRepository.save(new Category("상위 카테고리"));
subCategory1 = categoryRepository.save(new Category("하위 카테고리 1").changeParent(category1));
subCategory2 = categoryRepository.save(new Category("하위 카테고리 2").changeParent(category1));
category2 = categoryRepository.save(new Category("상위 카테고리 2"));
subCategory3 = categoryRepository.save(new Category("하위 카테고리 3").changeParent(category2));
subCategory4 = categoryRepository.save(new Category("하위 카테고리 4").changeParent(category2));
subCategory5 = categoryRepository.save(new Category("하위 카테고리 5").changeParent(category2));
}
@Test
@DisplayName("상위 카테고리 조회 - dto 버전")
void find_parent_dto_test() {
// given
// when
List<CategoryDto> parentsDto = categoryQueryRepository.findParentsDto();
List<Long> parentIds = parentsDto.stream()
.map(CategoryDto::getId)
.collect(Collectors.toList());
// then
assertThat(parentIds).containsExactly(
category1.getId(), category2.getId()
);
}
@Test
@DisplayName("하위 카테고리 조회 - dto 버전")
void find_children_dto_test() {
// given
List<CategoryDto> parents = categoryQueryRepository.findParentsDto();
List<Long> ids = parents.stream()
.map(CategoryDto::getId)
.collect(Collectors.toList());
// when
List<CategoryDto> childrenAll = categoryQueryRepository.findSubCategoryDto(ids);
List<Long> childrenIds = childrenAll.stream()
.map(CategoryDto::getId)
.collect(Collectors.toList());
// then
assertThat(childrenIds).containsExactly(
subCategory1.getId(), subCategory2.getId(), subCategory3.getId(), subCategory4.getId(), subCategory5.getId()
);
}
}
테스트 결과는 아래와 같이 잘 나오는 것을 확인할 수 있었다.
위에서 새로 만든 쿼리를 활용하여 Service 코드를 다시 만들었다.
public List<CategoryResponse> findCategories() {
List<CategoryDto> parentsDto = categoryQueryRepository.findParentsDto();
List<Long> parentIds = parentsDto.stream()
.map(CategoryDto::getId)
.collect(toList());
List<CategoryDto> childrenDto = categoryQueryRepository.findSubCategoryDto(parentIds);
Map<Long, List<CategoryDto>> subCategoryMap = childrenDto.stream()
.collect(groupingBy(CategoryDto::getParentId));
return parentsDto.stream()
.map(categoryDto -> new CategoryResponse(categoryDto.getId(), categoryDto.getName(), subCategoryMap.get(categoryDto.getId()).stream()
.map(subCategoryDto -> new CategoryResponse(subCategoryDto.getId(), subCategoryDto.getName()))
.collect(toList())))
.collect(toList());
}
위 코드의 절차는 다음과 같다.
- 상위 카테고리 dto를 모두 조회한다.
- 상위 카테고리들의 id 만을 따로 추출한다.
- 상위 카테고리의 id들을 통해 하위 카테고리 dto를 모두 조회한다.
- 하위 카테고리를 상위 카테고리를 기준으로 Map으로 변환한다.
- 상위 카테고리 dto 를 순회하면서 CategoryResponse 객체를 생성해서 return 한다.
일단 위 코드를 대상으로 테스트 코드를 돌려보겠습니다.
테스트는 성공했으니, 의도한대로 쿼리 2번으로 실행되었는지, 테스트코드의 실행로그를 보겠습니다.
Request method: GET
Request URI: http://localhost:60473/categories
Proxy: <none>
Request params: <none>
Query params: <none>
Form params: <none>
Path params: <none>
Headers: Accept=application/json
Content-Type=application/json
Cookies: <none>
Multiparts: <none>
Body: <none>
Hibernate:
/* select
new springboot.shoppingmall.category.dto.CategoryDto(id,
name,
parent.id)
from
Category
where
parent is null
order by
id */ select
category0_.id as col_0_0_,
category0_.name as col_1_0_,
category0_.parent_id as col_2_0_
from
category category0_
where
category0_.parent_id is null
order by
category0_.id
Hibernate:
/* select
new springboot.shoppingmall.category.dto.CategoryDto(id,
name,
parent.id)
from
Category
where
parent.id in :parentIds
order by
id */ select
category0_.id as col_0_0_,
category0_.name as col_1_0_,
category0_.parent_id as col_2_0_
from
category category0_
where
category0_.parent_id in (
? , ?
)
order by
category0_.id
2023-03-16 19:38:50.323 TRACE 2992 --- [o-auto-1-exec-2] o.h.type.descriptor.sql.BasicBinder : binding parameter [1] as [BIGINT] - [1]
2023-03-16 19:38:50.324 TRACE 2992 --- [o-auto-1-exec-2] o.h.type.descriptor.sql.BasicBinder : binding parameter [2] as [BIGINT] - [4]
HTTP/1.1 200
Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
Content-Type: application/json
Transfer-Encoding: chunked
Date: Thu, 16 Mar 2023 10:38:49 GMT
Keep-Alive: timeout=60
Connection: keep-alive
{
"data": [
{
"id": 1,
"name": "상위1",
"subCategories": [
{
"id": 2,
"name": "하위1-1"
},
{
"id": 3,
"name": "하위1-2"
}
]
},
{
"id": 4,
"name": "상위2",
"subCategories": [
{
"id": 5,
"name": "하위2-1"
},
{
"id": 6,
"name": "하위2-2"
},
{
"id": 7,
"name": "하위2-3"
}
]
}
]
}
2023-03-16 19:38:51.177 INFO 2992 --- [ionShutdownHook] j.LocalContainerEntityManagerFactoryBean : Closing JPA EntityManagerFactory for persistence unit 'default'
2023-03-16 19:38:51.181 INFO 2992 --- [ionShutdownHook] com.zaxxer.hikari.HikariDataSource : HikariPool-1 - Shutdown initiated...
2023-03-16 19:38:51.187 INFO 2992 --- [ionShutdownHook] com.zaxxer.hikari.HikariDataSource : HikariPool-1 - Shutdown completed.
Process finished with exit code 0
실제로 카테고리 목록을 호출하는 GET 요청 부터 끝까지의 로그를 가져왔습니다.
select 쿼리가 2번 나갔고, 그 중에 2번째 select 쿼리에 in 절이 있는 것을 보아 의도한대로 2개의 쿼리가 나간 것을 볼 수 있습니다.
4-3. CategoryService 리팩토링 (feat. CategoryResponse)
이전에 수정한 Service 코드는 직전 Service 코드보다 무슨 동작을 하는지 알아보기 쉬워진 것 같습니다.
하지만 아직 메서드의 길이가 너무 길고, 어떤 동작을 하는지 한 눈에 들어오지 않습니다.
이번에는 가독성을 고려한 리팩토링을 한 번 해보려 합니다.
(매 단계 리펙토링을 할 때 마다 테스트코드를 돌려야 합니다. 본 포스팅에선 너무 같은 말을 반복할 것 같아 생략하겠지만, 리팩토링 할 때는 꼭 작은 수정 직후에도 테스트코드를 돌리는 것을 잊지 말아야 합니다!)
먼저, 메서드로 분리할 수 있는 내용 먼저 분리하겠습니다.
subCategoryMap 을 생성하는 로직을 메서드로 분리하겠습니다.
public List<CategoryResponse> findCategories() {
List<CategoryDto> parentsDto = categoryQueryRepository.findParentsDto();
List<Long> parentIds = parentsDto.stream()
.map(CategoryDto::getId)
.collect(toList());
List<CategoryDto> childrenDto = categoryQueryRepository.findSubCategoryDto(parentIds);
// 메서드로 분리
Map<Long, List<CategoryDto>> subCategoryMap = groupingSubCategoryByParent(childrenDto);
return parentsDto.stream()
.map(categoryDto -> new CategoryResponse(categoryDto.getId(), categoryDto.getName(), subCategoryMap.get(categoryDto.getId()).stream()
.map(subCategoryDto -> new CategoryResponse(subCategoryDto.getId(), subCategoryDto.getName()))
.collect(toList())))
.collect(toList());
}
private Map<Long, List<CategoryDto>> groupingSubCategoryByParent(List<CategoryDto> childrenDto) {
return childrenDto.stream()
.collect(groupingBy(CategoryDto::getParentId));
}
그 다음으론 가장 복잡해보이는 return 쪽을 메서드 분리를 통해 보기 좋게 만들어 보겠습니다.
return 문의 문제는 parent 역할을 할 CategoryResponse를 new 하는 시점에 어디서 어디까지가 parent 인지 알아보기 어렵습니다.
그래서 id와 name 은 일단 두고, subCategories 를 메서드로 분리해보겠습니다.
public List<CategoryResponse> findCategories() {
List<CategoryDto> parentsDto = categoryQueryRepository.findParentsDto();
List<Long> parentIds = parentsDto.stream()
.map(CategoryDto::getId)
.collect(toList());
List<CategoryDto> childrenDto = categoryQueryRepository.findSubCategoryDto(parentIds);
Map<Long, List<CategoryDto>> subCategoryMap = groupingSubCategoryByParent(childrenDto);
return parentsDto.stream()
.map(categoryDto -> new CategoryResponse(categoryDto.getId(), categoryDto.getName(), getSubCategories(categoryDto, subCategoryMap)))
.collect(toList());
}
private Map<Long, List<CategoryDto>> groupingSubCategoryByParent(List<CategoryDto> childrenDto) {
return childrenDto.stream()
.collect(groupingBy(CategoryDto::getParentId));
}
private List<CategoryResponse> getSubCategories(CategoryDto parent, Map<Long, List<CategoryDto>> subCategoryMap ) {
return subCategoryMap.get(parent.getId()).stream()
.map(subCategoryDto -> new CategoryResponse(subCategoryDto.getId(), subCategoryDto.getName()))
.collect(toList());
}
그리고 또 수정할 곳을 찾아보고 나서, 아래 부분을 수정해봤습니다.
.map(categoryDto -> new CategoryResponse(categoryDto.getId(), categoryDto.getName(), getSubCategories(categoryDto, subCategoryMap)))
여기서는 new 로 생성하지 않고, 정적팩토리로써 객체를 생성하게 해보았습니다.
그리고 categoryDto의 getId() 와 getName()를 따로 넘기지 않고, CategoryDto 자체를 넘겼습니다.
public List<CategoryResponse> findCategories() {
List<CategoryDto> parentsDto = categoryQueryRepository.findParentsDto();
List<Long> parentIds = parentsDto.stream()
.map(CategoryDto::getId)
.collect(toList());
List<CategoryDto> childrenDto = categoryQueryRepository.findSubCategoryDto(parentIds);
Map<Long, List<CategoryDto>> subCategoryMap = groupingSubCategoryByParent(childrenDto);
return parentsDto.stream()
.map(categoryDto -> CategoryResponse.of(categoryDto, getSubCategories(categoryDto, subCategoryMap)))
.collect(toList());
}
private Map<Long, List<CategoryDto>> groupingSubCategoryByParent(List<CategoryDto> childrenDto) {
return childrenDto.stream()
.collect(groupingBy(CategoryDto::getParentId));
}
private List<CategoryResponse> getSubCategories(CategoryDto parent, Map<Long, List<CategoryDto>> subCategoryMap ) {
return subCategoryMap.get(parent.getId()).stream()
.map(subCategoryDto -> new CategoryResponse(subCategoryDto.getId(), subCategoryDto.getName()))
.collect(toList());
}
위와 같이 수정된 탓에 CategoryResponse 에도 새로운 of 메서드가 정의되었습니다.
public static CategoryResponse of(CategoryDto categoryDto, List<CategoryResponse> subCategories) {
return new CategoryResponse(categoryDto.getId(), categoryDto.getName(), subCategories);
}
이렇게 수정하고 나니 또 찝찝한 부분이 있었습니다.
바로 새로 정의한 CategoryResponse 의 of 메서드 입니다.
매개변수로 2개를 받는데, 하나는 CategoryDto 타입이고 나머지 하나는 List<CategoryResponse> 타입입니다.
CategoryResponse 를 생성하기 위한 메서드 인데, 이미 Response 를 받고있다는게 좀 걸려서 해당 부분도 Dto 로 받게 수정했습니다.
그 과정에서 자연스럽게(?) of 메서드 안에서 CategoryDto 타입의 List 를 CategoryResponse 타입의 List로 변경해주었습니다.
public static CategoryResponse of(CategoryDto categoryDto, List<CategoryDto> subCategoriesDto) {
return new CategoryResponse(categoryDto.getId(), categoryDto.getName(),
subCategoriesDto.stream()
.map(subCategoryDto -> new CategoryResponse(subCategoryDto.getId(), subCategoryDto.getName()))
.collect(Collectors.toList()));
}
이렇게 하고 나니 CategoryService 에서 컴파일 에러가 발생해버렸습니다.
public List<CategoryResponse> findCategories() {
List<CategoryDto> parentsDto = categoryQueryRepository.findParentsDto();
List<Long> parentIds = parentsDto.stream()
.map(CategoryDto::getId)
.collect(toList());
List<CategoryDto> childrenDto = categoryQueryRepository.findSubCategoryDto(parentIds);
Map<Long, List<CategoryDto>> subCategoryMap = groupingSubCategoryByParent(childrenDto);
return parentsDto.stream()
.map(categoryDto -> CategoryResponse.of(categoryDto, getSubCategories(categoryDto, subCategoryMap))) // 이 부분이 컴파일 에러!
.collect(toList());
}
private Map<Long, List<CategoryDto>> groupingSubCategoryByParent(List<CategoryDto> childrenDto) {
return childrenDto.stream()
.collect(groupingBy(CategoryDto::getParentId));
}
private List<CategoryResponse> getSubCategories(CategoryDto parent, Map<Long, List<CategoryDto>> subCategoryMap ) {
return subCategoryMap.get(parent.getId()).stream()
.map(subCategoryDto -> new CategoryResponse(subCategoryDto.getId(), subCategoryDto.getName()))
.collect(toList());
}
왠지 봤더니, CategoryResponse.of 하는 부분에서 2번 째 매개변수가 getSubCategories() 메서드의 return 값인데, 해당 return의 타입이 List<CategoryResponse> 였습니다. 이 부분 또한 List<CategoryDto> 로 return 하게끔 수정해줬습니다.
public List<CategoryResponse> findCategories() {
List<CategoryDto> parentsDto = categoryQueryRepository.findParentsDto();
List<Long> parentIds = parentsDto.stream()
.map(CategoryDto::getId)
.collect(toList());
List<CategoryDto> childrenDto = categoryQueryRepository.findSubCategoryDto(parentIds);
Map<Long, List<CategoryDto>> subCategoryMap = groupingSubCategoryByParent(childrenDto);
return parentsDto.stream()
.map(categoryDto -> CategoryResponse.of(categoryDto, getSubCategories(categoryDto, subCategoryMap)))
.collect(toList());
}
private Map<Long, List<CategoryDto>> groupingSubCategoryByParent(List<CategoryDto> childrenDto) {
return childrenDto.stream()
.collect(groupingBy(CategoryDto::getParentId));
}
private List<CategoryDto> getSubCategories(CategoryDto parent, Map<Long, List<CategoryDto>> subCategoryMap ) {
return subCategoryMap.get(parent.getId());
}
최초에 복잡하던 코드와 비교해보면 훨씬 이해하기 쉬워진 것 같습니다.(저만 그런가요?ㅠ)
4-3. 인수테스트 코드 리팩토링
현재 인수테스트는 아래와 같이 작성되어 있습니다.
// @SpringBootTest(webEnvironment = WebEnvironment.RANDOM_PORT) 등 인수테스트에 필요한 설정은
// AcceptanceTest class 를 별도로 생성하여 상속받아 자동으로 적용되게 처리함.
public class CategoryAcceptanceTest extends AcceptanceTest {
/* -- 해당 기능 외 테스트 코드 중략 -- */
/**
* given: 기존에 카테고리(상위1 : 하위1-1,하위1-2 / 상위2 : 하위2-1,하위2-2,하위2-3) 가 생성되어 있고,
* when: 카테고리를 전체 조회 하면,
* then: 카테고리 전체 목록이 조회된다.
*/
@Test
@DisplayName("전체 카테고리 목록 조회")
void find_categories_test() {
// given
CategoryResponse 상위1 = 카테고리_등록("상위1", null).as(CategoryResponse.class);
카테고리_등록("하위1-1", 상위1.getId());
카테고리_등록("하위1-2", 상위1.getId());
CategoryResponse 상위2 = 카테고리_등록("상위2", null).as(CategoryResponse.class);
카테고리_등록("하위2-1", 상위2.getId());
카테고리_등록("하위2-2", 상위2.getId());
카테고리_등록("하위2-3", 상위2.getId());
// when
ExtractableResponse<Response> 전체_카테고리_조회_결과 = RestAssured.given().log().all()
.contentType(MediaType.APPLICATION_JSON_VALUE)
.accept(MediaType.APPLICATION_JSON_VALUE)
.when().get("/categories")
.then().log().all()
.extract();
// then
assertThat(전체_카테고리_조회_결과.statusCode()).isEqualTo(HttpStatus.OK.value());
List<CategoryResponse> data = 전체_카테고리_조회_결과.jsonPath().getList("data", CategoryResponse.class);
assertThat(data).hasSize(2);
List<Long> ids = data.stream()
.map(CategoryResponse::getId)
.collect(Collectors.toList());
assertThat(ids).contains(
상위1.getId(), 상위2.getId()
);
CategoryResponse 결과1 = data.stream()
.filter(categoryResponse -> categoryResponse.getId().equals(상위1.getId()))
.findAny()
.orElseThrow();
assertThat(결과1.getSubCategories()).hasSize(2);
CategoryResponse 결과2 = data.stream()
.filter(categoryResponse -> categoryResponse.getId().equals(상위2.getId()))
.findAny()
.orElseThrow();
assertThat(결과2.getSubCategories()).hasSize(3);
}
}
현재 인수테스트는 2가지의 문제가 있다고 생각합니다.
- 검증 부분에 중복코드
- 개발자 외의 타인이 보는 경우 해석이 어려움
저는 여기서 2번이 중요하다고 생각합니다.
왜냐하면, 인수테스트의 본질은 해당 API의 개발을 요청한 사람도 알아볼 수 있어야 하기 때문입니다.
그리고 그 요청한 주체가 개발자라는 보장은 없습니다.
즉, 개발자 외의 사람이 봐도 어느정도 이해가 되는 흐름을 가지고 기능을 점검할 수 있게 해주는 것이
바로 인수테스트의 본질이라고 생각합니다.
위 부분에서 CategoryResponse 를 2번 검증합니다. 결과1 과 결과2 인데요.
두 검증 코드는 너무도 흡사합니다. 특정 상위 카테고리의 하위 카테고리의 갯수를 검증하고 있죠.
이 흡사한 두 검증을 하나의 메서드로 분리해보았습니다.
/**
* given: 기존에 카테고리(상위1 : 하위1-1,하위1-2 / 상위2 : 하위2-1,하위2-2,하위2-3) 가 생성되어 있고,
* when: 카테고리를 전체 조회 하면,
* then: 카테고리 전체 목록이 조회된다.
*/
@Test
@DisplayName("전체 카테고리 목록 조회")
void find_categories_test() {
// given
CategoryResponse 상위1 = 카테고리_등록("상위1", null).as(CategoryResponse.class);
카테고리_등록("하위1-1", 상위1.getId());
카테고리_등록("하위1-2", 상위1.getId());
CategoryResponse 상위2 = 카테고리_등록("상위2", null).as(CategoryResponse.class);
카테고리_등록("하위2-1", 상위2.getId());
카테고리_등록("하위2-2", 상위2.getId());
카테고리_등록("하위2-3", 상위2.getId());
// when
ExtractableResponse<Response> 전체_카테고리_조회_결과 = RestAssured.given().log().all()
.contentType(MediaType.APPLICATION_JSON_VALUE)
.accept(MediaType.APPLICATION_JSON_VALUE)
.when().get("/categories")
.then().log().all()
.extract();
// then
assertThat(전체_카테고리_조회_결과.statusCode()).isEqualTo(HttpStatus.OK.value());
List<CategoryResponse> data = 전체_카테고리_조회_결과.jsonPath().getList("data", CategoryResponse.class);
assertThat(data).hasSize(2);
List<Long> ids = data.stream()
.map(CategoryResponse::getId)
.collect(Collectors.toList());
assertThat(ids).contains(
상위1.getId(), 상위2.getId()
);
카테고리_유무_갯수_검증(data, 상위1, 2);
카테고리_유무_갯수_검증(data, 상위2, 3);
}
private void 카테고리_유무_갯수_검증(List<CategoryResponse> data, CategoryResponse category, int subCategoryCount) {
CategoryResponse 결과1 = data.stream()
.filter(categoryResponse -> categoryResponse.getId().equals(category.getId()))
.findAny()
.orElseThrow();
assertThat(결과1.getSubCategories()).hasSize(subCategoryCount);
}
그리고 타인이 테스트 시나리오를 보고 대략적으로 의미를 알아볼 수 있게 코드를 좀 간결하게 만들어 보겠습니다.
이 과정에서 변수 혹은 메서드명에 한글이 사용되는 것은 괜찮다고 생각합니다.
/**
* given: 기존에 카테고리(상위1 : 하위1-1,하위1-2 / 상위2 : 하위2-1,하위2-2,하위2-3) 가 생성되어 있고,
* when: 카테고리를 전체 조회 하면,
* then: 카테고리 전체 목록이 조회된다.
*/
@Test
@DisplayName("전체 카테고리 목록 조회")
void find_categories_test() {
// given
CategoryResponse 상위_카테고리_1 = 카테고리_등록("상위1", null).as(CategoryResponse.class);
카테고리_등록("하위1-1", 상위_카테고리_1.getId());
카테고리_등록("하위1-2", 상위_카테고리_1.getId());
CategoryResponse 상위_카테고리_2 = 카테고리_등록("상위2", null).as(CategoryResponse.class);
카테고리_등록("하위2-1", 상위_카테고리_2.getId());
카테고리_등록("하위2-2", 상위_카테고리_2.getId());
카테고리_등록("하위2-3", 상위_카테고리_2.getId());
// when
ExtractableResponse<Response> 전체_카테고리_조회_결과 = 전체_카테고리_조회_요청();
// then
전체_카테고리_조회_성공_검증(전체_카테고리_조회_결과);
List<CategoryResponse> 상위_카테고리_전체 = 전체_카테고리_조회_결과.jsonPath().getList("data", CategoryResponse.class);
카테고리_갯수_검증(상위_카테고리_전체, 2);
카테고리_목록_검증(상위_카테고리_전체, 상위_카테고리_1, 상위_카테고리_2);
카테고리_유무_갯수_검증(상위_카테고리_전체, 상위_카테고리_1, 2);
카테고리_유무_갯수_검증(상위_카테고리_전체, 상위_카테고리_2, 3);
}
인수 테스트는 이런식으로 한글말 위주로 작성하되, 실제로 메서드 내에서의 검증로직은 구체적으로 작성해야 될 것입니다.
치환된 메서드들은 아래에 별도로 추가해놨으니 참고 부탁드립니다.
private void 카테고리_목록_검증(List<CategoryResponse> 카테고리_목록, CategoryResponse... 대상_카테고리) {
List<Long> ids = getIdsInList(카테고리_목록);
List<CategoryResponse> targetCategories = Arrays.asList(대상_카테고리);
List<Long> targetCategoryIds = getIdsInList(targetCategories);
assertThat(ids).containsAll(targetCategoryIds);
}
private List<Long> getIdsInList(List<CategoryResponse> targetCategories) {
return targetCategories.stream()
.map(CategoryResponse::getId)
.collect(Collectors.toList());
}
private void 카테고리_갯수_검증(List<CategoryResponse> categories, int categoryCount) {
assertThat(categories).hasSize(categoryCount);
}
private void 전체_카테고리_조회_성공_검증(ExtractableResponse<Response> 전체_카테고리_조회_결과) {
assertThat(전체_카테고리_조회_결과.statusCode()).isEqualTo(HttpStatus.OK.value());
}
private ExtractableResponse<Response> 전체_카테고리_조회_요청() {
return RestAssured.given().log().all()
.contentType(MediaType.APPLICATION_JSON_VALUE)
.accept(MediaType.APPLICATION_JSON_VALUE)
.when().get("/categories")
.then().log().all()
.extract();
}
private void 카테고리_유무_갯수_검증(List<CategoryResponse> data, CategoryResponse category, int subCategoryCount) {
CategoryResponse parentCategory = data.stream()
.filter(categoryResponse -> categoryResponse.getId().equals(category.getId()))
.findAny()
.orElseThrow();
카테고리_갯수_검증(parentCategory.getSubCategories(), subCategoryCount);
}
토이프로젝트를 개발하면서, 우아한 테크캠프 Pro 와 인프런 강의를 듣고 조금씩 고도화를 하는 중이었는데
이렇게 나마 리팩토링하는 과정을 포스팅 할 수 있어 작성하면서 정리가 된 느낌이었습니다.
이 글을 보시게 되는 누군가에게 도움이 되기를 바라면서, 긴 글 읽어주셔서 감사합니다.
'Refactoring' 카테고리의 다른 글
본문 이미지 저장 로직을 개선해서 서버 성능 개선하기. (0) | 2024.01.28 |
---|
댓글