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

[FEAT] 데일리 루틴 테마 리스트 조회 #17

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

thguss
Copy link
Member

@thguss thguss commented Jan 8, 2024

✨ Related Issue

📝 기능 구현 명세

image

🐥 추가적인 언급 사항

  • 예시 API가 필요하다고 하여, 우선 테스트 진행 전 PR 업로드 했습니다.
  • DB 세팅 후 추가적인 테스트, 코드 리팩토링, 의논 반영 등 작업 진행하겠습니다.
  • 유연한 쿼리 환경을 위해 QueryDSL을 추가했습니다.
  • 데일리 루틴의 테마에 따라 이미지 정보가 결정되어 테마 엔티티로 이미지 정보를 이동했습니다.
  • 아이콘이 루틴 달성 여부에 관계없이 유색 이미지로 통일되는 것으로 전달받아 무색 이미지는 삭제했습니다.
  • 공통 ResponseDTO 클래스를 추가했습니다. 패키지 이름을 통해 클래스 역할이 정의된다고 생각하여 클래스명은 Response로 우선 정의했습니다.
  • 자유롭게 의견이나 궁금한 점 있으시면 리뷰로 부탁드려요 :)

@thguss thguss added sohyeon 소현 작업 feat 기능 개발 labels Jan 8, 2024
@thguss thguss requested review from csb9427 and Chan531 January 8, 2024 21:18
@thguss thguss self-assigned this Jan 8, 2024
Copy link
Contributor

@csb9427 csb9427 left a comment

Choose a reason for hiding this comment

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

코드 너무 잘 봤습니다 짧은 시간이었는데.. 바로 이렇게 개발하시다니 역시 천재 소현..!

Comment on lines +25 to +26
@Embedded
private RoutineImage imageInfo;
Copy link
Contributor

@csb9427 csb9427 Jan 8, 2024

Choose a reason for hiding this comment

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

이렇게 class로 만드는 거 가독성이 높아져서 좋은 거 같아요!!

Comment on lines +21 to +26
public List<DailyTheme> findAllOrderByNameAsc() {
return queryFactory
.selectFrom(dailyTheme)
.orderBy(dailyTheme.name.asc())
.fetch();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

findAll 메서드를 많이 사용하고 원하는 칼럼의 순서대로 정렬하고 싶을 때

이때, 함수 이름은 findAllByOrderByColumnDesc 와 같은 식으로 사용한다. Column에는 우리가 원하는 칼럼의 이름을 써주는 것이다. 만약 By를 사용하지 않으면 에러가 발생하게 됩니다.

오호 방금 검색하며 봤는데 JPA 참 신기하군요

Copy link
Member Author

Choose a reason for hiding this comment

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

@csb9427 맞아요~ 메소드명으로 사용해도 가능하지만 가독성을 위해 QueryDSL을 활용해보았어요!

private final DailyThemeRepository dailyThemeRepository;

public DailyThemesResponse getThemes() {
val themes = dailyThemeRepository.findAllOrderByNameAsc();
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 let이 아닌 val를 쓰신 이유가 있나요?? 그냥 궁금해서요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

@csb9427 엇 자바에도 let이 있나요??(javascript만 존재하는 것으로 압니다!) 우선 val은 lombok에서 지원하는 기능으로, 불변성을 가진 변수를 선언할 때 사용합니다!

image

출처: https://partnerjun.tistory.com/53

위 이미지를 포함해서 한 번 직접 검색해보셔도 좋을 것 같아요 :)
또한 lombok의 val로 변수 타입 선언을 통일할지 말지도 함께 정해보면 좋을 것 같아요~

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
Contributor

Choose a reason for hiding this comment

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

아 ㅋㅋ 나 정신없이 리뷰 달다가 자스랑 착각했나봐요 친절한 설명 감사합니다.. ㅜㅜ

@@ -0,0 +1,20 @@
package com.soptie.server.common.dto;

public record Response(
Copy link
Contributor

Choose a reason for hiding this comment

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

후후 만들어주셔서 감사합니다~

Copy link
Contributor

@Chan531 Chan531 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~!!!


@GetMapping("/themes")
public ResponseEntity<Response> getThemes() {
val response = dailyRoutineService.getThemes();
Copy link
Contributor

Choose a reason for hiding this comment

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

val을 쓰신 이유가 궁금합니다!

Copy link
Member Author

@thguss thguss Jan 9, 2024

Choose a reason for hiding this comment

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

@Chan531
#17 (comment)
승빈님 리뷰에도 답 달아뒀어요! 함께 확인해주시면 좋을 것 같아요 :)

import lombok.*;

@Service
@RequiredArgsConstructor
Copy link
Contributor

Choose a reason for hiding this comment

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

service 클래스 작성할 때 @transactional(readonly = true)를 미리 적어놓는 게 낫나요 아니면 필요할 때 적는게 낫나요?? 궁금해서 여쭤봅니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

@Chan531 아 놓치고 있었는데 언급 감사해요!! 미리 붙여놓는 게 좋아요. 불필요한 트랜잭션을 부르지 않아도 되니까요 :)

@thguss thguss merged commit 177a6ef into develop Jan 9, 2024
1 check passed
@thguss thguss deleted the feature/#15-theme-get-api branch January 9, 2024 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat 기능 개발 sohyeon 소현 작업
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 데일리루틴 테마 리스트 조회
3 participants