코드 리팩토링
KDT 팀단위 프로젝트가 끝나고 약간의 시간이 흘렀다. 프로젝트 중에도 스스로 코드를 너무 더럽게 짰다는 후회가 항상 존재했으나 시간에 쫓기는 일정 때문에(변명이겠지만) 기능만 하는 코드를 대충 작성하고 넘어간 바 있다. 이는 팀원들도 마찬가지였다. 이로 인해 서로의 코드를 리뷰하는 것은 거의 불가능에 가까웠고 이미 구현되어있는 메서드를 팀원이 가져다 쓰는 것에도 위험이 따랐다. 해당 코드의 동작이 한 눈에 들어오지 않으며, 코드가 제대로 작성되었는지, 에러는 없는지 의문이 있기 때문이다.
테스트 코드
안전하지 않은 코드에 대한 문제를 항상 상기하고 있었기 때문에, 드디어 코드를 리팩토링하는 시간을 가지고자 한다. 우선 리팩토링 하기 전 내가 짠 코드의 동작이 일정한 결과를 낳게 하기 위한 첫 번째 안전 마진으로서 테스트 코드 작성을 우선하고자 한다.
나는 테스트 방법으로 통합 테스트를 진행한다.
@SpringBootTest(classes = DemoApplication.class)
class productServiceTest {
@Autowired
ProductService productService;
@Test
@DisplayName("1, 2, 3 중 랜덤 값에 대한 상품 이벤트 반환값 테스트")
void When_SetRandomEventNumberToProduct_Expect_FilteredProductData() {
Random random = new Random();
random.setSeed(System.currentTimeMillis());
int randomNumber = random.nextInt(3) + 1;//1~3 난수
ProductFilterDto filterDto = new ProductFilterDto();
filterDto.setSearchingTerm("");
filterDto.setEventNumber(randomNumber);
filterDto.setSelect(0);
List<ProductDto> productDtoList = productService.getProductDtoListByFilter(filterDto, 0, 30);
for(ProductDto dto:productDtoList) {
if(dto.getProductEvent() != randomNumber) {
fail("test fail - getProductEvent()가 randomNumber와 다름");
}
}
}
@Test
@DisplayName("1, 2, 3 중 랜덤 값에 대한 상품 select 반환값 테스트")
void When_SetRandomSelectNumberToProduct_Expect_FilteredProductData() {
Random random = new Random();
random.setSeed(System.currentTimeMillis());
int randomNumber = random.nextInt(3);//1, 2, 3 난수
ProductFilterDto filterDto = new ProductFilterDto();
filterDto.setSearchingTerm("");
filterDto.setEventNumber(0);
filterDto.setSelect(randomNumber);
List<ProductDto> productDtoList = productService.getProductDtoListByFilter(filterDto, 0, 30);
Stack<ProductDto> productStack = new Stack<>();
for(int i = 0; i < productDtoList.size(); i++) {
ProductDto dto = productDtoList.get(i);
if(i == 0) {
productStack.push(dto);
continue;
};
if(randomNumber == 0) {
assertTrue(() -> productStack.peek().getCreatedAt().compareTo(dto.getCreatedAt()) <= 0,
"test fail - 상품이 시간순이 아닙니다.");
}
else if(randomNumber == 1) {
assertTrue(productStack.peek().getPriceNumber() <= dto.getPriceNumber(),
"test fail - 상품이 가격 낮은 순이 아닙니다.");
}else{
assertTrue(productStack.peek().getPriceNumber() >= dto.getPriceNumber(),
"test fail - 상품이 가격 높은 순이 아닙니다.");
}
productStack.push(dto);
}
}
@Test
@DisplayName("랜덤 리미트, 오프셋 대한 반환값 테스트")
void When_OffsetIsRandomTil35_Expect_RandomAmountOfProduct() {
Random random = new Random();
random.setSeed(System.currentTimeMillis());
int randomOffsetNumber = random.nextInt(5);
random.setSeed(System.currentTimeMillis());
int randomLimitNumber = random.nextInt(10) + 1;
ProductFilterDto filterDto = new ProductFilterDto();
filterDto.setSearchingTerm("");
filterDto.setEventNumber(0);
filterDto.setSelect(0);
List<ProductDto> productDtoList = productService.getProductDtoListByFilter(filterDto, randomOffsetNumber, randomLimitNumber);
assertTrue(()->productDtoList.size() == randomLimitNumber, "test fail - 예상 반환양과 실제 반환양이 일치하지 않습니다.");
}
}서비스의 한 기능에 대한 4가지 테스트 케이스를 작성하였다. 테스트 케이스의 삽입 값은 랜덤하게 적용하는 것이 좋다는 글을 읽어 그렇게 진행해보았다.
테스트는 각각 1. 상품 이벤트 타입에 대하여, 2. 상품 select선택에 대하여, 3. offset, limit에 따른 상품 산출 량에 대하여 진행하였다.
테스트 케이스를 작성해보고 느낀 것이, 좋은 테스트 케이스를 작성하는 것 또한 공부해야 할 문제라고 느꼈다. 지금은 너무 막연히 핵심 기능이라고 생각한 요소만 작성하여 추후 공부가 필요하겠다.
아무튼 예상 결과에 대한 테스트 케이스를 작성하였고, 모든 경우에 대해 통과하는 것을 확인하였다. 이제 코드 리팩토링을 안심하고 할 수 있다. 왜냐하면 리팩토링 후 테스트 케이스를 통과하지 못한 경우 로직이 바뀌었다는 것이고, 이는 리팩토링 원칙에 어긋나기 때문이다.
리팩토링할 내용
리팩토링 할 코드는 아래와 같다. select, event, 검색어를 한 번에 받아서 처리 한 물품 결과를 보여주는 로직이다.
@Override
public List<ProductDto> getProductDtoListByFilter(ProductFilterDto productFilterDto, int offset, int limit) {
if(productFilterDto.getSelect() == 3) {
List<Product> productEntityList = productRepository
.getProductDtoListByFilterNotPagable(
productFilterDto.getSearchingTerm(),
productFilterDto.getEventNumber());
List<ProductWithOrderCountDto> productWithOrderCount = new ArrayList<>();
for(Product product: productEntityList) {
int orderCount = getOrderNumberByProductName(product.getProductTitle());
ProductWithOrderCountDto pod = new ProductWithOrderCountDto();
pod.setProductId(product.getProductId());
pod.setPriceNumber(product.getPriceNumber());
pod.setPriceString(product.getPriceString());
pod.setProductTitle(product.getProductTitle());
pod.setProductExpirationDate(product.getProductExpirationDate());
pod.setProductType(product.getProductType());
pod.setProductImgUrl(product.getProductImgUrl());
pod.setProductEvent(product.getProductEvent());
pod.setCreatedAt(product.getCreatedAt());
pod.setProductTimeSale(product.getProductTimeSale());
pod.setOrderCount(orderCount);
productWithOrderCount.add(pod);
}
orderByOrderNumber(productWithOrderCount);
/* 중복 이름 처리, 중복 이름이 존재하면 아예 보여주지 않도록 함*/
List<ProductDto> dtoList = new ArrayList();
for(ProductWithOrderCountDto product:productWithOrderCount) {
boolean isContain = false;
for(ProductDto dto:dtoList) {
String dtoProductName = dto.getProductTitle();
String entityProductName = product.getProductTitle();
if(dtoProductName.equals(entityProductName)) {
isContain = true;
break;
}
}
if(isContain) continue;
ProductDto dto = new ProductDto();
dto.setProductId(product.getProductId());
dto.setPriceNumber(product.getPriceNumber());
dto.setPriceString(product.getPriceString());
dto.setProductTitle(product.getProductTitle());
dto.setProductExpirationDate(product.getProductExpirationDate());
dto.setProductType(product.getProductType());
dto.setProductImgUrl(product.getProductImgUrl());
dto.setProductEvent(product.getProductEvent());
dto.setCreatedAt(product.getCreatedAt());
dto.setProductTimeSale(product.getProductTimeSale());
dtoList.add(dto);
}
return sliceList(dtoList, offset, limit);
}
else {
List<Product> productEntityList = productRepository
.getProductDtoListByFilter(
productFilterDto.getSearchingTerm(),
productFilterDto.getEventNumber(),
productFilterDto.getSelect(),
PageRequest.of(offset, limit));//페이징 offset limit
System.out.println(productEntityList);
/* 중복 이름 처리, 중복 이름이 존재하면 아예 보여주지 않도록 함*/
List<ProductDto> dtoList = new ArrayList();
for(Product product:productEntityList) {
boolean isContain = false;
for(ProductDto dto:dtoList) {
String dtoProductName = dto.getProductTitle();
String entityProductName = product.getProductTitle();
if(dtoProductName.equals(entityProductName)) {
isContain = true;
break;
}
}
if(isContain) continue;
ProductDto dto = new ProductDto();
dto.setProductId(product.getProductId());
dto.setPriceNumber(product.getPriceNumber());
dto.setPriceString(product.getPriceString());
dto.setProductTitle(product.getProductTitle());
dto.setProductExpirationDate(product.getProductExpirationDate());
dto.setProductType(product.getProductType());
dto.setProductImgUrl(product.getProductImgUrl());
dto.setProductEvent(product.getProductEvent());
dto.setCreatedAt(product.getCreatedAt());
dto.setProductTimeSale(product.getProductTimeSale());
dtoList.add(dto);
}
return dtoList;
}
}
문제점
- If문에 따른 분기(flag)
- 역할이 모호, 즉 코드의 응집도가 낮다
시작부터 if문으로 분기한 것이 상당히 거슬린다. 이는 select 3번은 주문 많은 순 정렬인데, pageable을 이용한 offset, limit을 Repository에서 SQL로 구현하기 빡빡하다고 여겨져 자바 코드로 로직을 구현했기 때문이다. 이외에는 모두 JPA에서 지원해주는 메소드로 offset, limit을 처리하였다.
또한 얼핏 봐도 코드에 중복이 존재하며, 여러 기능(select에 따른 결과)이 확실하게 나뉘어져 있지 않다. 또한 중복 아이템 처리, slice등 핵심 로직이 아닌 것 또한 눈에 띈다. 쉽게 말해 코드가 난잡하다. 우선 전략 패턴을 이용해 추후 특정 로직이 변경되더라도 다른 로직에는 영향이 가지 않도록 하자.
리팩토링
우선 같은 클래스에있던 기능을 util로 빼버렸다.
public class CustomPageable {
static public <T> List<T> sliceList(List<T> list, int offset, int limit) {
List<T> resultList = new ArrayList<T>();
int start = (offset)*limit;
int end = Math.min(start + limit, list.size());
for(int i = start; i < end; i++) {
resultList.add(list.get(i));
}
return resultList;
}
}
그리고 중복 이름을 제거하는 부분도 나눌 여러 메서드에서 공통적으로 사용할 기능이기에 새로 빼버리고 스트림을 사용하는 형식으로 더 깔끔하게 만들어버렸다.
중복 이름 제거 로직
public static List<ProductDto> convertToUniqueNameProductList(List<Product> productList) {
ModelMapper modelMapper = new ModelMapper();
return productList.stream()
.collect(Collectors.toMap(
Product::getProductTitle,
product -> modelMapper.map(product, ProductDto.class),
(existing, replacement) -> existing,
LinkedHashMap::new
))
.values()
.stream()
.collect(Collectors.toList());
}
전략 인터페이스
public interface ProductSearchStrategy {
List<ProductDto> getFilteredProductList(ProductFilterDto productFilterDto, int offset, int limit);
}
전략1
첫 번째 전략을 아래와 같은 형태로 완성하였다.
public class ProductSelectZeroStrategy implements ProductSearchStrategy{
private final ProductRepository productRepository;
private final ModelMapper modelMapper;
@Autowired
public ProductSelectZeroStrategy(ProductRepository productRepository, ModelMapper modelMapper) {
this.productRepository = productRepository;
this.modelMapper = modelMapper;
}
@Override
public List<ProductDto> getFilteredProductList(ProductFilterDto productFilterDto, int offset, int limit) {
List<Product> productEntityList = productRepository
.getProductDtoListByFilter(
productFilterDto.getSearchingTerm(),
productFilterDto.getEventNumber(),
productFilterDto.getSelect(),
PageRequest.of(offset, limit));//페이징 offset limit
System.out.println(productEntityList);
/* 중복 이름 처리, 중복 이름이 존재하면 아예 보여주지 않도록 함*/
List<Product> uniqueTitleProductList = ProductUtil.convertToUniqueNameValue(productEntityList);
//entityList to dtoList
List<ProductDto> productDtoList = uniqueTitleProductList
.stream()
.map(user -> modelMapper.map(user, ProductDto.class))
.collect(Collectors.toList());
return productDtoList;
}
}본래 코드와 비교해 훨씬 깔끔해진 모습이다. 두 번째 전략인 select가 1일때 역시 같은 코드를 사용한다. 현재는 로직상 완전히 작동하는 형태가 동일하기 때문이다.
전략3
public class ProductSelectThreeStrategy implements ProductSearchStrategy{
private final ProductRepository productRepository;
private final ModelMapper modelMapper;
private final ProductDao productDao;
public ProductSelectThreeStrategy(ProductRepository productRepository, ProductDao productDao, ModelMapper modelMapper) {
this.productRepository = productRepository;
this.modelMapper = modelMapper;
this.productDao = productDao;
}
@Override
public List<ProductDto> getFilteredProductList(ProductFilterDto productFilterDto, int offset, int limit) {
List<Product> productEntityList = productRepository
.getProductDtoListByFilter(
productFilterDto.getSearchingTerm(),
productFilterDto.getEventNumber(),
productFilterDto.getSelect(),
PageRequest.of(offset, limit));//페이징 offset limit
//order count 추가
List<ProductWithOrderCountDto> productWithOrderCount = new ArrayList<>();
for(Product product: productEntityList) {
int orderCount = productDao.getOrderNumberByProductName(product.getProductTitle());
ProductWithOrderCountDto pod = modelMapper.map(product, ProductWithOrderCountDto.class);
pod.setOrderCount(orderCount);
productWithOrderCount.add(pod);
}
//정렬
Collections.sort(productWithOrderCount, (pre, post) -> Integer.compare(post.getOrderCount(), pre.getOrderCount()));
/* 중복 이름 처리, 중복 이름이 존재하면 아예 보여주지 않도록 함*/
List<ProductDto> uniqueTitleProductList = ProductUtil.convertToUniqueNameProductDtoList_order(productWithOrderCount);
return CustomPageable.sliceList(uniqueTitleProductList, offset, limit);
}
}
전략 선택 Context
@Component
public class GetSearchedProductDtoListByFilter {
private ProductSearchStrategy productSearchStrategy;
public List<ProductDto> getProductListByFilter(ProductFilterDto productFilterDto, int offset, int limit){
return productSearchStrategy.getFilteredProductList(productFilterDto, offset, limit);
}
public void setFilterStrategy(ProductSearchStrategy productSearchStrategy){
this.productSearchStrategy = productSearchStrategy;
}
}
비교
달라진 점을 비교한다. 비교 대상이 service와 dao가 되었지만, 핵심 로직을 관리하는 코드가 service로 변한 것이기에 로직의 핵심 주체로서 비교 대상이 될 거라 생각한다.
이전
@Override
public List<ProductDto> getProductDtoListByFilter(ProductFilterDto productFilterDto, int offset, int limit) {
if(productFilterDto.getSelect() == 3) {
List<Product> productEntityList = productRepository
.getProductDtoListByFilterNotPagable(
productFilterDto.getSearchingTerm(),
productFilterDto.getEventNumber());
List<ProductWithOrderCountDto> productWithOrderCount = new ArrayList<>();
for(Product product: productEntityList) {
int orderCount = getOrderNumberByProductName(product.getProductTitle());
ProductWithOrderCountDto pod = new ProductWithOrderCountDto();
pod.setProductId(product.getProductId());
pod.setPriceNumber(product.getPriceNumber());
pod.setPriceString(product.getPriceString());
pod.setProductTitle(product.getProductTitle());
pod.setProductExpirationDate(product.getProductExpirationDate());
pod.setProductType(product.getProductType());
pod.setProductImgUrl(product.getProductImgUrl());
pod.setProductEvent(product.getProductEvent());
pod.setCreatedAt(product.getCreatedAt());
pod.setProductTimeSale(product.getProductTimeSale());
pod.setOrderCount(orderCount);
productWithOrderCount.add(pod);
}
orderByOrderNumber(productWithOrderCount);
/* 중복 이름 처리, 중복 이름이 존재하면 아예 보여주지 않도록 함*/
List<ProductDto> dtoList = new ArrayList();
for(ProductWithOrderCountDto product:productWithOrderCount) {
boolean isContain = false;
for(ProductDto dto:dtoList) {
String dtoProductName = dto.getProductTitle();
String entityProductName = product.getProductTitle();
if(dtoProductName.equals(entityProductName)) {
isContain = true;
break;
}
}
if(isContain) continue;
ProductDto dto = new ProductDto();
dto.setProductId(product.getProductId());
dto.setPriceNumber(product.getPriceNumber());
dto.setPriceString(product.getPriceString());
dto.setProductTitle(product.getProductTitle());
dto.setProductExpirationDate(product.getProductExpirationDate());
dto.setProductType(product.getProductType());
dto.setProductImgUrl(product.getProductImgUrl());
dto.setProductEvent(product.getProductEvent());
dto.setCreatedAt(product.getCreatedAt());
dto.setProductTimeSale(product.getProductTimeSale());
dtoList.add(dto);
}
return sliceList(dtoList, offset, limit);
}
else {
List<Product> productEntityList = productRepository
.getProductDtoListByFilter(
productFilterDto.getSearchingTerm(),
productFilterDto.getEventNumber(),
productFilterDto.getSelect(),
PageRequest.of(offset, limit));//페이징 offset limit
System.out.println(productEntityList);
/* 중복 이름 처리, 중복 이름이 존재하면 아예 보여주지 않도록 함*/
List<ProductDto> dtoList = new ArrayList();
for(Product product:productEntityList) {
boolean isContain = false;
for(ProductDto dto:dtoList) {
String dtoProductName = dto.getProductTitle();
String entityProductName = product.getProductTitle();
if(dtoProductName.equals(entityProductName)) {
isContain = true;
break;
}
}
if(isContain) continue;
ProductDto dto = new ProductDto();
dto.setProductId(product.getProductId());
dto.setPriceNumber(product.getPriceNumber());
dto.setPriceString(product.getPriceString());
dto.setProductTitle(product.getProductTitle());
dto.setProductExpirationDate(product.getProductExpirationDate());
dto.setProductType(product.getProductType());
dto.setProductImgUrl(product.getProductImgUrl());
dto.setProductEvent(product.getProductEvent());
dto.setCreatedAt(product.getCreatedAt());
dto.setProductTimeSale(product.getProductTimeSale());
dtoList.add(dto);
}
return dtoList;
}
}
이후
@Override
public List<ProductDto> getProductDtoListByFilter(ProductFilterDto productFilterDto, int offset, int limit) {
//List<ProductDto> productDtoList = productDao.getProductDtoListByFilter(productFilterDto, offset, limit);
GetSearchedProductDtoListByFilter context = new GetSearchedProductDtoListByFilter();
switch (productFilterDto.getSelect()) {
case 0: {
ProductSearchStrategy strategy = new ProductSelectZeroStrategy(productRepository, moddelMapper);
context.setFilterStrategy(strategy);
return context.getProductListByFilter(productFilterDto, offset, limit);
}
case 1: {
ProductSearchStrategy strategy = new ProductSelectOneStrategy(productRepository, moddelMapper);
context.setFilterStrategy(strategy);
return context.getProductListByFilter(productFilterDto, offset, limit);
}
case 2: {
ProductSearchStrategy strategy = new ProductSelectTwoStrategy(productRepository, moddelMapper);
context.setFilterStrategy(strategy);
return context.getProductListByFilter(productFilterDto, offset, limit);
}
case 3: {
ProductSearchStrategy strategy = new ProductSelectThreeStrategy(productRepository, productDao, moddelMapper);
context.setFilterStrategy(strategy);
return context.getProductListByFilter(productFilterDto, offset, limit);
}
default:
throw new IllegalArgumentException("Unexpected select value: " + productFilterDto.getSelect());
}
}결론적으로 코드 자체는 길어졌지만, 각 전략별로 수정 및 추가가 필요할 때 유연한 구조를 가질 수 있게 되었으며, 실제 사용하는 부분의 경우 눈에 띄게 보기 좋아졌다는 것을 알 수 있다.
댓글