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

4주차 과제- 고양이 장난감가게 만들기 #76

Merged
merged 33 commits into from Oct 3, 2022

Conversation

jdalma
Copy link

@jdalma jdalma commented Aug 22, 2022

과제 목표

고양이 장난감을 등록하고, 조회하고, 수정하고 삭제하는 기능이 있는 간단한 웹이 있습니다. 이 웹에서 사용할 REST API를 만들어주세요.

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

요구 사항

  • 테스트 커버리지 100%를 달성해야 합니다.
  • 모든 API 테스트를 통과해야 합니다.
  • 모든 E2E 테스트를 통과해야 합니다.

한 주간 잘 부탁드립니다!

import java.util.Optional;

@Service
@Transactional

Choose a reason for hiding this comment

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

Transactional 어노테이션이 붙었군요!
이 기능은 모든 메소드에 필요한 기능일까요?

Copy link
Author

@jdalma jdalma Aug 24, 2022

Choose a reason for hiding this comment

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

내일 조사하고 수정해보겠습니다!


@Transactional(readOnly = true)를 달면 최적화가 된다고만 알고 있었어서 JPA에서 최적화가 어떻게 되는건지 공식 문서에서 찾을려고 했는데.. 못 찾았네요 ㅎㅎ 계속 조사해보겠습니다

Choose a reason for hiding this comment

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

Transactional 어노테이션 관련 공식 도큐먼트를 읽어보는 것이 시작이 될 것 같아요!


public Product findById(Long id){
return repository.findById(id)
.orElseThrow(() -> new ResourceNotFoundException("상품이 존재하지 않습니다. id : " + id));

Choose a reason for hiding this comment

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

도메인에 관련된 예외를 던지고 있다면, 주석에 추가해주는 것이 다른 사람이 이해하는데 큰 도움이 되요!(미래에 나도 포함해서요!)

Copy link
Author

@jdalma jdalma Aug 24, 2022

Choose a reason for hiding this comment

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

넵 주석 추가하겠습니다 ㅎㅎ
주석을 작성하는 버릇이 없어서 계속 까먹네요..
어떤 상황에 주석을 꼭 작성해야하는지 잘 모르겠네요 ㅎㅎ
영환님은 주석을 "꼭 작성해야지" 하는 기준이 있으신가요??

Choose a reason for hiding this comment

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

public 메소드이면서 도메인 로직이 추가된다면 주석을 넣어보려고 할거 같아요!

service = new ProductService(repository);
}

