[1차 프로젝트] 코드리뷰 피드백 반영하기

오늘은 각자 구현했던 코드들에 대해 소개하고 나머지 조원들이 피드백해주는 시간을 가졌다. 그때 정리하면 좋을 것 같은 내용이 있어 정리해보고자한다.

✅ 피드백 1: Entity에 setter보다는 별도의 의미있는 메서드를 선언해 사용하자.

✍️ 내가 구현한 코드

@Transactional
public ProductResponse editProduct(String productId, ProductRequest productRequest) {
    Product product = productRepository.findById(UUIDUtil.hexToBytes(productId))
            .orElseThrow(() -> new ProductNotFoundException("없는 상품입니다."));

    // 내부에 별도의 이름을 가진 메서드를 선언
    if(productRequest.getProductName() != null) product.setProductName(productRequest.getProductName());
    if(productRequest.getCategory() != null) product.setCategory(productRequest.getCategory());
    if(productRequest.getPrice() != null) product.setPrice(productRequest.getPrice());
    if(productRequest.getDescription() != null) product.setDescription(productRequest.getDescription());

    // 수정시간 업데이트
    product.setUpdatedAt(LocalDateTime.now());

    return ProductMapper.toResponse(product);
}

수정 과정에서 수정된 값을 setter로 넣어주어 처리했다.

 

🤔 피드백

하지만 Entity에는 setter를 지양하는 것이 좋다는 피드백을 받았다.

그 이유는 크게 두 가지이다.

 

1. 객체의 불변성 및 일관성 저해

setter를 허용하게 되면, setter는 public 이기 때문에 외부에서 내가 예상치 못한 상태 변경이 발생할 수 있다.

 

2. 사용 의도를 파악하기 힘들다.

if(productRequest.getProductName() != null) product.setProductName(productRequest.getProductName());
if(productRequest.getCategory() != null) product.setCategory(productRequest.getCategory());
if(productRequest.getPrice() != null) product.setPrice(productRequest.getPrice());
if(productRequest.getDescription() != null) product.setDescription(productRequest.getDescription());

단순히 코드가 이렇게 작성되어 있다면 어떤의도로 상태를 변경하는지 그 의도를 파악하기 힘들어진다.

객체 내에 값이 더 많아진다면 더더욱 알아보기 힘들어질 것이다.

따라서 두 가지 이유를 잘 유념해 코드를 수정해보겠다.

 

🤩 코드 수정

1️⃣ Entity 클래스에 Setter 제거

Lombok 라이브러리를 사용하고 있어서 기존에 추가했던 @Setter 어노테이션을 주석처리하였다.

@Entity(name = "products")
@Getter 
//@Setter
@NoArgsConstructor @AllArgsConstructor
public class Product {

    @Id
    private byte[] productId;

    @Column(length = 20, nullable = false)
    private String productName;

    @Column(length = 50, nullable = false)
    private String category;

    @Column(nullable = false)
    private Long price;

    @Column(length = 500)
    private String description;

    @Column(nullable = false)
    private LocalDateTime createdAt;

    private LocalDateTime updatedAt;

}

 

2️⃣ 의도가 확실한 메서드 작성하기

Product 내부에 updateProduct() 메서드 작성

public void updateProduct(String productName, String category, Long price, String description) {
	  if(productName != null) this.productName = productName;
	  if(category != null) this.category = category;
	  if(price != null) this.price = price;
	  if(description != null) this.description = description;
	
	  // 수정시간 업데이트
	  this.updatedAt = LocalDateTime.now();
}

 

서비스 계층에선 해당 메서드를 호출만함

@Transactional
public ProductResponse editProduct(String productId, ProductRequest productRequest) {
    Product product = productRepository.findById(UUIDUtil.hexToBytes(productId))
            .orElseThrow(() -> new ProductNotFoundException("없는 상품입니다."));

    // 내부에 별도의 이름을 가진 메서드를 선언
    product.updateProduct(
            productRequest.getProductName(),
            productRequest.getCategory(),
            productRequest.getPrice(),
            productRequest.getDescription()
    );

    return ProductMapper.toResponse(product);
}

 

