[Numble] Spring으로 타임딜 서버 구축 - 클린코드를 포함한 리팩토링 (TOP3 선정!)

 

처음 참가한 넘블 벡엔드 챌린지에서 무려 TOP3 안에 들게 되었습니다!!

👏👏👏👏👏

 

 

감사하게도 처음으로 현직자 분에게 코드리뷰를 받게 되었고
코드리뷰 받은 것과 클린코드(좋은 코드)를 토대로 리팩토링을 할 것입니다.

 

총평에 대한 코멘트

• 블로그와 깃허브 read me 칭찬을 받았습니다!
  • llsrrll96/numble-timedeal-server: 넘블챌린지-Spring으로 타임딜 서버 구축하기 (github.com)
• 성능테스트에서 TPS 개선은 미흡했습니다 ㅠ
  • 아직 테스트 지표를 보고 성능 개선에 대한 지식이 부족: 책이나 강의를 보아야겠습니다.
• API 명세를 노션으로 정리해 두었습니다.
  • Swagger 를 사용하여 문서화를 한 경험이 있었으니 이번 챌린지에는 적용하지는 않았습니다.
• projects 를 활용한 것이 인상적이지만 일정도 관리하면 좋을 것 같습니다.
  • 깃허브 프로젝틀에서 일정을 관리할 수 있는 데, 사용법이 아직 미숙합니다.

• PR에 대한 CI/CD가 없습니다.
  • CI/CD를 Master push 에 대해서만 적용을 시켰습니다.
  • 다음에 CI/CD를 해볼 기회가 생긴다면 PR에 대해서 적용시켜보겠습니다.
    • Github PR시 Jenkins를 통한 빌드 자동화 (tistory.com)

 

코드 리뷰 코멘트 

• phase별로 application.yml 파일을 나누어 관리한 점이 매우 좋음. 다만 비밀번호 등 민감한 정보가 커밋되는건 위험하니 다른 방안을 검토필요
  • Spring Cloud Config Server를 구축하여 코드와 설정파일을 분리하는 방법
  • 환경변수를 사용하여 설정파일에는 환경변수명으로 대체하는 방법
• package구조가 깔끔하지만 레이어링 기준으로 나눌지, 도메인 기준으로 나눌지 검토(모듈로 추출하기 용이해짐)
  • 레이어링 기준(controller, service, repository)
  • 도메인으로 나눌 때에도 [user > (controller, service, repository)], 도메인으로 나누는 연습을 해볼 겁니다.
• "상품등록성공" 같은 메세지들은 message properties를 활용하면 관리가 더 용이할것 같습니다.
  • 클린 코드와 관련된 이야기로 하드 코딩은 유지보수에 취약하다, 그래서 Constant 인터페이스 방법과 Enum 등이 있는데, 찾아본 결과 Enum 사용을 지향하고 있고 위치는 해당 클래스를 사용하는 패키지에 위치하면 좋다는 답변을 받았습니다.

기존코드

@PutMapping("/v1/{productid}")
private ResponseEntity<APIMessage<RespProduct>> updateProduct(@PathVariable Long productid, @RequestBody ReqProduct reqProduct){
    if(isAdminUser(reqProduct.getUserId())) {
        return new ResponseEntity<>(new APIMessage<>(HttpStatus.OK.toString(),"상품수정성공",productService.updateProduct(productid, reqProduct)),HttpStatus.OK);
    }
    return new ResponseEntity<>(new APIMessage<>(HttpStatus.BAD_REQUEST.toString(), "상품수정실패",new RespProduct()),HttpStatus.BAD_REQUEST);
}

 

리팩토링

@PutMapping("/v1/{productid}")
private ResponseEntity<APIMessage<RespProduct>> updateProduct(@PathVariable Long productid, @RequestBody ReqProduct reqProduct){
    if(isAdminUser(reqProduct.getUserId())) {
        return new ResponseEntity<>(new APIMessage<>(HttpStatus.OK.toString(),ProductResponseMessage.UPDATING_PRODUCT_SUCCESS.getRespDesc(), productService.updateProduct(productid, reqProduct)),HttpStatus.OK);
    }
    return new ResponseEntity<>(new APIMessage<>(HttpStatus.BAD_REQUEST.toString(), ProductResponseMessage.UPDATING_PRODUCT_FAILED.getRespDesc(), new RespProduct()),HttpStatus.BAD_REQUEST);
}

 

public enum ProductResponseMessage {
    SAVING_PRODUCT_SUCCESS("상품등록성공"),
    SAVING_PRODUCT_FAILED("상품등록실패"),
    UPDATING_PRODUCT_SUCCESS("상품수정성공"),
    UPDATING_PRODUCT_FAILED("상품등록실패"),
    DELETING_PRODUCT_SUCCESS("상품삭제성공"),
    DELETING_PRODUCT_FAILED("상품삭제실패"),
    FINDING_PRODUCTS_SUCCESS("상품리스트조회"),
    FINDING_PRODUCT_SUCCESS("상품조회"),
    SAVING_CATEGORY_SUCCESS("카테고리 등록"),
    DELETING_CATEGORY_SUCCESS("카테고리 삭제 성공"),
    DELETING_CATEGORY_FAILED("카테고리 삭제 실패")
    ;

    private final String respDesc;

    ProductResponseMessage(String respDesc) {
        this.respDesc = respDesc;
    }

    public String getRespDesc(){
        return respDesc;
    }
}

 

• 컨트롤러에 버저닝을 사용하신 점이 인상적이네요. 공통 매핑은 상위로 올려도 좋을것 같습니다.
  
  • @RequestMapping 으로 공통 부분을 묶을 예정입니다.
• 공통적인 응답 모델은 클라이언트와 협의하기 꽤 좋은 모델이라고 합니다.

@Data
@NoArgsConstructor
@AllArgsConstructor
public class APIMessage<T> {
    private String status;
    private String message;
    private T data;
}

 

• 자바 컨벤션을 하나로 맞추기!
  • 언더스코어,, 스네이크 표기법을 사용했었는데, 

public class RespTimedeal {
    private Long timedealId;
    private ProductDto product;
    private int limitedAmount;
    private int sale_price;
    private LocalDateTime startDatetime;
}

• 응답 데이터를 스네이크 표기법으로 보여주기로 합의 했다면,
  
  • @JsonProperty를 사용하여 다음과 같이 해결하겠습니다.

@JsonProperty("sale_price")
private int salePrice;

 

• 또한 언더바가 포함된 변수명을 받을 경우
  • 기본적으로 Jackson 라이브러리를 기본적으로 사용하여 언더바를 카멜표기법으로 변환

@Data
public class ReqTimedeal{
    private Long product_id;
    private int limited_amount;
    private int sale_price;

 

변경

@Data
public class ReqTimedeal{
    @JsonProperty("product_id")
    private Long productId;
    @JsonProperty("limited_amount")
    private int limitedAmount;
    @JsonProperty("sale_price")
    private int salePrice;
    @JsonProperty("start_datetime")
    @JsonDeserialize(using = LocalDateTimeDeserializer.class)
    @JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyyMMddHHmm")
    private LocalDateTime startDatetime;
}

언더바 변수명 입력에도 카멜표기법으로 변환 

 

• 공통으로 추출할 수 있는 메서드는 리팩토링 할 수 있을것으로 보이고, if문에 길게 작성되는 조건문 또한 마찬가지 입니다.

if(!isOpenTimeForTimedeal(timedeal) || timedeal.getLimitedAmount()-reqPurchase.getCount() < 0) return false;

• 
  
  • 해당 부분을 함수로 처리할 예정입니다.

 

• 리턴을 할 경우 null로 리턴하는 것보다 Optional을 감싸서 리턴하는게 안전해 보입니다.

    public String findUserIdByNickname(String nickname) {
        UserEntity userEntity = userRepository.findByNickname(nickname);
        if(userEntity == null){
            return null;
        }
        return userEntity.getUserId();
    }

• • DB접근했다면 예외처리, Optional을 통해 예외처리를 하는 것이 좋다고 합니다.

 

---

 

 

리팩토링

 

1. 리패키징: 레이어링 기준을 도메인 기준으로 나누어 보려합니다.

현재 레이어링 기준과 도메인 기준이 혼재된 모습

domain (category, product, purchase, timedeal, user)

총 5개 나누어 모듈로 추출하기 좋도록 바꾸려고 합니다.

 

 

 

 

 

 

 

 

 

도메인 기준으로 재패키징 하였습니다.

User, product, timedeal, purchase로 구분하고

각 도메인 내에 controller 등 레이어 별로 나누었습니다.

공통으로 사용하는 APIMessage는 외부로 두었고

공통으로 사용할 수 있는 BaseEntity는 util에 두었습니다.

 

 

 

 

 

 

 

 

 

 

 

2. 컨트롤러의 공통 매핑은 상위로 올립니다.

 

기존

@PutMapping("/v1/products/{productid}")

리팩토링

 

 

공통적인 products를 @RequestMapping 으로 설정했습니다.

@RequestMapping(value = "products")

 

 

3. 반복되는 부분 함수로 묶기

 

기존 2번이상 반복

if(userService.checkUserRole(reqProduct.getUserId(), UserEnum.ROLE_ADMIN))

 

리팩토링

// 함수화
private boolean isAdminUser(String userId){
    return userService.checkUserRole(userId, UserEnum.ROLE_ADMIN);
}

// 사용
if(isAdminUser(reqProduct.getUserId()))

 

 

 

4. 와일드카드 보다는 타입 명시

// 기존
ResponseEntity<?>

// 변경
ResponseEntity<APIMessage<List<RespProduct>>>

 

 

5. if 조건문의 조건도 함수로 표현하고 한번에 묶어 표현

기존

if(!isOpenTimeForTimedeal(timedeal) || timedeal.getLimitedAmount()-reqPurchase.getCount() < 0) return false;

 

리팩토링

if(isOpenTimeForTimedeal(timedeal) && validatePurchaseQuantity(timedeal, reqPurchase)) {
    ...
}
return false;

 

 

6. DB접근했다면 예외처리, Optional을 사용

기존

    public String findUserIdByNickname(String nickname) {
        UserEntity userEntity = userRepository.findByNickname(nickname);
        if(userEntity == null){
            return null;
        }
        return userEntity.getUserId();
    }

 

리팩토링

public String findUserIdByNickname(String nickname) {
    UserEntity userEntity = userRepository.findByNickname(nickname).orElseThrow();
    return userEntity.getUserId();
}

 

 

7. Spring Boot에서 컨트롤러에서 상품 등록과 같은 작업이 실패한 경우에는 다양한 HttpStatus 중에서 적절한 상태 코드를 선택할 수 있습니다.

 

일반적으로는 다음과 같은 HttpStatus를 사용할 수 있습니다.

 

7.1. HttpStatus.BAD_REQUEST (400): 클라이언트가 잘못된 요청을 보낸 경우, 예를 들면 유효성 검사에 실패한 경우 등에 사용될 수 있습니다. 클라이언트에게 잘못된 요청임을 알려줄 때 사용할 수 있습니다.

@PostMapping("/products")
public ResponseEntity<String> createProduct(@RequestBody ProductDto productDto) {
    // 상품 등록 작업을 수행하고 실패한 경우
    if (상품_등록_실패) {
        // 실패한 경우 HttpStatus.BAD_REQUEST를 사용하여 클라이언트에게 잘못된 요청임을 알립니다.
        return ResponseEntity.badRequest().body("상품 등록에 실패하였습니다.");
    }
    
    // 상품 등록 작업을 성공한 경우
    return ResponseEntity.ok("상품이 성공적으로 등록되었습니다.");
}

 

7.2. HttpStatus.INTERNAL_SERVER_ERROR (500): 서버 내부에서 예기치 못한 오류가 발생한 경우에 사용될 수 있습니다. 예를 들면 데이터베이스 연결 실패, 서버 내부 로직 오류 등이 있습니다.

 

@PostMapping("/products")
public ResponseEntity<String> createProduct(@RequestBody ProductDto productDto) {
    try {
        // 상품 등록 작업을 수행하고 실패한 경우
        if (상품_등록_실패) {
            // 실패한 경우 HttpStatus.INTERNAL_SERVER_ERROR를 사용하여 서버 내부 오류임을 알립니다.
            return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body("상품 등록에 실패하였습니다.");
        }
        
        // 상품 등록 작업을 성공한 경우
        return ResponseEntity.ok("상품이 성공적으로 등록되었습니다.");
    } catch (Exception e) {
        // 예기치 못한 오류가 발생한 경우 HttpStatus.INTERNAL_SERVER_ERROR를 사용하여 서버 내부 오류임을 알립니다.
        return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body("상품 등록 중 오류가 발생하였습니다.");
    }
}