Product getNewProduct(long number){

Choose a reason for hiding this comment

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

nit : 왠지 그냥 newProduct 라고만해도 될 것같아요! 자바에서는 왠지 get- 있으니까.

이미 있는걸 가져오는 느낌적인 느낌이랄까요???

Comment on lines 40 to 41
Product product = new Product(number , TITLE + number , MAKER + number , (int) number , null);
return product;

Choose a reason for hiding this comment

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

Suggested change
Product product = new Product(number , TITLE + number , MAKER + number , (int) number , null);
return product;
return new Product(number , TITLE + number , MAKER + number , (int) number , null);

한번 사용하고 버려질 변수는 굳이 만들지 않아도 됩니다!

Comment on lines 3 to 5
public class NullResourceException extends RuntimeException{

}

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.

아.. 테스트 코드를 작성하면서 필요할 것 같았는데.. 왜 추가했는지 기억이 나지 않네요 ㅠㅠ
주석이 필요한 이유를 몸소 느꼇네요 ㅎㅎ

@@ -6,6 +6,8 @@
import lombok.Getter;
import lombok.ToString;

import javax.validation.constraints.NotNull;

Choose a reason for hiding this comment

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

591eb17 커밋의 제목과 실제 한 일이 달라요! 또한 @NotNull 은 사용하지 않는 것처럼 보이네요!

Copy link
Author

Choose a reason for hiding this comment

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

커밋 제목 , import 부분 신경쓰도록 하겠습니다 ㅎㅎ

Comment on lines 163 to 168
@DisplayName("저장한다")
void It_SaveResource(){
assertThat(service.save(product)).isEqualTo(product);

verify(repository).save(product);
}

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.

Servicesave메서드를 테스트 하고 Repositorysave메서드가 호출되는지 확인하고 싶었습니다

given(repository.save(product)).willReturn(product);

...

assertThat(service.save(product)).isEqualTo(product);

이 부분은 당연한 테스트 코드인 것 같은데 ㅎㅎ
이렇게 작성하는게 테스트 코드로서 도움이 되는건지는 모르겠습니다..

Choose a reason for hiding this comment

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

제 생각에 Mock이 제대로 동작하는지는 테스트하는 코드 같아보여요!

mock을 없애보는 것은 어떨까요?

@ssisksl77
Copy link

이것은 관용이 아닐 수도 있지만, 저의 경우 커밋 제목에는 골뱅이를 붙이지 않습니다. 클릭하면 이상한 곳으로 가기도 하거든요! 최근에 만드신 b728d01 의 경우 잘못 누르면 @Transaction 에 관련된 페이지로 갑니다!!

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

Choose a reason for hiding this comment

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

조회하는 기능과 아닌 기능을 클래스로 분리하는 것도 좋은 방법 같아요!

@@ -47,6 +48,7 @@ public Product findById(Long id){
* @param product
* @return 저장된 product
*/
@Transactional

Choose a reason for hiding this comment

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

트랜잭션은 H2에서는 잘 작동하지 않을 수 있어요! 그 이유를 한번 공식문서에서 힌트를 얻어보아요! 작은 것도 괜찮아요!

Copy link
Author

@jdalma jdalma Aug 26, 2022

Choose a reason for hiding this comment

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

넵 조사해보겠습니다 ㅎㅎ


찾은 내용은

  1. @Transactional(readOnly = true) 읽기 전용으로 조회한 정보를 수정하여도 예외가 던져지지 않고 저장된다
    • 직접 테스트를 해보았는데 읽기 전용으로 조회하고 수정하여도 저장되었습니다
    • 스프링 문서에 따르면 "반드시 쓰기 액세스 시도가 실패하는 것은 아닙니다. 읽기 전용 힌트를 해석할 수 없는 트랜잭션 관리자는 읽기 전용 트랜잭션을 요청할 때 예외를 throw하지 않고 힌트를 조용히 무시합니다." 라고 하는데 내용을 보면 디비(구현체)마다 처리하는게 다른 것 같습니다

H2 공식문서를 읽어보았는데 트랜잭션의 문제점에 대한 내용을 찾진 못 했네요 ㅠㅠ

Choose a reason for hiding this comment

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

�아 좋습니다. @Transactional 어노테이션은 readOnly 를 인자로 받을 수 있습니다.
하지만 이 readOnly 는 꼭 지켜야 하는 명세가 아닙니다.

https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/transaction/TransactionDefinition.html#isReadOnly

그리고 H2는 이 플래그를 무시하고 있죠!

@@ -12,6 +13,7 @@

@Entity
@Getter
@Setter

Choose a reason for hiding this comment

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

Setter보다는 변경하는 기능만을 위한 메소드가 있는 것이 좋을 것같아요!

Copy link
Author

Choose a reason for hiding this comment

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

Setter를 제거하고 Product Entity 안에서 수정 정보를 적용해 보았습니다 ㅎㅎ

Comment on lines 24 to 26
public List<Product> findAll(){
return (List<Product>) repository.findAll();
}

Choose a reason for hiding this comment

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

findAll 을 그냥 리턴하도록 명세를 만드는 것과 List 로 변환하는 것과 어떻게 차이가 있을까요?!!

Copy link
Author

Choose a reason for hiding this comment

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

List -> Collection -> Iterable을 확장하고 있어서 다형성을 활용하지 못한 불필요한 형변환이였네요

new ArrayList<>(tasks.values());

2주차 때 했던 실수와 똑같은 실수를 했네요..

Comment on lines 39 to 42
Optional<Product> optionalProduct = repository.findById(id);
optionalProduct
.orElseThrow(() -> new ResourceNotFoundException("상품이 존재하지 않습니다. id : " + id));
Product findProduct = optionalProduct.get();
findProduct.setName(product.getName());
findProduct.setMaker(product.getMaker());
findProduct.setPrice(product.getPrice());
findProduct.setFileName(product.getFileName());
return findProduct;
return optionalProduct.get().updateProduct(product);

Choose a reason for hiding this comment

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

optionalProduct이 Optional 이라는 특징을 살려서 Optionalmap 을 이용해보는 것도 좋은 경험일 것 같아요!

Comment on lines 39 to 42
Optional<Product> optionalProduct = repository.findById(id);
optionalProduct
.orElseThrow(() -> new ResourceNotFoundException("상품이 존재하지 않습니다. id : " + id));
return optionalProduct.map(p -> p.updateProduct(product)).get();

Choose a reason for hiding this comment

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

Suggested change
Optional<Product> optionalProduct = repository.findById(id);
optionalProduct
.orElseThrow(() -> new ResourceNotFoundException("상품이 존재하지 않습니다. id : " + id));
return optionalProduct.map(p -> p.updateProduct(product)).get();
return repository
.findById(id);
.map(p -> p.updateProduct(product))
.orElseThrow(() -> new ResourceNotFoundException("상품이 존재하지 않습니다. id : " + id));

제가 생각한 체이닝은 이런 느낌이에요!

…ce.java

Co-authored-by: 남영환 <ssisksl77@gmail.com>
@johngrib johngrib merged commit f6fe141 into CodeSoom:jdalma Oct 3, 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

3 participants