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

Feature/3 #6

Merged
merged 9 commits into from
Oct 1, 2024
Merged

Feature/3 #6

merged 9 commits into from
Oct 1, 2024

Conversation

soleu
Copy link
Collaborator

@soleu soleu commented Sep 28, 2024

#️⃣연관된 이슈

#3

📝작업 내용

상세 조회 api 추가
기존 상품 리스트 api 불필요한 필드 제거
sql 생성 방식 spring boot 동작시로 수정

@soleu soleu added the feature label Sep 28, 2024
@soleu soleu requested a review from mason136 September 28, 2024 17:48
@soleu soleu self-assigned this Sep 28, 2024
@Test
fun activeProductListControllerTest() {
fun productListIntegrationTest() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

테스트 검증 코드 자체가 sql 파일에서 생성하는 데이터 의존적이라 잘못된 테스트 코드입니다.
현재 클래스 내에서 데이터 생성하고 검증해주세요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵! 생각치 못했던 부분 피드백 감사합니다 :)

@@ -14,7 +16,12 @@ class ProductController(
private val productService: ProductService
) {
@GetMapping
fun getActiveProductList(@RequestParam userId: String): ApiResponse<List<ProductListDto>> {
return ApiResponse.success(productService.getActiveProductList(userId.toLong()))
fun getProductList(@RequestParam userId: String): ApiResponse<List<ProductListDto>> {
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
Collaborator Author

Choose a reason for hiding this comment

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

페이징 추가했습니당~

@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private val id: Long? = null
val id: Long? = null
Copy link
Collaborator

Choose a reason for hiding this comment

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

pk 아이디도 생성자에 넣어도 됩니다

val id: Long?,
val writerId: Long,
val writerNickname: String,
val writerProfileImg: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

writer 정보를 따로 객체로 만들어도 되지 않을까요?

val writer = userRepository.findById(id)
.orElseThrow { NotFoundException("$id 의 사용자는 존재하지 않습니다.") }

return ProductDetailDto(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Service에서 만드는 것보다 ProductDetailDto에서 static 함수 만들어서 객체 만드는 것이 편리합니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

내부에서 값을 넣는 방식 저도 선호하는데, '비즈니스 로직이 서비스에서 보이지 않으면 비즈니스 로직이 흐려지는 것 같다' 라는 피드백을 회사에서 받은 적이 있어서요.
혹시 멘토님은 어떻게 생각하지는지 궁금합니다!

Copy link
Collaborator

@mason136 mason136 Oct 1, 2024

Choose a reason for hiding this comment

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

저는 단순히 객체 생성하는 로직은 서비스에선 제외해도 좋다고 생각합니다.
서비스 로직 자체도 길 수 있는데 값을 세팅하는 줄만 여러줄 되는 것은 가독성이 떨어지긴 해요

@mason136
Copy link
Collaborator

mason136 commented Oct 1, 2024

.idea 폴더 .gitignore에 추가 및 .idea 폴더 내에서 이미 버전 관리 되고 있는 파일들은 제거해주세요

Copy link

sonarcloud bot commented Oct 1, 2024

@soleu soleu merged commit ada01ba into main Oct 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants