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

[Refactor] Anniversary 리팩터링 #86

Merged
merged 4 commits into from
Nov 26, 2023
Merged

[Refactor] Anniversary 리팩터링 #86

merged 4 commits into from
Nov 26, 2023

Conversation

Juser0
Copy link
Member

@Juser0 Juser0 commented Nov 24, 2023

📋 이슈 번호

#67

🛠 구현 사항

  • Anniversary : setter 메소드 명 변경
  • AnniversaryController : ResponseEntity 정적 팩토리 메소드로 변경 / Service에서 검증 처리하도록 수정, responseStatus 삭제
  • AnniversaryService : 내부 검증 메소드 접근 제한자 변경, for문 요소 stream으로 변경, return 타입 모두 responseDto 형태로 변경
  • RequestDto : AllArgsConstructor 어노테이션 삭제 (미사용)
  • ResponseDto : 생성자 -> 정적 팩토리 메소드로 변경

📚 기타

@Juser0 Juser0 requested a review from eunki96 November 24, 2023 06:23
@Juser0 Juser0 self-assigned this Nov 24, 2023
Copy link
Member

@eunki96 eunki96 left a comment

Choose a reason for hiding this comment

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

정적 팩토리 메서드로 적용 확인했습니다

import lombok.Builder;
import lombok.Getter;

import java.time.LocalDate;
import java.time.LocalDateTime;

@Getter
@AllArgsConstructor
Copy link
Member

Choose a reason for hiding this comment

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

@AllArgsContructor 를 지우고 밑에 static 으로 만들어 준 이유가 아래가 맞아? (정적 팩토리 메서드)

  1. from 정적 팩토리 메서드 사용을 통해서 DTO 변환 과정 중앙화
  2. AnniversaryResponseDto 가 Anniversary 에 덜 의존적이게 됨

Copy link
Member Author

Choose a reason for hiding this comment

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

@AllArgsConstructor가 있음에도 사용하지 않았기 때문에 어노테이션을 삭제한거고,
정적 팩토리 메소드를 사용한 이유는 인스턴스화에 있어서 형 말대로 덜 의존적이게 되도록 하는 의도지
(stream 등에서 활용 가능)

@@ -21,14 +19,27 @@ public class AnniversaryResponseDto {
private AnniversaryCategory anniversaryCategory;

@Builder
Copy link
Member

Choose a reason for hiding this comment

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

리팩토링 이유 : 파라미터 1개인데 빌더 사용
이 맞아?
생성자 사용 안하는 것을 고려할 때 사용하는 경우 보여주려고 남겨 둔 것 같아서

Copy link
Member Author

Choose a reason for hiding this comment

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

파라미터가 1개인데 빌더 어노테이션을 사용해서 리팩토링을 진행한게 아니라,
생성자에 있어서는 매개변수가 다른 클래스에 의존적인 형태가 적절하지 못하다고 생각했어

그래서 private 접근 제한자로 모든 매개변수에 대한 생성자를 정의하고,
정적 팩토리 메소드로는 매개변수로 클래스를 받아서 각 매개변수에 해당하도록 작성한거지

@GetMapping("/admin")
public ResponseEntity<DefaultResponseDto<Object>> readAll(){

List<AnniversaryResponseDto> dto = anniversaryService.readAll();

return ResponseEntity.status(200)
.body(resWithData("ANIVERSARIES_FOUND", "전체 기념일 조회 완료", dto));
return ResponseEntity.ok(DefaultResponseDto.from("ANNIVERSARIES_FOUND", "전체 기념일 조회 완료", dto));
Copy link
Member

Choose a reason for hiding this comment

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

기능은 같은 것으로 알고 있는데
가독성 위한 리팩토링 맞을까?
다른 이점이 있다면 알려줘! AnniversaryController 외 다른 컨트롤러는 아직 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.

created도 메소드가 존재하는데
찾아보면 created(URI location)이라고 나와있어

이 부분은 created라는 메소드가 매개변수로 body를 요구하지 않는 걸 확인할 수 있어
이때 location은 header에 location=""에 해당해서 응답 DTO를 담아서 반환해주는 우리 코드에서는 적용하려면
ResponseEntity.created(null).body(~); 로 작성해야해

그래서 200 OK와 같은 경우에는 ok(T body)로 더 줄일 수 있어서 줄였고
201 CREATED는 그대로 놔둔거지

@eunki96 eunki96 merged commit e36c107 into main Nov 26, 2023
1 check passed
@eunki96 eunki96 deleted the juwon/#67 branch November 26, 2023 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants