-
Notifications
You must be signed in to change notification settings - Fork 1
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
[feat] 광고 상품 post,get API 구현 #17
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
진짜 고생 많았어요!! 하루만에 이렇게 많이 작업하다니... 역시 갓소민👍
코드리뷰 할 실력은 안되지만, 작업하면서 모르는 내용 나오면 바로 물어보겠습니다.
} | ||
|
||
@Override | ||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
21행에 Transactional(readonly=true)
속성이 클래스 공통으로 적용되어 있어서, 없어도 될 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
없어도 되는게 아니라 없어야 할 것 같아요. 21행보다 우선순위로 트랜잭션이 적용돼서, 리드온리가 아닌 Read/write 트랜잭션으로 적용될 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 마다 맞게 적용시키는걸로 수정했습니다~
} | ||
|
||
@Override | ||
@Transactional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
얘도 없어도 될것같아요
|
||
import java.util.List; | ||
|
||
public record CreateProductRequest ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DTO를 record로 선언하는게 좋은가 봐요.
https://velog.io/@oyoungsun/Java-RECORD-DTO%EB%A5%BC-record-type%EC%9C%BC%EB%A1%9C-%EC%84%A4%EC%A0%95%ED%95%98%EB%8A%94-%EC%9D%B4%EC%9C%A0
좋은 정보 알아갑니다
import jakarta.validation.constraints.NotNull; | ||
|
||
import java.util.List; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dto에는 @builder
를 안쓰나요? 저는 웬만하면 빌더를 다 사용하는데, 좋은 방법이 아닌가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@builder를 혹시 어디서 쓰시나용..? 저는 builder가 필요하지 않아서 안쓴거라..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dto 생성할때, new CreateProductRequest()
로 안하고 빌더패턴 쓰려고요!
record든 class든 기본적으로 생성자 대신 빌더를 사용하는게 맘 편하더라고요.
https://velog.io/@letsdev/Java-16-Record-Class-2-Usage-DTO
참고하면 좋을 것 같아요 !!
|
||
@Repository | ||
public interface ProductRepository extends JpaRepository<Product, Long> { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findById
단건조회 메서드가 없어도 되나요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findById
단건조회 메서드가 없어도 되나요?!
findById()의 경우 이미 JpaRepository에 있어서 만들 필요가 없어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨슴돠~!
|
||
@Slf4j | ||
@RestController | ||
@RequestMapping("/product") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
api이니깐 url을 "/api/product"으로 하는거 어떠신가요? 진우님이랑도 상의해봐야할 것 같아요!
api와 api가 아닌 요청을 구분할 필요가 있을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 "/api/v1/product" 이런식으로 쓰면 좋겠다고 생각해서 상의해보면 좋을것같아요~
|
||
@Repository | ||
public interface ProductRepository extends JpaRepository<Product, Long> { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
findById
단건조회 메서드가 없어도 되나요?!
findById()의 경우 이미 JpaRepository에 있어서 만들 필요가 없어요!
|
||
import java.util.List; | ||
|
||
public interface ProductService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProductService를 인터페이스로 만드신 이유가 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예전에 처음배울때 ocp 때문에 만들어서 사용하는게 좋다고 배워서 항상 그렇게 하고 있어요..! 사실 이 프로젝트에서는 serviceimpl과 service가 일대일으로 될 것 같아 꼭 필요하지 않을 것 같기도 합니다
https://chiki-cha.tistory.com/107
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DI할때 구현체가아닌 추상화 단계에서 의존관계를 설정하는게 이상적이라고 배웠어요. 그래서 인터페이스를 만들고 Impl을 만드는게 좀 더 정확한거 같아요.
근데 프로젝트에서 보통 귀찮아서 구분하지 않는것일 뿐, 저는 이 구조 좋아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇군요! 감사합니다
🗃 Issue
🔥 Task
📄 Reference