Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

5주차 과제- 고양이 장난감가게 만들기 (심화) #75

Merged
merged 40 commits into from Sep 12, 2022

Conversation

jdalma
Copy link

@jdalma jdalma commented Aug 29, 2022

고양이 장난감가게 유효성 검사 추가하기

고양이 장난감은 이름, 메이커 그리고 가격은 필수 입력 항목입니다.
이 중에서 하나라도 빠트린 경우에는 처리를 거부하고 웹에게 에러 응답을 전달해야 합니다.
그래야 웹에서 잘못된 경우를 사용자에게 피드백을 줄 수 있습니다.
유효성을 검사하여 올바른 에러 응답을 만들어주세요.

  • 고양이 장난감 목록 얻기 - GET /products
  • 고양이 장난감 상세 조회하기 - GET /products/{id}
  • 고양이 장난감 등록하기 - POST /products
  • 고양이 장난감 수정하기 - PATCH /products/{id}
  • 고양이 장난감 삭제하기 - DELETE /products/{id}

회원 관리하기

회원 생성, 수정, 삭제하기 REST API를 만들어주세요.
회원은 이름, 이메일, 비밀번호는 필수 입력 항목입니다.
만약 잘못된 정보로 회원이 만들어지지 않도록 유효성 검사를 하고 올바른 에러를 웹에게 응답할 수 있도록 만들어 주세요.

  • 회원 생성하기 - POST /user
  • 회원 수정하기 - POST /user/{id}
  • 회원 삭제하기 - DELETE /user/{id}

한 주간 잘 부탁드립니다 ㅎㅎ

Copy link
Collaborator

@johngrib johngrib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

github을 사용하고 있으니 PR 본문이나 커밋 메시지에서 @를 사용할 때 주의해야 할 필요가 있어요.

image

