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

[이호석] DAY4 Controller, Service, Repository 구분하기(완료) #3

Merged
merged 16 commits into from Mar 20, 2023

Conversation

HiiWee
Copy link
Member

@HiiWee HiiWee commented Mar 16, 2023

To Reviewer

안녕하세요 모그님!
간단한 요구사항이었지만 이전에 말씀해주신 것처럼 난이도를 조금 올려서 이것저것 해보려고 시도해봤습니다!
코드를 작성하면서 가장 중점을 두었던 부분은 중복되는 코드의 제거에 대한 일반화와 책임의 분리에 많은 신경을 썼던 것 같습니다!
(티가 날지는 모르겠어요..)

  • Controller layer에선 제어의 역할에 중점을 두었고, 오로지 dto에만 의존하도록 구성했습니다.
  • Service layer는 비즈니스 로직이 위치하고, dto -> domain, domain -> dto의 작업과 예외 처리에 대한 부분을 모아 두었습니다.
  • Repository layer는 거의 단순 CRUD만 있고, ID에 대한 생성을 책임지고 있습니다. (ex: Auto Increment)

메모리 DB에 대한 Test

메모리를 사용한 DB를 사용하다 보니 테스트 코드에서 어려움이 많았는데요, Rollback이 되지 않아 테스트끼리 서로 영향이 미치는 부분에 대해서는 서로 다른 데이터를 이용한다는 것과 같이 임시 조치를 취했습니다.

Service 계층의 아쉬움

제가 작성한 코드를 볼 때마다 아쉽다고 느껴지는 부분입니다.
제공하는 API 혹은 비즈니스 로직에 비해 Service 계층의 코드가 너무 번잡하다는 생각이 듭니다.
메인 비즈니스 로직을 도메인으로 옮기려 시도도 해봤지만, 크게 진전이 없었습니다.

블로그 링크

[스터디] Controller, Service, Repository 구분하기(4회차)

@HiiWee HiiWee requested a review from mog-hi March 16, 2023 17:35
@HiiWee HiiWee self-assigned this Mar 16, 2023
Comment on lines 37 to 51
/**
* TODO: 테스트하기 어려운 코드
*/
public ProductListResponse findAll(@Nullable final String name, @Nullable final String monetaryUnit) {
if (name == null) {
List<Product> products = productRepository.findAll();
return new ProductListResponse(createConvertedPriceProducts(monetaryUnit, products));
}
List<Product> products = productRepository.findAllByName(name);
if (products.isEmpty()) {
throw new ProductNotFoundException();
}
return new ProductListResponse(createConvertedPriceProducts(monetaryUnit, products));
}

Copy link
Member Author

@HiiWee HiiWee Mar 16, 2023

Choose a reason for hiding this comment

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

@mog-hi

응답값에 대한 테스트가 어려운것 같습니다!

일급 컬렉션을 이용해보려 시도했습니다. 하지만 반환된 ProductListResponse에 대한 테스트를 작성하는것이 어려웠습니다.

내부 상태를 알기 위해 메소드를 추가하는 방법도 있지만 오로지 테스트를 위해 로직을 추가하는것은 옳지 않다고 생각해 따로 추가하진 않았습니다.
이런 경우에는 어떤 방식이 좋을지 궁금합니다!

Copy link
Member

Choose a reason for hiding this comment

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

이런 시도 너무 좋습니다~! ProductListResponse 객체 자체를 만들어서 리턴 값과 비교하거나, contains 함수 하나를 구현해서 이 Product가 여기에 포함되어 있다. 정도로 구현할 수 있을 것 같아요!

name별로 찾아주는 함수를 ProductListResponse내부에서 구현하면 테스트도 더 쉬울 것 같기도 하고요. 아래 링크 참고해주세요!
https://tecoble.techcourse.co.kr/post/2020-05-08-First-Class-Collection/

Copy link
Member Author

Choose a reason for hiding this comment

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

말씀해주신 글을 읽어보고 contains 메소드를 통해 검증할 수 있도록 구현했습니다!

글을 읽고, 일급 컬렉션을 사용하고자 했을때 사용해서 얻는 이점과 왜 사용했어야할까? 보다는 단순히 컬렉션을 반환하니 일급 컬렉션을 써야겠다고 생각하고 사용한것 같습니다!

일급 컬렉션을 왜 써야하는지, 사용하면서 얻는 이점을 공유해주신 글을 통해 다시 한번 상기시킬 수 있었습니다. 감사합니다!

Copy link
Member

@mog-hi mog-hi left a comment

Choose a reason for hiding this comment

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

수고하셨습니다. 고민 많이 하셨네요!!
널리널리 공유하겠습니다 ㅎㅎㅎ

this.id = id;
}

public boolean contains(final Long id) {
Copy link
Member

Choose a reason for hiding this comment

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

isSameName과 같은 기능을 하는 메소드인데, 이름이 달라서 좀 헷갈리는 것 같아요!

Copy link
Member Author

Choose a reason for hiding this comment

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

확실히 동일한 기능의 메소드지만 타인이 봤을때 다른 기능을 하는 메소드로 보일 수 있을것 같습니다!
같은 기능에 대한 메소드를 이름을 통해 구분지을 수 있도록 변경했습니다!


import com.jscode.spring.advice.BadRequestException;

public class DuplicateNameException extends BadRequestException {
Copy link
Member

Choose a reason for hiding this comment

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

👍👍👍

* TODO: 테스트하기 어려운 코드
*/
public ProductListResponse findAll(@Nullable final String name, @Nullable final String monetaryUnit) {
if (name == null) {
Copy link
Member

Choose a reason for hiding this comment

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

여기도 repository에서 처럼 if문으로 나누는 것보다 findAllByName으로 메소드를 구분해주는 것도 좋을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

findAll 메소드가 사실 두 가지 기능을 담당하는것 같아 보이네요! 분리하여 컨트롤러 레이어에서 구분하여 사용할 수 있도록 변경했습니다!

Comment on lines 37 to 51
/**
* TODO: 테스트하기 어려운 코드
*/
public ProductListResponse findAll(@Nullable final String name, @Nullable final String monetaryUnit) {
if (name == null) {
List<Product> products = productRepository.findAll();
return new ProductListResponse(createConvertedPriceProducts(monetaryUnit, products));
}
List<Product> products = productRepository.findAllByName(name);
if (products.isEmpty()) {
throw new ProductNotFoundException();
}
return new ProductListResponse(createConvertedPriceProducts(monetaryUnit, products));
}

Copy link
Member

Choose a reason for hiding this comment

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

이런 시도 너무 좋습니다~! ProductListResponse 객체 자체를 만들어서 리턴 값과 비교하거나, contains 함수 하나를 구현해서 이 Product가 여기에 포함되어 있다. 정도로 구현할 수 있을 것 같아요!

name별로 찾아주는 함수를 ProductListResponse내부에서 구현하면 테스트도 더 쉬울 것 같기도 하고요. 아래 링크 참고해주세요!
https://tecoble.techcourse.co.kr/post/2020-05-08-First-Class-Collection/

/**
* (미션 3 ) 조회/등록 실패할 때 응답 interface 구현
*/
@RestControllerAdvice
Copy link
Member

Choose a reason for hiding this comment

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

와우 여기까지!! 👏👏👏


class ProductRepositoryTest {

ProductRepository productRepository;
Copy link
Member

Choose a reason for hiding this comment

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

테스트 코드를 BDD패턴 등을 이용해서 케이스별로 나눠보면 좋을 것 같아요~!

Copy link
Member Author

Choose a reason for hiding this comment

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

BDD 패턴을 공부해보고 테스트 케이스를 차차 변경해보겠습니다 감사합니다!

@HiiWee HiiWee merged commit ccc1dc2 into main Mar 20, 2023
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