Skip to content

🚀 1단계 - 상품 관리 기능 #41

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

Merged
merged 11 commits into from
May 17, 2023

Conversation

haileyKimDev
Copy link

@haileyKimDev haileyKimDev commented May 15, 2023

안녕하세요. 미션제출이 늦었습니다.
repository, controller 테스트코드 적용해보았습니다!

  • 상품 목록 페이지 연동 기능
  • 상품 관리 CRUD API 작성
  • 관리자 도구 페이지 연동

Copy link
Member

@Hyeon9mak Hyeon9mak left a comment

Choose a reason for hiding this comment

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

안녕하세요 영화님, 상품 관리 기능 구현 고생하셨습니다.
함께 이야기 나눠보면 좋을 부분 코멘트 남겼습니다.
궁금하신 점 DM, 코멘트 남겨주세요 🙇‍♂️

Comment on lines +17 to +23
implementation 'org.projectlombok:lombok'

testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'io.rest-assured:rest-assured:4.4.0'
testImplementation 'org.projectlombok:lombok'
annotationProcessor('org.projectlombok:lombok')
testAnnotationProcessor('org.projectlombok:lombok')
Copy link
Member

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.

dto와 product 클래스에서 getter, builder 및 생성자어노테이션을 사용하여서 넣었습니다 !

Copy link
Member

Choose a reason for hiding this comment

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

없어도 생성하는데 문제가 없지 않나요? 조금 더 구체적인 사용 이유가 있다면 좋겠어요!!

Comment on lines +10 to +11
@Controller
public class AdminController {
Copy link
Member

Choose a reason for hiding this comment

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

자원과 뷰에 따른 명확한 컨트롤러 분리 👍

Comment on lines 19 to 24
@GetMapping("/admin")
public String getAll(Model model) {
List<Product> products = productService.getAll();
model.addAttribute("products", products);
return "admin";
}
Copy link
Member

Choose a reason for hiding this comment

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

ModelAndView 를 사용해볼 수도 있겠네요.

Copy link
Author

Choose a reason for hiding this comment

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

아 넵! ModelAndView로 적용해보겠습니다!

Comment on lines 32 to 35
@PutMapping(value = "/products")
public void updateProduct(@RequestBody ProductUpdateDto updateDto) {
productService.updateProduct(updateDto);
}
Copy link
Member

Choose a reason for hiding this comment

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

변경할 대상에 대해 pathVariable 로 명시하는 편인거 같아요.
https://gmlwjd9405.github.io/2018/09/21/rest-and-restful.html

Copy link
Author

Choose a reason for hiding this comment

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

@Hyeon9mak 추가 질문드립니다!
해당 기능 구현시 pathVariable도 고민했었는데,
updateProduct메서드에서 pathVariable를 파라미터로 받아서 메소드 내에서 파라미터를 따로 쓰고있지않아서
ProductUpdateDto만 넘겨받도록처리했습니다.

그래도 url에서 변경할 대상을 명시하는것에 좋을가요~?

Copy link
Author

@haileyKimDev haileyKimDev May 16, 2023

Choose a reason for hiding this comment

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

  @PutMapping(value = "/products/{id}")
  public void updateProduct(@PathVariable Long id,
                            @RequestBody ProductUpdateDto updateDto) {
    productService.findById(id);
    productService.updateProduct(updateDto);
  }
  • pathVariable로 url에 명시하도록 변경하였습니다.
  • 해당 상품이있는지 확인하는 로직을 추가하여 파라미터로 받은 id를 사용하도록했습니다.
  • productService.findById(id); 부분은 productService.updateProduct(updateDto);의 내부로 넣는게나을까요? 지금처럼 컨트롤러에서 확인해도 괜찮을지 궁금합니다!

Comment on lines 10 to 20
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED)
@AllArgsConstructor
@Builder
public class Product {

Long id;
String name;
String image;
BigDecimal price;

Copy link
Member

Choose a reason for hiding this comment

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

  • no args constructor 가 필요할까요?
  • id 를 통해 나머지 데이터의 변화를 비교하는 객체이므로, id 에 대해서 동등성을 부여해서 관리해볼 수 있을까요?
  • 필드 멤버의 접근제한을 관리해줄 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

  • 아 인자없는 생성자를 호출하고있지않아서 필요없을 것 같습니다! 해당 부분 삭제하겠습니다.
  • Product클래스 내부에 equals를 오버라이드하겠습니다. 아마 이부분은 다음 2단계 과제때 필요한것같네요!!
  • 아 넵!! 접근제한 적용하겠습니다!

Comment on lines 12 to 15
@Getter
@NoArgsConstructor(access = AccessLevel.PROTECTED, force = true)
@AllArgsConstructor
@Builder
Copy link
Member

Choose a reason for hiding this comment

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

jackson 2.13.0 버전부터 더 이상 request dto 에 no args constructor 가 필요하지 않습니다.
https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.13

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다! 삭제하겠습니다!

@Repository
public class ProductRepository {

private static Long sequence = 1L;
Copy link
Member

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.

아 네! 디비에서 자동증가로바꾸었는데, 해당 부분을 지우지않았네요. 삭제하겠습니다!

Comment on lines 9 to 11

@Service
public class ProductService {
Copy link
Member

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.

  • 서비스클래스 자체에 트랜잭셔널 어노테이션추가하였습니다.
  • 읽기를 수행하는 findById(), getAll()메소드에는 @transactional(readOnly = true) 을추가하였습니다.

Comment on lines 52 to 57

@Test
void getAll() {
List<Product> products = repository.getAll();
assertThat(products).hasSize(3);
}
Copy link
Member

Choose a reason for hiding this comment

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

ProductControllerTest 와 같이 @DisplayName 어노테이션을 명시해주면 더욱 관리가 쉬워지겠어요!

Copy link
Author

Choose a reason for hiding this comment

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

아네! 추가하겠습니다!

List<Product> products = repository.getAll();
assertThat(products).hasSize(4);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

intellij 를 사용하시는 경우 Editor > General > Ensure every saved file ends with a line break 를 체크하시면 파일 마지막에 자동으로 개행이 적용됩니다.

image

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
Member

@Hyeon9mak Hyeon9mak left a comment

Choose a reason for hiding this comment

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

안녕하세요 영화님, 피드백 잘 반영해주셨습니다 👍
꼼꼼한 트랜잭션 명세를 잘 해주신 덕분에 ACID 보장이 확실하겠네요~ 😄
다음 단계를 진행하면서 함께 고민해보시면 좋을 부분 코멘트 남겼습니다.
다음 단계 진행도 화이팅입니다 💪

Comment on lines +17 to +23
implementation 'org.projectlombok:lombok'

testImplementation 'org.springframework.boot:spring-boot-starter-test'
testImplementation 'io.rest-assured:rest-assured:4.4.0'
testImplementation 'org.projectlombok:lombok'
annotationProcessor('org.projectlombok:lombok')
testAnnotationProcessor('org.projectlombok:lombok')
Copy link
Member

Choose a reason for hiding this comment

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

없어도 생성하는데 문제가 없지 않나요? 조금 더 구체적인 사용 이유가 있다면 좋겠어요!!

Comment on lines +19 to +25
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof Product)) return false;
Product product = (Product) o;
return Objects.equals(id, product.id);
}
Copy link
Member

Choose a reason for hiding this comment

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

이왕 롬복을 사용하시니 equals, hashcode 관련 어노테이션을 활용해볼 수도 있겠네요!

Copy link
Author

Choose a reason for hiding this comment

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

넵 감사합니다! 머지되었지만 , 개인 레포에 어노테이션 이용해서 바꾸었습니다 !

Comment on lines +15 to +20
@NonNull
private String name;
@NonNull
private String image;
@NonNull
private BigDecimal price;
Copy link
Member

Choose a reason for hiding this comment

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

기본 생성자가 사라졌으므로 final 키워드를 사용해서 불변성을 제공할 수 있겠네요.

Copy link
Author

Choose a reason for hiding this comment

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

아 넵!! 필드들에 final 키워드 추가하여 불변성유지하도록 하겠습니다!

Comment on lines +32 to +37
@PutMapping(value = "/products/{id}")
public void updateProduct(@PathVariable Long id,
@RequestBody ProductUpdateDto updateDto) {
productService.findById(id);
productService.updateProduct(updateDto);
}
Copy link
Member

Choose a reason for hiding this comment

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

updateProduct메서드에서 pathVariable를 파라미터로 받아서 메소드 내에서 파라미터를 따로 쓰고있지않아서
ProductUpdateDto만 넘겨받도록처리했습니다.
그래도 url에서 변경할 대상을 명시하는것에 좋을가요~?

API 명세는 클라이언트와 서버간 소통을 위해 사용되므로
널리 이용되는 규약을 지킬수록 서로 의사소통 비용을 줄이는데 도움이 된다고 생각해요.
때문에 request 내부에서 id 를 갖고 있더라도, 명시하는 것에 의미가 있다고 생각합니다.

id 를 통해 검증을 해주시는 것도 좋지만, 현재 API 요청의 주된 목적은 상품정보의 변경이므로
updateProduct 내부에서 별개로 검증이 진행되는 것이 더 좋다는 생각이 드네요!
(가령 path variable id 로 엔티티를 조회한 후 update dto 의 값들로 변경한다던지..)

Copy link
Author

Choose a reason for hiding this comment

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

@Hyeon9mak 리뷰어님 답변 감사합니다!

  • API 명세를 구체적으로 명시해서 의사소통 비용을 줄일 수 있도록 하겠습니다!
  • 해당 product의 검증을 서비스 계층에서 하도록 변경해보겠습니다!

@Hyeon9mak Hyeon9mak merged commit c89773f into next-step:haileykim2014 May 17, 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.

2 participants