[Review] 스프링 카페 2단계 - 글 쓰기 기능 구현 (2022/03/08)

 

코드 리뷰

이번 시간에는 브라이언이 리뷰를 해주셨다!

[Shine] 스프링 카페 2단계 - 글 쓰기 기능 구현 by zbqmgldjfh · Pull Request #24 · codesquad-members-2022/java-spr

다만 피드백 해주신 점을 빠르게 수정한 후, 팀원들의 피드백 또한 정리해 봐야겠다.

 

질문 1

저는 Article을 Form으로 부터 넘겨받을때 다음 필드에 대하여만 입력받게 합니다.

    private String writer;
    private String title;
    private String contents;

하지만 저장할때 필요한 Article의 필드는 다음과 같습니다.

    private Long id;
    private String writer;
    private String title;
    private String contents;
    private LocalDateTime localDateTime;

즉, 사용자가 넘기는 값과 저장하는대 필요한 값에 차이가 생기게 됩니다.
따라서 다음과 같은 DTO를 만들어 사용자가 전달한 Form을 받게 하였습니다.

public class ArticleDto {
    private String writer;
    private String title;
    private String contents;

   // 생성자, getter 생략
}

이후 이를 컨트롤러에서 다음과 같이 변환하여 저장하였습니다.

    @PostMapping
    public String saveForm(@ModelAttribute ArticleDto dto) {
        Article article = new Article(dto.getWriter(), dto.getTitle(), dto.getContents(), LocalDateTime.now());
        articleService.addArticle(article);
        return "redirect:/";
    }

이와 같이 DTO를 사용해도 되는 것 일까요? 이전 까지의 리뷰들을 좀 봤을때 이유없이 DTO를 사용하는것은 좋지 못한것 같아 사용하지 않았는데, 이번 에는 DTO가 Form을 받아오는 용도로 사용하면 좋지 않을까? 하여 사용하게 되었습니다.

 

위와 같이 질문을 남기게 되었다.

브라이언은 "당연히 모델 클래스와 유저에게 받는 요청은 그 형식과 내용이 다를 수 있다고 생각하고요." 라고 답변해 주셨다.

다만 이 답변이 DTO를 사용해도 되느냐? 에 대한 질문에 "Yes"라고 답해주신 것 같다.

또한 커맨드 객체라 부를수도 있다.

---

1. 객체의 생성은 어디서?

원래 게시글에 대하여 사용자가 정보를 입력 한 후 Form으로 전달하면, 이를 Controller에서 받아 생성을 하고 있었다.

다음과 같이 말이다.

@PostMapping
public String saveForm(@ModelAttribute ArticleDto dto) {
    Article article = new Article(dto.getWriter(), dto.getTitle(), dto.getContents(), LocalDateTime.now());
    articleService.addArticle(article);
    log.info("save form = {}", article.getTitle());
    return "redirect:/";
}

이에 대하여 다음과 같은 피드백을 남겨주셨다.

생각해보니 생성을 하는 역할은 비지니스 layer에서 해주는 것 이 더 적합하다 생각하였다.

따라서 다음과 같이 Service에게 함수의 인자로 전달하여 생성하도록 변경하였다.

@PostMapping
public String saveForm(@Validated @ModelAttribute("article") ArticleDto dto, BindingResult bindingResult) {

    if (bindingResult.hasErrors()) {
        log.info("saveForm Validation had Errors");
        return "/qna/form";
    }

    articleService.addArticle(dto); // Service에게 인자로 전달
    log.info("save form = {}", dto.getTitle());
    return "redirect:/";
}

(ps. 추가로 검증 로직이 생겨 BindingResult를 사용하게 되었다.)

 

2. ifPresent() 로 변경하기

@Override
public void update(Long id, Article article) {
//      if (findById(id).isPresent()) {
//          Article find = findById(id).get();
//          find.setTitle(article.getTitle());
//          find.setContents(article.getContents());
//      }
    findById(id).ifPresent(findArticle -> findArticle.updateArticle(article.getTitle(), article.getContents()));
}

---

팀원들이 받은 리뷰 내용 정리

 

3. Controller의 이름

위 컨트롤러의 메서드의 이름은 createQuestion이다. 즉, 이름만 봐서는 게시물(Question)을 만들어야 할 것 같다.

하지만 정작 하는 일은 사용자에게 /qna/form.html 을 보여주는 역할을 한다.

 

내가 생각한 적합한 이름은 getQuestionForm() 정도가 될 것 같다? 

 

4. 묻지 말고 시켜라

좋은 리뷰인것 같다. 나 또한 위와같이 짠 기억이 있어 상기시킬겸 정리한다.

 

위 코드에서는 getter로 stream상의 Article의 id를 가져온 후, 다시 이 id를 인자로 받은 id와 비교하고 있다.

꼭 article의 id를 getter로 끌어올 필요가 있었느냐? 그렇지 않다. 다음과 같이 "묻지 말고 시켜라" 방식이 더 적합하다.

articles.stream().filter(article -> article.isSameId(articleId))

isSameId() 라는 메서드를 통해 article에게 시킬 수 있다.

 

5. Test 코드의 하드 코딩

음 MockMvc 로 요청을 받아온 내용을 검증하는 부분인데, 이 부분에 상수가 아닌 "하드코딩"을 권장하셨다.

내가 알기로는 상수로 분리할 경우 해당 Unit테스트에서 테스트 부분과 맨위 상수가 선언된 부분을 왔다갔다를 반복해야 한다.

이런 면에서 상수보다는 "하드코딩"이 더 한눈에 파악하기 좋다는 글을 봤었다.

 

6. Inner Class 의 사용

위 ArticleSetUp은 특정 Test에서만 필요한 요소이다.

따라서 위와같이 하나의 bean으로 만들어 사용하기 보다는, static Inner class를 잘 활용하면 해당 Test에서만 사용하는 class임을 좀더 명확하게 전달할 수 있을 것 같다.

 

7. 메서드의 의미표현

리뷰어의 말 처럼 단순하게 formatter.format(new Date()) 보다는, static factory method를 만들어 이름을 부여하면 생성할때 의미가 더 명확해진다는 장점이 있다.