이렇게 다른 github 사용자가 링크되고, 알림도 가게 되거든요. 그러므로 다음과 같이 `을 양쪽에 써서 이스케이프 시켜줘야 합니다.

`@johngrib`

this.productRepository = productRepository;
}

/**
* 모든 상품들을 반환한다
* @return List<Product>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리턴 타입은 메소드 시그니처에 이미 나와 있으니 여기에서는 생략해도 괜찮습니다.

Suggested change
* @return List<Product>

Comment on lines 19 to 20
@NoArgsConstructor
@AllArgsConstructor
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 두 lombok 애노테이션은 가급적이면 사용하지 않도록 합시다. 한편 @AllArgsConstructor는 위험한 특성이 있어서 사용을 아예 금지하는 회사들도 많습니다. 왜 그런지에 대해 조사해 보세요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정리 해보았습니다
lombok 애노테이션을 별 생각 없이 사용하였는데 .. 반성하게 되네요 ㅎㅎ

Comment on lines 131 to 151
@Nested
@DisplayName("delete 메서드는")
class Describe_Delete{

@Nested
@DisplayName("{id}에 해당하는 자원이 존재한다면")
class Context_ExistedId{

private final Long id = 1L;
private User user;

@BeforeEach
void setUp() {
}

@Test
@DisplayName("삭제 후 삭제한 정보를 반환한다")
void It_Delete(){
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

테스트 코드를 잊지 않고 만들기 위해 이렇게 준비를 해두셨군요. 좋습니다.
보통 이렇게 미리 테스트를 만들어 두는 경우에는 잊거나 빠뜨리지 않게 일부러 테스트를 실패하게 만들곤 합니다.

Suggested change
@Nested
@DisplayName("delete 메서드는")
class Describe_Delete{
@Nested
@DisplayName("{id}에 해당하는 자원이 존재한다면")
class Context_ExistedId{
private final Long id = 1L;
private User user;
@BeforeEach
void setUp() {
}
@Test
@DisplayName("삭제 후 삭제한 정보를 반환한다")
void It_Delete(){
}
}
}
@Nested
@DisplayName("delete 메서드는")
class Describe_Delete{
@Nested
@DisplayName("{id}에 해당하는 자원이 존재한다면")
class Context_ExistedId{
private final Long id = 1L;
private User user;
@BeforeEach
void setUp() {
}
@Test
@DisplayName("삭제 후 삭제한 정보를 반환한다")
void It_Delete(){
fail("미구현된 테스트입니다.");
}
}
}

@JsonProperty(access = JsonProperty.Access.WRITE_ONLY)
private String password;

// @Builder를 위한 private 생성자
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스 시그니처 위에 @Builder를 선언하지 않고 생성자 바로 위에 @Builder를 선언하는 경우의 장점에 대해 고민해 보시고, 조사도 해 보세요.

Suggested change
// @Builder를 위한 private 생성자
@Builder

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

종립님의 글 참고하여 정리해 봤습니다 ㅎㅎ

@Slf4j
@RestController
@RequestMapping("/user")
public class UserController {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨트롤러 클래스의 모든 public 메소드에는 꼭 JavaDoc 주석을 작성해 보세요.

어떻게 작성해야 할 지 잘 모르겠다면 이 글을 참고해 보세요.
https://johngrib.github.io/wiki/java/javadoc/

Comment on lines 43 to 46
private User findUser(Long id) {
return repository.findById(id)
.orElseThrow(() -> new ResourceNotFoundException(id));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 예외를 던지는 메소드는 꼭 JavaDoc 주석을 작성하시고 그 중에서 @throws를 반드시 명시해 주세요.

Comment on lines 20 to 28
@Transactional(readOnly = true)
public List<User> findAll(){
return repository.findAll();
}

@Transactional
public User save(User user){
return repository.save(user);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘 읽어보면 이 서비스 내에 조회용 메소드와 변경용 메소드가 함께 모여 있다는 것을 알 수 있습니다.
조회용 메소드만 모아서 UserQueryService 클래스로 만들고, 변경/저장용 메소드만 모아서 UserCommandService 클래스로 만들어 보세요. 그리고 이렇게 하는 것이 어떤 장단점이 있는지에 대해서도 생각해 보세요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

목적책임이 분리되니 로직이 더 복잡해진다면 확장하거나 수정할 때 더 편할 것 같다는 생각이 들었습니다.
하지만

  1. 컨트롤러가 여러 개의 도메인 서비스를 주입받는다면 번거로워지지 않을까?
  2. 컨트롤러는 간단하면 좋겠는데 아래와 같이 컨트롤러가 두 개의 서비스를 같이 호출하는것은 조금 이상하다

라고 생각했습니다

    @PostMapping("{id}")
    public User update(@PathVariable Long id , @RequestBody User user){
        return command.update(query.findUser(id) , user);
    }

    @DeleteMapping("{id}")
    public User delete(@PathVariable Long id){
        return command.delete(query.findUser(id));
    }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

컨트롤러는 경우에 따라 여러 개의 서비스를 주입받기도 합니다. 서비스 하나가 여러 개의 컨트롤러에 주입되는 경우도 있죠. 컨트롤러와 서비스의 관계가 1:1 로만 작동해야만 하는 상황이 아니라면 1:1 관계가 바람직하지 않은 경우도 많습니다.

public Product getProduct(Long id) {
return findProduct(id);
}

/**
* 상품의 DTO를 받아 매퍼를 통해 엔티티로 변환 후 저장한다.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

메소드 안쪽의 메커니즘을 너무 자세하게 설명하고 있어요. 지금은 "매퍼를 통해 엔티티로 변환"하고 있는데 만약 메소드 안쪽의 구현을 다른 방법으로 바꾸게 된다면 주석도 수정해야 할 것입니다. 이건 주석이 구현에 의존하고 있는 거라 할 수 있어요. 메커니즘을 수정해도 주석을 수정할 필요가 없게 만들려면 이 메소드의 목적을 짧고 명료하게 설명해주면 됩니다. 주석을 어떻게 수정해야 좋을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

식별자에 해당하는 상품을 수정한다.라고 수정해 보았습니다

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다!

import org.springframework.web.bind.annotation.*;

import javax.validation.Valid;
import java.util.List;

/**
* 상품을 조회 , 생성 , 삭제한다.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

콤마 앞에 있는 스페이스는 삭제하는 것이 좋을 것 같아요.

Comment on lines +66 to +67
* @throws MethodArgumentNotValidException 필수 정보가 비어있는 경우
* @throws ResourceNotFoundException 식별자에 해당하는 상품이 없는 경우
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋습니다.

@@ -9,38 +9,20 @@
import java.util.List;

@Service
public class UserService {
@Transactional(readOnly = true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QueryService 에 잘 어울리는 애노테이션입니다.

import org.springframework.web.bind.annotation.*;

import javax.validation.Valid;
import java.util.List;

/**
* 상품을 조회,생성,삭제한다.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

앗 이 정도가 적절하지 않을까요 😅

Suggested change
* 상품을 조회,생성,삭제한다.
* 상품을 조회, 생성, 삭제한다.

Comment on lines 96 to 98
@Nested
@DisplayName("필수 입력 검증을 통과하지 못 한다면")
class Context_NotPassValidation{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

필수 입력 검증을 통과하지 못하는 케이스를 다양하게 세분화할 수 있을 것 같습니다.

}

@Test
@DisplayName("삭제한다.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게도 할 수 있겠죠.

Suggested change
@DisplayName("삭제한다.")
@DisplayName("삭제를 수행하고, 삭제했음을 의미하는 응답을 돌려준다.")

하지만 제가 제안한 것보다 더 나은 방법도 있을 거에요. NoContent의 의미를 어떻게 구체적으로 표현할지에 대해서도 고민해 보세요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

응답 코드를 명시하고 싶지만 응답 코드가 바뀐다면 설명도 바뀌어야 할 것 같네요
테스트 코드의 설명도 JavaDoc 같이 수정에 유연하게 작성해야 할까 궁금하네요
실제로 구현 로직이 바뀐다면 테스트 코드를 먼저 수정하게 될테니 JavaDoc보다 자세하게 작성해도 되지 않을까 싶기도 합니다 ㅎㅎ
일단 응답 코드 설명을 추가해보도록 하겠습니다

this.name = u.name;
this.email = u.email;
this.password = u.password;
if(u.name != null && !"".equals(u.name)){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if 문 안쪽의 코드가 3번 반복되는군요. 메소드로 추출해보는 것을 고려해보세요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils.hasText메서드로 수정해봤습니다

Comment on lines +67 to +69
// DozerMapper
// DTO와 Domain을 매핑해주는 역할
implementation 'com.github.dozermapper:dozer-core:6.4.0'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dozermapper github repository에 방문해 보세요. 어떤 문제가 있다는 것을 알 수 있을 텐데, 어떤 문제인지 확인해서 글로 작성해 보세요. dozermapper 를 제거해야 할 지에 대해서도 생각해 보시고, 제거한다면 어떤 대안이 있는지도 글에 함께 적어 보세요.

Comment on lines +29 to +31
public User delete(User user){
repository.delete(user);
return user;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 삭제가 실패했다면 실패했다는 사실을 호출자에게 어떤 식으로 알릴 수 있을까요? 지금은 User를 그대로 리턴하고만 있기 때문에 알 수 없을 것입니다.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Controller에서 query.findUser가 식별자에 해당하는 정보가 없으면 예외를 던지게 되어 있습니다

@DeleteMapping("{id}")
@ResponseStatus(HttpStatus.NO_CONTENT)
public void delete(@PathVariable Long id){
    command.delete(query.findUser(id));
}

어떤 상황이 더 있을까요??

Copy link
Collaborator

@johngrib johngrib Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예외를 던진다면 delete 메소드 위에 JavaDoc으로 @throws를 작성해 주도록 합시다. 한 번에 알아볼 수 있도록 해주는 것이 필요해요. 한편 findUser가 예외를 던지도록 하는 것이 과연 바람직한 것인지에 대해서도 생각할 수 있겠죠. 만약 검색엔진에서 어떤 것을 검색했는데 "검색 결과가 0 입니다" 라고 결과가 나오는 것과 예외가 던져지는 것을 두고 상상해볼 수도 있을 겁니다. 지금은 예외를 Goto 처럼 쓰고 있는 셈인데 (이것이 꼭 나쁘다는 것은 아닙니다) 어떻게 해야 코드를 읽는 사람에게 빨리 파악할 수 있도록 도와줄 수 있는지도 고민해야 해요

@Getter
@NoArgsConstructor
@ToString
@EqualsAndHashCode
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lombok을 사용하지 않고 equals 와 hashCode 메소드를 직접 구현한다고 생각해 봅시다. 어떤 점에 신경써야 할까요?

Copy link
Author

@jdalma jdalma Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어떤 필드들을 포함해야할지 신경써야 할 것 같습니다.

@EqualsAndHashCode(exclude = {})
@EqualsAndHashCode.Exclude private Long id;
제외 필드를 직접 지정할 수 있는 어노테이션도 보게 되었습니다

Copy link
Collaborator

@johngrib johngrib left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

한 주간 고생 많으셨습니다. 주간 회고를 작성해 공유해 주세요.

@johngrib johngrib merged commit 3277ab2 into CodeSoom:jdalma Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants