Skip to content
This repository has been archived by the owner on Sep 16, 2024. It is now read-only.

[Feat] Add record field #38 #40

Merged
merged 4 commits into from
Jan 25, 2024
Merged

Conversation

zbqmgldjfh
Copy link
Member

개요

이번기능 생각보다 생각할점이 엄청 많았습니다...

Todo

  • GET - LookUp Plants 에 '식물 종' 추가
  • GET - LookUp Record에 상의 했던 내용 제공
    • 물 주기 Boolean
    • 식물의 별명(nickname)

논의가 필요한 사항

해당 부분에 comment 달아두겠습니다. 확인후 답변 달아주시쥬~

등록 전 확인

  • 모든 test가 pass했나요?
  • 모든 code가 well-formatted인가요?
  • commit message는 직관적이었을까요?
  • 삭제해야 할, 주석처리 된 코드는 없을까요?

@zbqmgldjfh zbqmgldjfh added the 🚀 기능 구현 🚀 기능 구현 label Jan 24, 2024
@zbqmgldjfh zbqmgldjfh self-assigned this Jan 24, 2024
AND DATE(history.createAt) = :recordDate
"""
)
fun findAllHistoryTypeByDate(companionPlantId: Long, recordDate: LocalDate): List<HistoryType>
Copy link
Member Author

Choose a reason for hiding this comment

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

해당 일자에 물을 줬는지만 확인하고 싶다면 쿼리메서드 or Exists절을 활용한 Boolean을 반환하도록 하면 되지 않았을까요?
제가 물을 줬는지 확인하기 위해 이렇게 돌아가는 이유는 뭘까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

마찬가지로 DAO의 역할은 data access입니다. 읽어온 data에 따른 판단 및 분기는 doamin/service의 역할입니다.

Copy link
Member Author

@zbqmgldjfh zbqmgldjfh Jan 25, 2024

Choose a reason for hiding this comment

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

이것도 그냥 물준거만 boolean값으로 조회하면 더 좋았던 상황입니다.
이건 조회쿼리라 Domain을 통으로 들고와 비즈니스로직을 가하기 보다는, 한번에 필요한 값만 갖고오는편이 더 좋습니다.

그럼에도 그러지 않은 이유는

  1. JPQL이 select 절에 exists를 지원하지 않습니다. (QueryDsl은 놀랍게도 가능합니다!)
  2. 해당 리포지토리는 쿼리메서드 사용시 CompanionPlant 위주로만 찾아올수 있습니다. (History는 다른 테이블이라..)

AND DATE(r.createAt) = :recordDate
"""
)
fun findRecordByDate(companionPlantId: Long, memberId: Long, recordDate: LocalDate): PlantRecordDto
Copy link
Member Author

Choose a reason for hiding this comment

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

왜 굳이 물준 기록은 별도로 분리한것 일까요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

DAO(Repository)의 역할은 data access에 있습니다. 데일리 기록과 물 준 기록이 함께 필요한건 비즈니스의 영역이지 데이터 조회 단계에서 필요한 사항이 아니라고 생각합니다.

Copy link
Member Author

@zbqmgldjfh zbqmgldjfh Jan 25, 2024

Choose a reason for hiding this comment

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

�그럼 그냥 데일리 기록과 물준 기록만 한번에 조회해 오면 되었던것 아닐까요?

이렇게 한 이유는, 한번에 (식물닉네임-레코드기록-물준기록) 을 확인하려면 JOIN이 2번 수행되어야 하기 때문입니다.
식물 기준으로 JOIN이 들어갈 경우, 1-N-M으로 퍼지기 때문에 데이터 뻥튀기(실제로 이렇게 다들 부릅니다 ㅋㅋ) 데이터 중복 문제가 발생할 뿐만 아니라, DB상에 너무 큰 단일 임시테이블이 생성되게 될 수 있습니다. (사용자의 기록만 충분하게 많다면)

기록은 1년동안 기록할 경우 꾸준하게 하는 사용자라면 200건도 충분하게 넘어갈탠데, 1-N-M까지 JOIN하면 테이블이 너무 커지죠, (총 행의 수가 N*M인 테이블)

이럴바에 1-N 테이블만 JOIN하여 N짜리를 한번 가져온후, M은 따로 조회하면 조회한 테이블의 수가 (N+M)으로 확 줄어들게 됩니다.

따라서 2개의 쿼리를 분리하여 작성하였습니다.

@@ -139,7 +140,7 @@ class CompanionPlantStep {
fun 데일리_기록_조회_응답_확인(response: ExtractableResponse<Response>) {
assertAll(
{ assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value()) },
{ assertThat(response.jsonPath().getString("id")).isNotBlank() },
{ assertThat(response.jsonPath().getString("plantRecordId")).isNotBlank() },
Copy link
Member Author

Choose a reason for hiding this comment

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

id라 그냥 넘겨주면 받는 입장에서 어떤 ID인지 모를거 같아 이름을 명시적으로 바꿨습니다.

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

@goldentrash goldentrash left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!

AND DATE(r.createAt) = :recordDate
"""
)
fun findRecordByDate(companionPlantId: Long, memberId: Long, recordDate: LocalDate): PlantRecordDto
Copy link
Collaborator

Choose a reason for hiding this comment

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

DAO(Repository)의 역할은 data access에 있습니다. 데일리 기록과 물 준 기록이 함께 필요한건 비즈니스의 영역이지 데이터 조회 단계에서 필요한 사항이 아니라고 생각합니다.

AND DATE(history.createAt) = :recordDate
"""
)
fun findAllHistoryTypeByDate(companionPlantId: Long, recordDate: LocalDate): List<HistoryType>
Copy link
Collaborator

Choose a reason for hiding this comment

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

마찬가지로 DAO의 역할은 data access입니다. 읽어온 data에 따른 판단 및 분기는 doamin/service의 역할입니다.

@@ -139,7 +140,7 @@ class CompanionPlantStep {
fun 데일리_기록_조회_응답_확인(response: ExtractableResponse<Response>) {
assertAll(
{ assertThat(response.statusCode()).isEqualTo(HttpStatus.OK.value()) },
{ assertThat(response.jsonPath().getString("id")).isNotBlank() },
{ assertThat(response.jsonPath().getString("plantRecordId")).isNotBlank() },
Copy link
Collaborator

Choose a reason for hiding this comment

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

좋습니다!

@zbqmgldjfh zbqmgldjfh merged commit 6e6a3be into develop Jan 25, 2024
1 check passed
@zbqmgldjfh zbqmgldjfh deleted the feat/add-record-field-#38 branch January 25, 2024 02:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants