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

#1 [feat] 1주차 과제 제출 #2

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

#1 [feat] 1주차 과제 제출 #2

wants to merge 12 commits into from

Conversation

mikekks
Copy link
Contributor

@mikekks mikekks commented Oct 11, 2023

✒️ 관련 이슈번호 ✒️

💡 과제 내용 및 배운 점 💡

  1. 기본과제 : 세미나 내용 복습
  2. 심화과제 : HealthCheckResponse 객체 사용

📢 To Reviewers 📢

@Getter
@NoArgsConstructor
public class HealthCheckResponseDtoV2 {
    private String code;
    private String status;
    private boolean success;

    @Builder
    public HealthCheckResponseDtoV2(String code, String status, boolean success) {
        this.code = code;
        this.status = status;
        this.success = success;
    }
}

해당 내용은 심화과제 관련 내용입니다.
소현님 코드를 음미해봤는데 좋아서 V1에 좀 따라해봤습니다..
너무 따라한거 같아서 위의 코드는 V2 버전으로 세미나 시간에 배운 빌더 패턴을 적용한 객체로 새로 만들어 봤습니다!

이런식으로 코드리뷰하는 것이 처음이어서 이게 맞나 싶네요..
처음이라 어설프게 따라한 부분이 많은 거 같은데 좋은 의견 있으시면 가감없이 말씀해주시면 감사하겠습니다 !

Copy link

@sohyundoh sohyundoh left a comment

Choose a reason for hiding this comment

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

진짜 너무 멋있다! 고생하셨어요 👍

private String status;
private boolean success;

@Builder

Choose a reason for hiding this comment

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

오 builder 패턴을 사용하셨네요! 짱짱맨!
아래 코드와 똑같이 작동한다는 점도 염두해두시면 좋을 것 같아요!! 제가 생각할 때는 민규님이 써준 코드도 생성자의 의미와 작동을 볼 수 있어서 좋지만, 어노테이션을 통해 더 깔끔한 코드를 만들 수 있을 것 같아요!

@Getter
@Builder
@AllArgsConstructor
public class HealthCheckResponseDtoV2 {
    private String code;
    private String status;
    private boolean success;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오우 꼼꼼하게 봐주셔서 감사합니다 ㅎㅎ Builder 원리를 모르고 했다가 덕분에 어떻게 컴파일 되는지 알 수 있었네요!
오늘도 많이 배우고 갑니다..!

import lombok.NoArgsConstructor;

@Getter
@NoArgsConstructor

Choose a reason for hiding this comment

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

생성자가 이미 있으니 기본생성자(@NoArgsConstructor)는 없어도 될 것 같아요!
객체 생성에 기본 생성자를 쓰는 일이 없으니 생각해보시면 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제가 Builder를 잘 몰라서 기본 생성자를 넣어야 하는 줄 알았네요..! 기본 생성자를 잘 안쓰는거 명심하겠습니다!

Copy link
Member

@jun02160 jun02160 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 +54 to +58
return HealthCheckResponseDtoV2.builder()
.code("200")
.status("OK")
.success(true)
.build();
Copy link
Member

Choose a reason for hiding this comment

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

빌더 패턴 사용하신 거 직관적이고 아주 좋은 것 같습니다!!


@Getter
@Builder
@AllArgsConstructor
Copy link
Member

Choose a reason for hiding this comment

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

저도 지난 기수 때 처음 알게 되었는데, AccessLevel 을 명시하지 않으면 디폴트로 public을 생성자의 접근지정자로 가져가게 됩니다. 이 제어 수준을 @AllArgsConstructor(access = AccessLevel.PRIVATE) 와 같이 private/protected까지만 제한하도록 명시해주는 것이 DTO 객체의 캡슐화 및 불변성 측면에서 더 좋다고 합니다!!!
'롬복 생성자 어노테이션과 접근 제어'를 주제로 참고해보시면 좋을 것 같아요 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

근데 다시 보니까 v1에서는 이미 쓰셨군요!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

오 정말 감사합니다. 혼자 공부할 때는 이런 고민을 많이 안해봐서 빼먹었네요.. 접근제어자도 신경써보겠습니다!
감사합니다!!!

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.

3 participants