프로젝트 코드 리팩토링

 

 

 

지난 6개월간 혼자했던 프로젝트를 멘토님과 함께 리팩토링을 진행하면서 어떤 부분을 어떻게 리팩토링하고자 했는지 전반적인 정리를 해보고자 한다. 

리팩토링은 하나의 PR당 하나의 객체를 주제로 하고, 레이어별로 리팩토링 및 테스트 코드 수정의 방식으로 진행했다. 

 

 

 

 

 

 

자바 코드 컨벤션, 객체지향 생활체조, 케이스 스타일 지키고 리팩토링 학습

가장 먼저 지켜야 할 기본적 규칙들에 대해 학습하고 이를 지켜서 코드를 구현하고자 했다. 블로그에 글을 작성하여 학습을 진행했고 추가적으로 리팩토링 책의 예제부분을 공부하면서 리팩토링을 해야하는 이유 및 방법에 대해 학습하였다. 또한 코드를 작성할 때는 method reference및 stream을 이용하여 간단하고 명료하게 의도를 나타낼 수 있도록 작성했다. 

https://dodop-blog.tistory.com/262

Java Code Convention ( + Google Java Style Guide)

https://dodop-blog.tistory.com/263

객체지향 생활 체조

https://dodop-blog.tistory.com/274

케이스 스타일 (Camel case, Kebab case, Snake case, Pascal case)

https://dodop-blog.tistory.com/269

Refactoring ① : 첫번째 예제

https://dodop-blog.tistory.com/283

Java 8 Stream

https://dodop-blog.tistory.com/271

Method Reference (Java 8)

 

 

예외처리전략 적용

예외처리 전략(GlobalExceptionHandler)가 적용되도록 하였다. 해당내용은 다음의 블로그 내용을 참고한다. 

https://dodop-blog.tistory.com/229

예외처리전략 (Exception Handler)

https://dodop-blog.tistory.com/282

예외처리전략 ( ExceptionHandler ) 2

 

 

 

 

REST API 방식 반영 

가장 먼저 restAPI방식을 따르도록 url 수정이 이루어지도록 진행했다. 

https://dodop-blog.tistory.com/272

RESTful API

기존의 코드를 보면 다음과 같이 동사 명사 구분없이 url에 마음대로 사용하고 자원 또한 단수로 작성하고 있는 모습을 보인다. 🤦‍♀️

@RestController
@RequiredArgsConstructor
public class ActivityController {

    private final ActivityService activityService;

    @PostMapping("/activity/save")
    public ActivityCreateDTO saveActivity(@RequestParam(value = "id", required = false) Integer id,
        @RequestParam("name") String name,
        @RequestParam("score") Integer score,
        @RequestParam("description") String description,
        @RequestParam(value = "multipartFile", required = false) MultipartFile multipartFile) {

        ActivityCreateDTO activityCreateDTO = new ActivityCreateDTO(id, name, score, description);
        return activityService.saveActivity(activityCreateDTO, multipartFile);
    }
}

 

이 부분을 restAPI를 사용하여 행위에 대한 부분은 method 로 자원에 대한 부분은 복수체제로 나타내도록 변경하였다. 

@RestController
@RequestMapping("/activities")
public class ActivityController {

    private final ActivityService activityService;

    public ActivityController(ActivityService activityService) {
        this.activityService = activityService;
    }

    @PostMapping
    public ActivityResponse save(@Valid @RequestBody ActivityRequest activityRequest) {
        return activityService.create(activityRequest);
    }

}

 

 

 

목적에 따른 메소드 분리 

위에서 보듯이 기존의 코드를  보면 save라는 메소드를 이용하여 null체크를 false로 두고 create와 update기능을 한번에 하려고 하고있다. 이 방식은 행위의 목적이 분명하게 하지 못하고 controller에서 null체크도 하지 못하도록 하고 있으므로 이부분을 수정하여 create, update 메소드로 분리하도록 하였다.

또한 이미지는 액티비티를 업로드할 때 필수항목이 아니었기 때문에 액티비티 객체와 라이프 스타일을 달리 하여 required=false 옵션을 사용했는데 이렇게 라이프 스타일이 다른 대상은 따로 빼내어 업로드 기능을 만들어주고 controller에서 null체크를 담당할 수 있도록 변경하였다. 

@RestController
@RequestMapping("/activities")
public class ActivityController {

    private final ActivityService activityService;

    public ActivityController(ActivityService activityService) {
        this.activityService = activityService;
    }

    @PostMapping
    public ActivityResponse create(@Valid @RequestBody ActivityRequest activityRequest) {
        return activityService.create(activityRequest);
    }

    @PutMapping("/{id}")
    public ActivityResponse update(@PathVariable("id") Integer id,
        @Valid @RequestBody ActivityRequest activityRequest) {
        return activityService.update(id, activityRequest);
    }

    @PostMapping("/images")
    public String uploadImage(@RequestParam("multipartFile") MultipartFile multipartFile)
        throws IOException {
        return activityService.uploadImage(multipartFile);
    }
}

 

 

DTO 목적에 따라 분리 

DTO의 경우 이전 코드를 확인하면 request와 response에 사용되는 것을 구분없이 사용했기 때문에 id 부분의 경우 포함되는 부분, 포함되지 않는 부분을 따로 생성자를 생성하고 구분없이 사용하고 있었다.  다음의 예시로 보여지는 ActivityCreateDTO는 액티비티 객체를 생성할때, 업데이트할 때, 액티비티 정보를 돌려줄때 모두 사용하고 있었다. 만약, RestAPI를 제대로 적용했었다면 update시에도 url로 객체 구분자를 넣어주었을 것이기 때문에 이 부분을 필요없는 부분이었을 것이다. 

@Getter
@Setter
public class ActivityCreateDTO {

    private Integer id;
    private String name;
    private Integer score;
    private String description;
    private String imageUrl;

    public ActivityCreateDTO(Integer id, String name, Integer score, String description) {
        this.id = id;
        this.name = name;
        this.score = score;
        this.description = description;
    }

    public ActivityCreateDTO(Activity activity) {
        this.id = activity.getId();
        this.name = activity.getName();
        this.score = activity.getScore();
        this.description = activity.getDescription();
        this.imageUrl = activity.getImageUrl();
    }

    public ActivityCreateDTO(String name, Integer score, String description) {
        this.name = name;
        this.score = score;
        this.description = description;
    }


}

 

 

해당 DTO는 다음과 같이 request용과 response용을 달리하여 controller에서 체크가 가능하도록 변경해주었다. 또한 빌더패턴을 이용하여 생성자를 과다하게 작성할 필요 없이 꼭 필요한 부분은 @NonNull을 이용하여 필수값으로 지정하도록 적용하였다. 추가로 toActivity메소드를 이용하여 request -> 실제 객체 부분을 request에서 담당하도록 해주었다. 

@Getter
@Setter
public class ActivityRequest {

    @NotNull
    private String name;

    @NotNull
    private Integer score;

    @NotNull
    private String description;

    private String imageUrl;

    @Builder
    public ActivityRequest(String name, Integer score, String description, String imageUrl) {
        this.name = name;
        this.score = score;
        this.description = description;
        this.imageUrl = imageUrl;
    }

    public Activity toActivity() {
        return new Activity(name, score, description, imageUrl);
    }

}


@Getter
@Setter
public class ActivityResponse {

    private Integer id;
    private String name;
    private Integer score;
    private String description;
    private String imageUrl;

    public ActivityResponse(Activity activity) {
        this.id = activity.getId();
        this.name = activity.getName();
        this.score = activity.getScore();
        this.description = activity.getDescription();
        this.imageUrl = activity.getImageUrl();
    }

}

 

 

비지니스로직을 도메인으로 옮기기

기존의 코드에서는 핵심 비지니스 로직을 모두 서비스 클래스 내에서 담당하고 있기 때문에 객체는 단순히 데이터 덩어리의 역할만 하게 된다. 또한 이로 인해 서비스는 복잡해질수록 더 많은 객체를 읽어 들이려고 하기 때문에 로직의 복잡도가 증가하게 되고 유지보수와 테스트가 힘든 코드로 변질되게 된다. 

또한 다음의 코드는 save라는 메소드로 update, create 두가지 기능을 수행하고 있어 controller의 null체크 활용도 하지 못하고 복잡도가 더 증가하였다. 

import org.springframework.util.StringUtils;
import org.springframework.web.multipart.MultipartFile;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;

@Service
@RequiredArgsConstructor
public class ActivityService {

    private final ActivityRepository activityRepository;


    private void saveActivityImage(Activity activity, MultipartFile multipartFile, boolean isNew) {
        try {
            String fileName = StringUtils.cleanPath(multipartFile.getOriginalFilename());
            String uploadDir = "activityUploads/" + activity.getId();

            if (!isNew) {
                FileUploadUtils.cleanDir(uploadDir);
            }

            FileUploadUtils.saveFile(uploadDir, fileName, multipartFile);
            activity.setImageUrl("/activityUploads/" + activity.getId() + "/" + fileName);

            activityRepository.save(activity);

        } catch (IOException ex) {
            new IOException("Could not save file : " + multipartFile.getOriginalFilename());
        }
    }

    public ActivityCreateDTO saveActivity(ActivityCreateDTO activityCreateDTO,
        MultipartFile multipartFile) {

        if (activityCreateDTO.getId() != null) {
            Activity existingActivity = activityRepository.findById(activityCreateDTO.getId())
                .get();
            existingActivity.setName(activityCreateDTO.getName());
            existingActivity.setDescription(activityCreateDTO.getDescription());
            existingActivity.setScore(activityCreateDTO.getScore());

            if (multipartFile != null) {
                saveActivityImage(existingActivity, multipartFile, false);
            }
            activityRepository.save(existingActivity);
            return new ActivityCreateDTO(existingActivity);
        } else {
            Activity activity = new Activity();
            activity.setName(activityCreateDTO.getName());
            activity.setDescription(activityCreateDTO.getDescription());
            activity.setScore(activityCreateDTO.getScore());
            activityRepository.save(activity);

            if (multipartFile != null) {
                saveActivityImage(activity, multipartFile, true);
            }
            activityRepository.save(activity);
            return new ActivityCreateDTO(activity);
        }
    }

@Table(name = "activity")
@Getter
@Setter
@NoArgsConstructor
public class Activity {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "activity_id")
    private Integer id;
    @Column(nullable = false)
    private String name;
    @Column(nullable = false)
    private Integer score;
    @Column(nullable = false)
    private String description;
    @Column(name = "image_url")
    private String imageUrl;
    @OneToMany(mappedBy = "activity", cascade = CascadeType.ALL, orphanRemoval = true)
    @OrderBy("createdAt DESC")
    private Set<UserActivity> userActivities = new HashSet<>();

    public Activity(String name, Integer score, String description) {
        this.name = name;
        this.score = score;
        this.description = description;
    }

따라서 다음과 같이 로직을 분리하고 핵심 비지니스 로직은 도메인이 담당하도록 하고 서비스 클래스는 트랜잭션 담당 역할에 집중할 수 있도록 변경해주었다. 

 

도메인 을 보면 기존에 단순 데이터 덩어리 였던 객체를 update, setDefaultImageUrl이라는 비지니스 로직을 담당 할 수 있도록 하였다. 또한 이로 인해 불필요한 setter의 사용을 방지하여 객체가 여기저기서 변경되는 것을 막을 수 있었다. 

https://dodop-blog.tistory.com/265

Setter의 사용 금지

@Table(name = "activity")
@Getter
public class Activity {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "activity_id")
    private Integer id;
    @Column(nullable = false)
    private String name;
    @Column(nullable = false)
    private Integer score;
    @Column(nullable = false)
    private String description;
    @Column(name = "image_url")
    private String imageUrl;
    @OneToMany(mappedBy = "activity", cascade = CascadeType.ALL, orphanRemoval = true)
    @OrderBy("createdAt DESC")
    private Set<UserActivity> userActivities = new HashSet<>();

    public Activity() {
    }

    public Activity(String name, Integer score, String description) {
        this.name = name;
        this.score = score;
        this.description = description;
    }

