[Review] 2022/04/22 2차 PR (반찬 서비스)

이번 리뷰는 wooody 께서 해주셨다! 리뷰해주셔서 감사합니다 !!

[Team-12 BE] 1주차 두번째 PR (2022/4/22) by geombong · Pull Request #106 · codesquad-members-2022/sidedish

 

1. 질문

1) 요구사항에 없는 아이템 등록(상품 등록) 같은 기능 또한 고려하여 DB 설계를 진행해야 하는지 궁금합니다.

 

2) 다음처럼 3개의 분리되어 있던 URL을 맨 아래 1개의 URL로 표현하였는데, 둘 중 어떤게 표현하는게 좀 더 바람직한지 궁금합니다.

GET api/categories/main/{pageId} : 메인 4개 조회
GET /api/categories/sidedish/{pageId} : 사이드 4개 조회
GET /api/categories/soup/{pageId} : 국물 4개 조회

VS

GET /api/categories/{type}?pageId={pageId}

다음과 같은 답변을 주셨다!

나같은 경우 데이터모델링은 거의 초짜라... 좀 한 테이블 안에 다 추가하는 경우가 많은 것 같다.

일단은 요구사항부터 만족시키고, 돌아가게 해야 한다는 생각이 강한것 같다.

다음 프로젝트 부터는 JPA를 사용하니, 좀더 모델링에 시간을 투자 해야겠다!

 

URL 또한 카테고리가 동적으로 늘어날 수 있다는 점에서 type을 경로 변수로 받도록 수정한점이 더 좋다는 답변을 얻게 되었다!

 

2. 코드 리뷰

1. SQL 스키마의 Comment 기능

처음알게된 기능이다... DB에도 comment 기능이 있었다니! 완전 꿀팁인것 같다!

 

2. 동일한 API path

생각해보니 그렇다!!

DetailController, CategoryController 모두 @RequestMapping("/api/categories") 에서 URL 이 시작하는데, 둘이 다른 Class 에 있게 되었었다.

 

getItemsByCategory 메서드는 "/api/categories/{type}" 이고

getItemByDetailType 메서드는 "/api/categories/{type}/detail" 이였다.

이를 다른 class에 분리시켰던 것 이다. 

 

하나의 CategoryController 내부로 이동시키게 되었다!

 

3. 경로 URL에 대한 고민

위와 같이 원래는 detail?type={type}과 같이 인자를 전달받고 있었다. 쿼리 파라미터를 사용중 이였다.

하지만 wooody가 추천해주신 방식처럼 중간에 {type}을 경로표현식으로 삽입해주는 방식이 더 적합하다 생각되었다.

따라서 추천해주신 방식으로 변경하게 되었다.

 

4. 의미가 좀더 명확한 함수 이름

원래 해당 메서드의 역할은 Type에 해당되는 items 들을 가져오는 코드 였다.

하지만 리뷰어님의 말씀처럼 Categories를 가져올거 같은 이상한 이름으로 되어 있었다.

페어인 검봉과 같이 만들때는 느끼지 못했는데, 다시 보니 이상한? 이름이였다.

 

메서드의 행동에 부합하도록 이름을 다음으로 변경하게 되었다.

getItemsByCategory

 

5. 범위 밖의 page 요청에 대한 검증

지금 우리의 컨트롤러는 1이상의 자연수에 대해서만 정상 작동하고 있다.

0 또는 -1이 나오는 경우를 방지하기 위해 예외를 던지도록 변경하였다.

public List<Item> findUnitPageById(CategoryType type, Long pageId, int pageCount) {
    Long categoryId = categoryRepository.findCategoeyId(type);
    int startPage = pageId.intValue() - 1;
    if (startPage < 0) {
        throw new NoSuchElementException(NOT_FOUND_PAGE_EXCEPTION);
    }
    PageRequest pageable = PageRequest.of(startPage, pageCount);
    return itemRepository.findByCategory(categoryId, pageable);
}

startPage가 0보다 작은 경우 NoSuchElementException을 발생시키도록 수정하였다.

 

6. BeanValidatoin 검증하기

사실 우리의 원래 의도는,

Enum이 대문자로 matching 되기 때문에 대문자가 들어오는 경우 바로 바인딩 되고,

소문자로 들어오는 경우 대문자로 변경하여 해당 Enum에 바인딩 하도록 변경하는 것 이였다.

 

리뷰어님께서는 validation 적용을 고민하라 하셨는데 음....

대, 소 문자 상관없이 대문자로만 변환시켜주면 되기 때문에 겸증이 꼭 필요할까? 라는 생각이 들었다.

또한 DTO를 사용하여 request를 받는것 또한 아니기 때문에 BeanValidation을 적용할수도 없다.

 

7. Enum 타입의 Value 하드 코딩 문제

DB에 저장되는 AUTO_INCREMENT를 예측해서 하드코딩 해버렸다.

차라리 각 Type에 value값을 주지 말고, 다음처럼 그냥 Type을 가지고 검색하도록 변경하였다.

public enum CategoryType {
    MAIN, SOUP, SIDE;
}

DB에서 해당 type으로 PK값을 찾아 온 후, 해당 PK값을 통해 검색하도록 변경하였다.