📄 참고자료

왜 Entity에 setter를 사용하지 말아야 할까?

 

 

✅ 피드백 2: 예외 처리를 조금 더 객체 지향적으로 해라

✍️ 내가 구현한 코드

비즈니스 예외를 적절한 응답으로 처리하기 위해 다음과 같은 ExceptionHandler를 선언해주었다.

@RestControllerAdvice
public class GlobalExceptionHandler {

    @ExceptionHandler({ProductNotFoundException.class, OrderNotFoundException.class})
    public ResponseEntity<ErrorDTO> notFoundException(RuntimeException e) {
        ErrorDTO errorDTO = new ErrorDTO(
                HttpStatus.NOT_FOUND.value(),
                ProductNotFoundException.class.toString(),
                e.getMessage(),
                LocalDateTime.now()
        );

        return new ResponseEntity<>(errorDTO, HttpStatus.NOT_FOUND);
    }

    @ExceptionHandler(EmailMismatchForOrderException.class)
    public ResponseEntity<ErrorDTO> emailMismatchForOrderException(EmailMismatchForOrderException e) {
        ErrorDTO errorDTO = new ErrorDTO(
                HttpStatus.BAD_REQUEST.value(),
                ProductNotFoundException.class.toString(),
                e.getMessage(),
                LocalDateTime.now()
        );

        return new ResponseEntity<>(errorDTO, HttpStatus.BAD_REQUEST);
    }
}

발생하는 예외를 에러코드 별로 분리해서 ErrorDTO라는 형태로 응답해줬다.

 

중복 코드 발생

ErrorDTO errorDTO = new ErrorDTO(
        HttpStatus.NOT_FOUND.value(),
        ProductNotFoundException.class.toString(),
        e.getMessage(),
        LocalDateTime.now()
);

return new ResponseEntity<>(errorDTO, HttpStatus.NOT_FOUND);

에러에 대한 객체를 생성 부분이 반복된다.

이는 예외마다 보내줘야 하는 상태 코드가 다르기 때문이다.

이러한 부분을 일괄적으로 처리해야 할 필요가 있다.

 

🤔 피드백

1️⃣ ErrorCode에 대한 Enum을 만들어라

@Getter
public enum ErrorCode {
    PRODUCT_NOT_FOUND(HttpStatus.NOT_FOUND, "Product not found"),
    ORDER_NOT_FOUND(HttpStatus.NOT_FOUND, "Order not found"),
    EMAIL_MISMATCH(HttpStatus.BAD_REQUEST, "Email mismatch for order");

    private final HttpStatus status;
    private final String message;

    ErrorCode(HttpStatus status, String message) {
        this.status = status;
        this.message = message;
    }
}

Enum을 통해 에러메시지나 상태 코드를 더 일관성 있게 관리하고 재사용성을 높여준다.

이를 통해 코드 중복을 줄이고 유지보수를 쉽게 할 수 있다.

 

2️⃣ 커스텀 예외에 대한 부모클래스를 만들어라

조금 더 객체지향적인 설계를 위해 CustomException이라는 부모 예외를 선언했다.

해당 예외는 내부에 ErrorCode를 필드로 가진다.

@Getter
@RequiredArgsConstructor
public class CustomException extends RuntimeException{
    private final ErrorCode errorCode;
}

 

이후 만들었던 예외들이 CustomException을 상속받게 한다.

public class ProductNotFoundException extends CustomException {
    public ProductNotFoundException() {
        super(ErrorCode.PRODUCT_NOT_FOUND);
    }
}

public class OrderNotFoundException extends CustomException {
    public OrderNotFoundException() {
        super(ErrorCode.ORDER_NOT_FOUND);
    }
}

public class EmailMismatchForOrderException extends CustomException {
    public EmailMismatchForOrderException() {
        super(ErrorCode.EMAIL_MISMATCH);
    }
}

 

3️⃣ GlobalExceptionHandler 수정

수정 전

@RestControllerAdvice
public class GlobalExceptionHandler {

    @ExceptionHandler({ProductNotFoundException.class, OrderNotFoundException.class})
    public ResponseEntity<ErrorDTO> notFoundException(RuntimeException e) {
        ErrorDTO errorDTO = new ErrorDTO(
                HttpStatus.NOT_FOUND.value(),
                ProductNotFoundException.class.toString(),
                e.getMessage(),
                LocalDateTime.now()
        );

        return new ResponseEntity<>(errorDTO, HttpStatus.NOT_FOUND);
    }

    @ExceptionHandler(EmailMismatchForOrderException.class)
    public ResponseEntity<ErrorDTO> emailMismatchForOrderException(EmailMismatchForOrderException e) {
        ErrorDTO errorDTO = new ErrorDTO(
                HttpStatus.BAD_REQUEST.value(),
                ProductNotFoundException.class.toString(),
                e.getMessage(),
                LocalDateTime.now()
        );

        return new ResponseEntity<>(errorDTO, HttpStatus.BAD_REQUEST);
    }
}

 

수정 후

@RestControllerAdvice
public class GlobalExceptionHandler {

    @ExceptionHandler(CustomException.class)
    public ResponseEntity<ErrorDTO> notFoundException(CustomException e) {
        ErrorCode errorCode = e.getErrorCode();
        ErrorDTO errorDTO = new ErrorDTO(
               errorCode.getStatus().value(),
                errorCode.name(),
                errorCode.getMessage(),
                LocalDateTime.now()
        );

        return new ResponseEntity<>(errorDTO, errorCode.getStatus());
    }
}

중복코드가 제거되었다.

 

🤔 추가 고민사항

추후 도메인이 복잡해져 예외가 상당히 많아진다면 ErrorCode Enum 하나로 모든 예외를 처리하기에는 무리가 있을 것이다.

따라서 도메인별로 Enum을 나누는 것을 고려할 수 있다.

하지만 Enum에선 직접적인 상속이 불가능하다.

따라서 별도의 인터페이스를 정의한 뒤, 해당 인터페이스를 구현하는식으로 해야한다.

 

1️⃣ 공통 인터페이스 정의

public interface ErrorCode {
    HttpStatus getStatus();
    String getMessage();
}

 

2️⃣ 도메인별 Enum 구현

public enum ProductErrorCode implements ErrorCode {
    PRODUCT_NOT_FOUND(HttpStatus.NOT_FOUND, "Product not found");

    private final HttpStatus status;
    private final String message;

    ProductErrorCode(HttpStatus status, String message) {
        this.status = status;
        this.message = message;
    }

    @Override
    public HttpStatus getStatus() {
        return status;
    }

    @Override
    public String getMessage() {
        return message;
    }
}

public enum OrderErrorCode implements ErrorCode {
    ORDER_NOT_FOUND(HttpStatus.NOT_FOUND, "Order not found"),
    EMAIL_MISMATCH(HttpStatus.BAD_REQUEST, "Email mismatch");

    private final HttpStatus status;
    private final String message;

    OrderErrorCode(HttpStatus status, String message) {
        this.status = status;
        this.message = message;
    }

    @Override
    public HttpStatus getStatus() {
        return status;
    }

    @Override
    public String getMessage() {
        return message;
    }
}

 

3️⃣  Exception 수정

public class ProductNotFoundException extends CustomException {
    public ProductNotFoundException() {
        super(ProductErrorCode.PRODUCT_NOT_FOUND);
    }
}

public class OrderNotFoundException extends CustomException {
    public OrderNotFoundException() {
        super(OrderErrorCode.ORDER_NOT_FOUND);
    }
}

public class EmailMismatchForOrderException extends CustomException {
    public EmailMismatchForOrderException() {
        super(OrderErrorCode.EMAIL_MISMATCH);
    }
}