    public Activity(String name, Integer score, String description, String imageUrl) {
        this.name = name;
        this.score = score;
        this.description = description;
        this.imageUrl = imageUrl;
    }

    // 비지니스 로직
    public Activity update(Activity requestActivity) {
        this.name = requestActivity.name;
        this.description = requestActivity.description;
        this.score = requestActivity.score;
        this.imageUrl = requestActivity.imageUrl;

        return this;
    }

    public void setDefaultImageUrl(String imageUrl, String defaultImageUrl) {
        if (imageUrl == null || imageUrl.isEmpty() || imageUrl.isBlank()) {
            this.imageUrl = defaultImageUrl;
        }
    }


}

서비스 클래스는 트랜잭션을 담당하고 도메인에게 비지니스로직이 수행요청을 보낸다. 이를 통해 유지보수가 가능하고 깔끔한 코드 구현이 가능해졌다. 

@Service
@Transactional
public class ActivityService {

    private ActivityRepository activityRepository;

    private S3ImageUploader s3ImageUploader;

    private static final String UPLOAD_DIR = "activity-uploads";

    @Value("${AWS_S3_BASE_IMAGE_URL}")
    private String defaultImageUrl;

    @Value("${AWS_S3_BUCKET_URL}")
    private String bucketUrl;

    public ActivityService(
        ActivityRepository activityRepository, S3ImageUploader s3ImageUploader) {
        this.activityRepository = activityRepository;
        this.s3ImageUploader = s3ImageUploader;
    }

    public ActivityResponse create(ActivityRequest activityRequest) {
        Activity activity = activityRequest.toActivity();
        activity.setDefaultImageUrl(activityRequest.getImageUrl(), defaultImageUrl);
        activityRepository.save(activity);
        return new ActivityResponse(activity);
    }

    public ActivityResponse update(Integer id, ActivityRequest activityRequest) {
        Activity existingActivity = activityRepository.findById(id)
            .orElseThrow(() -> new ActivityNotFoundException("Activity not found with id : " + id));
        Activity requestActivity = activityRequest.toActivity();
        deleteOriginalImage(requestActivity, existingActivity);
        Activity updatedActivity = existingActivity.update(requestActivity);

        return new ActivityResponse(updatedActivity);
    }
}

 

 

Wrapper클래스 및 Embeddable 타입의 활용

도메인 객체를 보면 기존에 도메인을 분리하지 않아 하나의 도메인이 여러 비지니스 로직을 처리하기 때문에 복잡해지고 무거워지는 모습을 보인다. 

@Entity
@Table(name = "orders")
@Getter
@Setter
@NoArgsConstructor
public class Order extends BaseTimeEntity {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "order_id")
    private Integer id;
    
    @Column(name = "order_status")
    @Enumerated(EnumType.STRING)
    private OrderStatus orderStatus;

    private Float shipping;

    @Column(name = "is_paid")
    private boolean isPaid;

    @Column(name = "payment_method")
    private String paymentMethod;

    @Column(name = "paid_at")
    private LocalDateTime paidAt;

    @Column(name = "is_delivered")
    private boolean isDelivered;

    @Column(name = "delivered_at")
    private LocalDateTime deliveredAt;

    @Embedded
    private Address address;

    @ManyToOne(fetch = FetchType.LAZY)
    @JoinColumn(name = "user_id")
    private User user;

    @OneToMany(mappedBy = "order", cascade = CascadeType.ALL, orphanRemoval = true)
    private Set<OrderItem> orderItems = new HashSet<>();

이 부분을 Wrapper 클래스를 활용하도록 변경해주었다.  요청이 들어오면 Embedded객체에게 요청을 보낸다. 

@Entity
@Table(name = "orders")
@Getter
@NoArgsConstructor
public class Order extends BaseTimeEntity {

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    @Column(name = "order_id")
    private Integer id;
    
    @Column(name = "order_status")
    @Enumerated(EnumType.STRING)
    private OrderStatus orderStatus;

    @Embedded
    private Payment payment;

    @Embedded
    private Delivery delivery;

    private Integer userId;

    @Embedded
    private OrderItems orderItems;
}

 

 

실제 비지니스 로직은 분리된 Embeddable타입의 객체에서 처리하고 관리한다. 

@Embeddable
public class OrderItems {

    @OneToMany(mappedBy = "order", cascade = CascadeType.ALL, orphanRemoval = true)
    private Set<OrderItem> orderItems = new HashSet<>();

    public OrderItems() {
        this.orderItems = new HashSet<>();
    }

    public OrderItems(OrderItem... orderItems) {
        this.orderItems = new HashSet<>(Arrays.asList(orderItems));
    }

    public OrderItems(Set<OrderItem> orderItems) {
        this.orderItems = orderItems;
    }

    public void addOrderItem(OrderItem orderItem) {
        this.orderItems.add(orderItem);
    }

    public void cancelOrder() {
        orderItems.stream()
            .forEach(orderItem -> orderItem.cancel());
    }

    @Transient
    public BigDecimal getTotalAmount() {
        return orderItems.stream()
            .map(OrderItem::getAmount)
            .reduce(BigDecimal.ZERO, BigDecimal::add);
    }

    public Set<OrderItem> getOrderItems() {
        return Collections.unmodifiableSet(orderItems);
    }

}

 

또한 Wrapper 클래스가 아닌 부분도 관련 정보끼리 묶어주어 비지니스로직을 처리할 수 있도록 해준다. 

@Embeddable
@Getter
@NoArgsConstructor
public class Payment {

    private BigDecimal shipping;

    @Column(name = "payment_method")
    private String paymentMethod;

    @Column(name = "paid_at")
    private LocalDateTime paidAt;

    @Column(name = "transaction_id")
    private String transactionId;

    public Payment(BigDecimal shipping, String paymentMethod, String transactionId) {
        this.shipping = shipping;
        this.paymentMethod = paymentMethod;
        this.paidAt = LocalDateTime.now();
        this.transactionId = transactionId;
    }
}

 

 

 

static의 사용 지양

다음의 코드를 보면 fileUpload를 담당하는 클래스에서 메소드가 static 으로 지정되어있었다. 

public class FileUploadUtils {


    public static void saveFile(String uploadDir, String fileName, MultipartFile multipartFile) throws IOException {
    }

    public static void cleanDir(String dir) {
    }
}

하지만 static의 경우 함수의 기능 실행이 아닌 단지 절차지향적 프로그래밍에 불과하며 다형성, 캡슐화를 불가하게 만들고 메모리가 낭비 되는 등 사용을 지양해야 하는 이유가 많으므로 다음과 같이 component로 등록하여 사용하도록 변경해주었다. 

https://dodop-blog.tistory.com/275

자바에서 static의 사용

@Component
public class FileUploadUtils {


    public void saveFile(String uploadDir, String fileName, MultipartFile multipartFile) throws IOException {
    }

    public void cleanDir(String dir) {
    }
}

 

 

 

생성자 주입방식으로 변경 

기존 코드를 보면 Service레이어에서 의존관계를 @autowired 를 통해 주입받고 있기 때문에 테스트 코드에서 @Value값을 읽어오지 못하고 mock을 주입하지 못하지 못하는 등의 문제가 발생하였다. 

@Service
@RequiredArgsConstructor
public class PostService {

    private final PostRepository postRepository;
    private final UserRepository userRepository;
    private final PostImageRepository postImageRepository;
    private final FollowRepository followRepository;
    public static final int POST_PER_PAGE = 9;
    private final S3ImageUploader s3ImageUploader;
    @Value("${AWS_S3_BUCKET_URL}")
    private String AWS_S3_BUCKET_URL;
}

 

다음과 같이 생성자 주입을 통해서 문제를 해결하였다. 

@Service
public class PostService {

    public static final int POST_PER_PAGE = 9;
    private PostRepository postRepository;
    private UserRepository userRepository;
    private PostImageRepository postImageRepository;
    private FollowRepository followRepository;
    private S3ImageUploader s3ImageUploader;
    private String bucketUrl;

