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

헌혈매칭(글) 엔티티 및 도메인 로직, API 구현 #23

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

dongkyunkimdev
Copy link
Member

@dongkyunkimdev dongkyunkimdev commented Jul 11, 2022

📌 Linked Issues

🔎 Change Details

헌혈 요청글 엔티티 및 도메인 로직 작성

끌어올리기 기능을 위해 pull_date, pull_count 컬럼을 추가하고 erd cloud에도 반영

연관관계가 있는 반려동물, 동물병원 엔티티가 현재 존재하지 않아 id값으로 임시 작성

헌혈 요청글 API 구현

  • 헌혈 요청글 리스트 조회(Status, Title로 필터링, 끌어올린 날짜 desc)
  • 헌혈 요청글 상세 조회
  • 헌혈 요청글 등록
  • 헌혈 요청글 수정
  • 헌혈 요청글 삭제
  • 헌혈 요청글 끌어올리기
  • 헌혈 요청글 만료

동적 where절이 필요한 리스트 조회의 경우 spring data jpa의 query method를 사용하게 되면 메서드 이름이 너무 길어져 가독성이 떨어지고, 조건이 많아지는 만큼 많은 메서드를 만들어야 하는 단점이 있기 때문에, querydsl과 BooleanExpression을 사용하여 구현

💬 Comment

  • 현재 head 버전에 문제가 있어 권한에 대한 부분은 제외하고 구현하였음, 추후 해결 시 권한 관련 로직 및 테스트 코드 작성 예정
  • 오버헤드를 최대한 줄이기 위해 nested class로 각 응답에 대한 세부 dto 클래스를 작성하였으며, 클라이언트와 QA 진행하며 필요한 데이터만을 제공할 예정
  • 프로토타입 개발이 목표이기 때문에 어느 정도의 결합도를 허용하여 구현, 추후 DIP, facade 패턴 등을 적용하여 레이어간 결합도를 최소화하도록 리팩토링할 예정
  • ex) Controller -> Facade -> Service <- ServiceImpl
  • ex) ServiceImpl -> Reader <- ReaderImpl -> Repository

📑 References

✅ Check Lists

  • 추가한 기능에 대한 테스트는 모두 완료하셨나요?
  • 코드 정렬(Ctrl + Alt + L), 불필요한 코드나 오타는 없는지 확인하셨나요?
  • merge할 base branch가 올바른지 확인하셨나요?

- 연관관계가 있는 반려동물, 동물병원 엔티티가 현재 존재하지 않아 id값으로 임시 작성
- 프로토타입 개발이 목표이기 때문에 어느 정도의 결합도를 허용하여 구현
- 추후 DIP, facade 패턴 등을 적용하여 레이어간 결합도를 최소화하도록 리팩토링할 예정
- ex) Controller -> Facade -> Service <- ServiceImpl
- ex) ServiceImpl -> Reader <- ReaderImpl -> Repository
@dongkyunkimdev dongkyunkimdev added ✈ Feature 새로운 기능 추가/변경/삭제 ✏ Review request 코드 리뷰 요청 labels Jul 11, 2022
@dongkyunkimdev dongkyunkimdev self-assigned this Jul 11, 2022
@seonpilKim
Copy link
Member

nested class로 관련 있는 dto들을 묶은 거 되게 좋은 것 같습니다!
facade 패턴도 처음 알게 되었는데, 되게 좋은 것 같습니다! 근데 혹시 간단하게 메소드를 하나만 호출하는 것들도 facade에 추가해야 하는 걸까요?

@dongkyunkimdev
Copy link
Member Author

저는 facade 패턴을

  • 적절한 도메인 객체, 로직, 외부 서비스들을 '조합해서 사용'
  • 구현의 변경에 따른 영향을 다른 레이어에 전파하지 않고자 함

크게 위 두 가지의 이유로써 적용하고자 하는데, 한 개의 메서드만 필요하다면 전자의 경우는 필요가 없어지니, 후자의 경우 Interface를 사용하는 것만으로도 얻을 수 있다고 생각해서 사용하지 않아도 좋다고 생각합니다!