    public PostService(PostRepository postRepository,
        UserRepository userRepository,
        PostImageRepository postImageRepository,
        FollowRepository followRepository,
        S3ImageUploader s3ImageUploader,
        @Value("${AWS_S3_BUCKET_URL}") String bucketUrl) {
        this.postRepository = postRepository;
        this.userRepository = userRepository;
        this.postImageRepository = postImageRepository;
        this.followRepository = followRepository;
        this.s3ImageUploader = s3ImageUploader;
        this.bucketUrl = bucketUrl;
    } 
}

 

 

 

인덱스 설계를 이용하여 불필요 요청 최소화

다음의 코드를 보면 다음과 같이 repository에 요청을 보내 최근 2초동안 사용자의 아이디로 주문이 들어왔는지 여부를 통해 중복주문을 확인하는 코드를 구현했다. 

@Service
@Transactional
public class OrderService {

    public OrderResponse createOrder(OrderRequest request) {
        Cart cart = cartService.findCartByUserId(request.getUserId());
        checkOrderValidity(cart, request.getUserId());
        User user = userService.findUserById(request.getUserId());
        Order order = orderRepository.save(request.toOrder());
        Set<OrderItem> orderItems = saveOrderItems(cart, order);
        notificationService.sendCreateOrderNotification(order, user);
        return OrderResponse.of(order,
            UserIconResponse.of(user),
            orderItemService.orderItemResponses(orderItems));
    }

    private void checkOrderValidity(Cart cart, Integer userId) {
        checkOrderDuplicated(userId);
        checkCartEmpty(cart);
    }

    private void checkOrderDuplicated(Integer userId) {
        if (isDuplicated(userId)) {
            throw new OrderDuplicated("Order is duplicated. Please try a few seconds later.");
        }
    }

    private boolean isDuplicated(Integer userId) {
        return orderRepository.existsByCreatedAtBetweenAndUserId(
            LocalDateTime.now().minusSeconds(2),
            LocalDateTime.now(),
            userId);
    }
}

@Repository
public interface OrderRepository extends JpaRepository<Order, Integer> {
    boolean existsByCreatedAtBetweenAndUserId(LocalDateTime from, LocalDateTime to, Integer userId);
}

 

 

해당부분은 timeSeperator라는 column을 이용해서 분단위의 시간을 저장하도록 하고 user_id, time_separator 인덱스 유니크 제약을 통해 불필요한 코드 수행이 이루어지지 않도록 수정하였다. 

@Entity
@Table(name = "orders", indexes = @Index(name = "idx_userId_timeSeparator", columnList = "user_id, time_separator", unique = true))
@Getter
@NoArgsConstructor
public class Order extends BaseTimeEntity {

    @Column(name = "time_separator")
    private String timeSeparator = LocalDateTime.now()
        .format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm"));
}

@Service
@Transactional
public class OrderService {

    public OrderResponse createOrder(OrderRequest request) {
        Cart cart = cartService.findCartByUserId(request.getUserId());
        checkCartEmpty(cart);
        User user = userService.findUserById(request.getUserId());
        // 중복주문 체크 부분이 불필요해짐
        Order order = saveOrder(request);
        Set<OrderItem> orderItems = saveOrderItems(cart, order);
        NotificationMapper.of(user.getNotificationType()).sendCreateOrderNotification(order, user);
        return OrderResponse.of(order,
            UserIconResponse.of(user),
            orderItemService.orderItemResponses(orderItems));
    }
}

 

 

 

 

처리해온 PR들

이렇게 전반적으로 적용된 리팩토링 방식을 정리해보았다. 2021년 12월부터 리팩토링을 진행하면서 틈틈이 블로그에 학습내용을 정리하기도 하고, 처음으로 협업으로 리팩토링을 진행하면서 PR요청시 fileChanges가 너무 많이 (100개 이상😱)보내드려 응답을 늦게 받은 경험도 겪는 등 협업시 주의해야 할 점들을 배울 수 있어 소중한 시간이었다!

 

해당 리팩토링 과정을 보고 싶다면 다음의 링크에서 확인할 수 있다!

https://github.com/projects-workshop/walkerholic/pulls?q=is%3Apr+is%3Aclosed

GitHub - projects-workshop/walkerholic