그리고 facade 패턴이 layered architecture, ddd의 application layer의 역할과 비슷하다고 생각하여 ddd로 개발하면서 한번 적용해 보고자 하는 생각도 있었습니다!
제가 ddd에 대해 깊게 알지는 못해서, 혹 잘못된 정보나 보완할 내용 있으면 코멘트 부탁드립니다~~~

ex) 간단한 facade 패턴
image

private final LocalDateTime pullDate;
private final int pullCount;

public InfoResponse(BloodPost bloodPost) {
Copy link
Contributor

Choose a reason for hiding this comment

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

ObjectMapper 를 이용하는 방법은 어떻게 생각하시나요!?
똑같은 필드를 모두 가져오는 것 같아서요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니다~~
클라이언트 측에서 가공된 데이터를 필요로 하는 경우가 있을 것 같아 사용하지 않았는데, 우선적으로 ObjectMapper를 사용해도 좋을 것 같습니다!

}

@Builder
public BloodPost(
Copy link
Contributor

Choose a reason for hiding this comment

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

별건 아니지만.. 기본값 설정하는 부분은 Property 에 직접
private Status status = Status.TODO
이런식으로 초기화하는 방식도 자주 사용하는것 같아요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

아하 그렇군요!!
이 부분은 추후에 다른 Status를 가진 생성자 혹은 정적 팩터리 메서드가 생기는 것을 고려하여 작성하였는데, 말씀해주신 방법도 좋을 것 같습니다!!

Copy link
Member Author

Choose a reason for hiding this comment

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

조금 더 부연 설명을 해보자면, 새로운 객체의 생성 방법을 정의하는 경우(ex status가 DONE인 객체) 변수의 초기화가 되어있지 않으면 status가 null로 떨어져 값 세팅을 누락하는 실수를 방지할 수 있는 반면(nullable=false), 변수의 초기화가 되어있는 경우 status 값이 잘못 세팅되어 있는 것을 모르는 채로 넘어가 비즈니스에 문제가 생기는 경우가 발생할 수도 있을 거라는 생각을 했습니다!!

Copy link
Contributor

Choose a reason for hiding this comment

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

그런 케이스가 있을 수 있겠네요! 말씀해주신 부분은 처음 Entity 가 생성될 때 다양한 값들이 들어가면서 생성되는 케이스일 것 같네요!

제가 말씀드린 전제는 Entity 가 생성될 때 기본값이 채워지는 상황이라면 위와같이 초기화 해도 괜찮을 것이라고 생각해서 말씀드린�것입니다 :D

@slolee
Copy link
Contributor

slolee commented Jul 13, 2022

말씀해주신 Facade 패턴은 DDD 의 Domain Service 개념과 매우 흡사하네요!

@dongkyunkimdev
Copy link
Member Author

넵 맞습니다! 말씀해주신 DDD의 도메인 서비스 역할을 생각했습니닷


@Transactional
public BloodPostDto.ExpireResponse expireBloodPost(Long bloodPostId) {
BloodPost bloodPost = getBloodPost(bloodPostId);
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
Member Author

Choose a reason for hiding this comment

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

좋습니다!

Comment on lines +20 to +22
public static class RegisterRequest {
@NotNull
private Long petId;
Copy link
Member

Choose a reason for hiding this comment

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

보통 inner 클래스는 맨 첫 줄과 끝 줄에 빈 줄을 추가하지 않는 걸까요?-?

Copy link
Member Author

Choose a reason for hiding this comment

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

음... 일반 class convention과 동일하게 사용해도 좋을 것 같습니다! 저희가 약속해서 사용하면 될 것 같은데, 어떻게 할까요?

) {
List<BloodPostDto.InfoResponse> response = bloodPostService.selectAllBloodPost(status, title);

return ResponseEntity.ok(ResultResponse.of(ResultCode.BLOODPOST_SELECTALL_SUCCESS, response));
Copy link
Member

Choose a reason for hiding this comment

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

ResultCode, ErrorCode는 static import해서 사용하는 건 어떨까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

좋습니다~~ static import로 통일해서 사용하면 코드가 더 깔끔해지겠네요!

seonpilKim added a commit that referenced this pull request Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✈ Feature 새로운 기능 추가/변경/삭제 ✏ Review request 코드 리뷰 요청
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants