[코드 리뷰] - Day 1.

김영훈·2024년 2월 29일

[인프런]_미니프로젝트

목록 보기

2/6

피드백 1.

  public List<GetAllMembersResponse> getAllMembers() {
        return memberRepository.findAll().stream().map(Member::toResponse).toList();
    }

📌 Member 랑 Team 둘다 Entity 내부에서 Response 를 생성하여 반환해주고 있는데, 이렇게 작성하신 이유가 무엇일까요? 저도 DTO를 어디서 변환시켜 줘야하는지 항상 고민 많은데 Entity Response 생성 후 반환을 시켜준 이유가 궁금합니다.

해결 과정 1.

처음엔 단순히 변환과정을 일관되게 관리하면 나중에 유지보수하기 쉽지않을까?
해서 도메인 내부에 변환 로직을 만들어뒀는데 , 다른분의 PR 리뷰를 보고 DTO 내부에서
변환하는 방법도 있다는것을 알게 되었다.

@Builder 
public record GetAllMembersResponse(String name, String teamName, String role, LocalDate birthday,
                                    LocalDate workStartDate) {
   
    public static GetAllMembersResponse from(Member member){
        String isManager = member.isRole() ? "MANAGER" : "MEMBER";

        return GetAllMembersResponse.builder()
                .name(member.getName())
                .teamName(member.getTeamName())
                .role(isManager)
                .birthday(member.getBirthday())
                .workStartDate(member.getWorkStartDate().toLocalDate())
                .build();
    }
}

record는 모든 필드값을 파라미터로 받는 생성자를 자동 생성하기 때문에, @Builder를 클래스에 붙여준다.

@Transactional(readOnly = true)
    public List<GetAllMembersResponse> getAllMembers() {
        return memberRepository.findAll().stream().map(GetAllMembersResponse::from).toList();
    }

그 후, ResponseDTO의 from을 통해 Entity를 변환시켰다.

💡 ResponseDTO를 record로 생성하고 @Builder를 적용하여 엔티티를 변환하는 방법은 정말 좋은 방법인것 같다.
용기를 내서 PR리뷰에 참여하길 잘한 것 같다.

피드백 2.

 public Team findTeamByName(SaveMemberRequest request) {
        Team team = teamRepository.findByName(request.getTeamname()).orElseThrow(IllegalArgumentException::new);
        if (request.getIsManager()) {
            updateManager(team, request.getName());
        }
        return team;
    }

📌 메서드 이름만 보면 단순히 이름을 가지고 팀을 찾는 동작을 할것 같은데, 안에 updateManager 메서드를 호출하고 있어 왜 이렇게 작성을 하신건지 궁금합니다! 여기서 updateManager 를 해주는 이유가 무엇을까요??

해결 과정 2.

findTeamByName이라는 메서드 내부에서 request의 isManager값을 검증하고
updateManager 메서드를 호출하고 있다.
이게 정말 클린한 코드일까?
아무리 현재 saveMember 의 작동 순서상 함께 검증을 해야 한다지만, 좋지 않은 설계인것 같다.
목표는 findTeamByName과 updateManager를 분리해서, 단일 책임 원칙과 코드 재사용성을 확보하는 것이다.

public Team findTeamByName(SaveMemberRequest request) {
       return teamRepository.findByName(request.teamName()).orElseThrow(IllegalArgumentException::new);
   }

@Transactional
   public void updateManager(Team team, Member member) {
       if (team.getManager() == null) team.updateManager(member.getName());
       else {
           Member previous_Manager = memberRepository.findByTeamIdAndRoleIsTrueAndIdNot(team.getId(), member.getId())
                   .orElseThrow(IllegalArgumentException::new);
           previous_Manager.changeRole();
           team.updateManager(member.getName());
       }
       teamRepository.save(team);
   }

💡 리팩토링 후, findTeamByName은 정말 말 그대로 팀을 이름으로 찾는 메서드 본연의 역할에 충실하게 되었다.

  @Transactional
   public void saveMember(SaveMemberRequest request) {
       Team team = teamService.findTeamByName(request);
       Member member = memberRepository.save(request.toEntity(team));
       if(request.isManager()) teamService.updateManager(team, member);
   }

saveMember 메서드에서 , findTeamByName과 updateManager를 수행하는 방식으로 변경하였다.

피드백 3.

 public void updateManager(Team team, String managerName) {
        if (team.getManager() == null) team.updateManager(managerName);
        else {
            Member member = memberRepository.findByTeamIdAndRoleIsTrue(team.getId())
                    .orElseThrow(IllegalArgumentException::new); 
            member.changeRole();
            team.updateManager(managerName);
        }
    }

📌 JPA 의 변경감지를 통해 업데이트를 진행하고 계신것 같아보입니다! 혹시 이부분 정상적으로 동작이 되나요?? 제가 알았던 바로는 변경감지 또한 트랜잭션 안에서 동작하게 되어있어 별도의 트랜잭션 설정이 없다면 동작을 안하는걸로 알고있었는데 제가 잘못알고 있던건지 궁금해서 여쭤봅니다.

해결 과정 3.

해당 부분은 조금 길어서 따로 작성하였다. - JPA 변경 감지와 커밋

피드백 4.

 public MemberController(MemberService memberService) {
        this.memberService = memberService;
    }

📌 이 부분은 롬복의 @RequiredArgsConstructor를 사용하면 어떤가요? @RequiredArgsConstructor을 사용하면 생성자를 정의하지 않고 애노테이션으로 생성해줍니다!!

해결 과정 4.

기존에 private final 으로 선언했던 Repository 나 service를 생성자를 통해 넣어줬는데,
롬복의 @RequiredArgsConstructor 를 통해 자동 주입 받아보자.

@RequiredArgsConstructor 장점과 단점

장점
1. 생성자 주입을 사용하는 경우에는 중복해서 작성해야 하는 슈가 코드를 줄일 수 있다.
2. 역으로 리팩토링하여 주입이 더 이상 필요하지 않을 때, 세 군대를 수정해야 할 것을 하나의 수정으로 해결 할 수 있다.
단점
1. 처음 보는 사람이라면 잘 이해가 안갈 수 있다.
2. 상속 받은 클래스에서는 적용이 안된다.
3. @Value값으로 사용하려고 하는 경우 실수로 Bean으로 주입을 받으려는 현상을 겪을 수 있다.

@Service
@RequiredArgsConstructor
public class TeamService {
   private final TeamRepository teamRepository;
   private final MemberRepository memberRepository;

💡 @RequiredArgsConstructor 를 통해 생성자로 넣어주는 코드를 줄일 수 있었다.

피드백 5.

   private String name;

   private String teamName;

📌 아래에 Team과 연관 관계 설정을 해주었는데 굳이 teamName을 남긴 이유가 있나요?

해결 과정 5.

이미 양방향 연관관계를 맺어줬는데, Response로 반환할때, member.getTeam().getName()
로 넘겨줄 수 있는데 어째서 따로 필드를 만들었는가?
정말 옳은 말인것 같다. 이미 필드 내에 Team을 매핑을 해뒀으니,
response로 나갈때 teamName이 필요하면 필드 내에 있는 Team에서 getName으로 값을 받아오면 되는데,
굳이 따로 teamName 컬럼을 만들 필요가 없는것이다.

public class Member {

    protected Member() {}

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private long id;

    private String name;

    private boolean role;

    private LocalDate birthday;

    @CreatedDate
    private LocalDateTime workStartDate;

    @ManyToOne
    private Team team;

    @Builder
    public Member(String name, boolean role, LocalDate birthday, Team team) {
        this.name = name;
        this.role = role;
        this.birthday = birthday;
        this.team = team;
    }

    public void changeRole() {
        this.role = !this.role;
    }
}

@Builder
public record GetAllMembersResponse(String name, String teamName, String role, LocalDate birthday,
                                    LocalDate workStartDate) {

    public static GetAllMembersResponse from(Member member){
        String isManager = member.isRole() ? "MANAGER" : "MEMBER";

        return GetAllMembersResponse.builder()
                .name(member.getName())
                .teamName(member.getTeam().getName())
                .role(isManager)
                .birthday(member.getBirthday())
                .workStartDate(member.getWorkStartDate().toLocalDate())
                .build();
    }
}

teamName을 제거한다.

Response를 반환할때 member.getTeam().getName() 로 TeamName을 반환한다.

피드백 6.

@Getter
public class SaveMemberRequest {
    private String name;
    private String teamname;
    private Boolean isManager;
    private LocalDate birthday;

    public Member toEntity(Team team) {
        return Member.builder()
                .name(this.name)
                .teamName(team.getName())
                .role(isManager)
                .birthday(this.birthday)
                .team(team)
                .build();
    }
}

📌 저는 자바 17을 사용하고 나서 dto는 거의 record로 구현하는 편인데, dto로 record를 사용하지 않고 구현하신 이유가 궁금합니다!!

해결 과정 6.

record에 대해 알게 된 게 최근이라 사용할 생각을 못했다..
한번 적용해보자..!

public record SaveMemberRequest (String name, String teamName, Boolean isManager, LocalDate birthday){
    public Member toEntity(Team team) {
        return Member.builder()
                .name(this.name)
                .role(isManager)
                .birthday(this.birthday)
                .team(team)
                .build();
    }
}

💡 코드가 깔끔해지는 장점이 있다..!

피드백 7.

public GetAllTeamsResponse toResponse() {
        return GetAllTeamsResponse.builder()
                .name(name)
                .manager(manager)
                .memberCount(memberList.size())
                .build();
    }

📌 GetAllTeamsResponse를 만들어 주는 것은 아래와 같이 dto 내부에서 만들어 줄 수 있는데, 도메인에 만들어 놓은 이유가 있을까요??
public GetAllTeamsResponse from(Team team) {
    return GetallTeamsResponse.builder()
            .name(team.getName())
            .manager(team.getManager())
           ...
}

해결 과정 7.

단순히 Entity에서 toDTO()를 통해 변환 과정을 처리하면 변환 로직이 어디있는지 예상할 수 있고,
유지보수 하기에도 편하지 않을까? 싶었는데, DTO에서 변환을 하면 내 방법의 완벽한 상위호환이 아닌가 싶다..
PR 리뷰를 통해 정말 많이 배우는 것 같다.

@Builder
public record GetAllMembersResponse(String name, String teamName, String role, LocalDate birthday,
                                    LocalDate workStartDate) {

    public static GetAllMembersResponse from(Member member){
        String isManager = member.isRole() ? "MANAGER" : "MEMBER";

        return GetAllMembersResponse.builder()
                .name(member.getName())
                .teamName(member.getTeam().getName())
                .role(isManager)
                .birthday(member.getBirthday())
                .workStartDate(member.getWorkStartDate().toLocalDate())
                .build();
    }
}

DTO 내부에서 builder를 통해 변환 로직을 처리한다.

@Transactional(readOnly = true)
    public List<GetAllMembersResponse> getAllMembers() {
        return memberRepository.findAll().stream().map(GetAllMembersResponse::from).toList();
    }

(GetAllMembersResponse::from) 를 통해 Response로 변환을 한다.

💡 builder를 DTO 내부에 만들어서 도메인과 분리도하고, 변환 로직도 어디서 이뤄지는지 파악하기 용이하다.

피드백 8.

  @Transactional
    public void createTeam(CreateTeamRequest request) {
        if (teamRepository.existsByName(request.getName())) throw new IllegalArgumentException("존재하는 팀입니다.");

📌 단순히 IllegalArgumentException을 사용하지 말고 커스텀 Exception을 만들어서 어느 예외인지 눈에 띄면 좋을 것 같습니다!

해결 과정 8.

해당 부분은 조금 길어져서 따로 작성하였다. - 스프링 예외 처리

피드백 9.

import java.util.Arrays;
import java.util.Map;
import java.util.stream.Stream;
@EnableJpaAuditing

📌 @EnableJpaAuditing 어노테이션을 main 메서드에다가 붙이는 방법 외에도
auditing 관련 설정 클래스를 만들어서 그 클래스에 적용하는 방법도 있더라구요!
참고 해주시면 좋을 것 같습니다 😆

해결 과정 9.

main 메서드에서 @EnableJpaAuditing 어노테이션을 제거한다.
config 디렉토리에 audit 디렉토리를 새로 생성한다.
AuditConfig 클래스를 생성 후, @Configuration , @EnableJpaAuditing 어노테이션을 붙여준다.

@Configuration
@EnableJpaAuditing
public class AuditConfig {
    // 추후 생성한 사람, 수정한 사람 기능 구현시 추가로 구현
    //[김기태의 JAVA를 자바](https://www.youtube.com/watch?v=D0c4t2NSYF4)
    //23년 스프링 (48) Auditing 설정 클래스 생성하기
}

💡 추후, 필요에 따라 CreatedBy나 UpdateBy 같은 생성한 사람, 수정한 사람 기능을 구현시
추가적인 설정을 AuditConfig 클래스에 할 수 있을것 같다.

피드백 10.

@Entity
@Getter
@EntityListeners(AuditingEntityListener.class)

📌 @EntityListeners(AuditingEntityListener.class)
위의 어노테이션을 엔티티위에 선언해서 사용하시는 방법도 있지만, 나중에 확장성을 고려해서
추상클래스에 붙이고 상속 받는 방법은 어떨까요?

해결 과정 10.

엔티티 위에 있는 @EntityListeners(AuditingEntityListener.class) 어노테이션을 제거한다.
Common 디렉토리를 생성 후, CreatedDateEntity를 생성한다.

@Getter
@EntityListeners(AuditingEntityListener.class)
@MappedSuperclass
public class CreatedDateEntity {
    @CreatedDate
    @Column(updatable = false, nullable = false)
    private LocalDateTime createdAt;
}

💡 @MappedSuperclass 란?
공통 매핑 정보가 필요할 때 부모 클래스에 선언된 필드를 상속받는 클래스에서 그대로 사용할 때 사용.
이때, 부모 클래스에 대한 테이블은 별도로 생성되지 않는다.

Member 클래스의 workStartDate 필드를 삭제 후, @AttributeOverride 어노테이션으로
CreatedDateEntity의 CreatedAt 필드와 테이블의 work_start_date칼럼을 매핑 시켜준다

@Entity
@Getter
@AttributeOverride(name = "createdAt", column = @Column(name = "work_start_date"))
public class Member extends CreatedDateEntity {

    protected Member() {}

    @Id
    @GeneratedValue(strategy = GenerationType.IDENTITY)
    private long id;

    private String name;

    private boolean role;

    private LocalDate birthday;

    @ManyToOne
    private Team team;

    @Builder
    public Member(String name, boolean role, LocalDate birthday, Team team) {
        this.name = name;
        this.role = role;
        this.birthday = birthday;
        this.team = team;
    }

    public void changeRole() {
        this.role = !this.role;
    }
}

후기

💡 리뷰를 통해 내가 미처 생각하지 못했던 더 좋은 프로그래밍 방법이나 앞으로 무엇을 더 배우면 좋을지 등등 정말 많이 배우는 것 같다.
용기를 내서 코드 리뷰에 참여하길 참 잘한 것 같다.
양질의 피드백을 남겨주시는 우리 리뷰 그룹원님들 너무 감사합니다 🙇

GitHub

김영훈

이전 포스트

[미니 프로젝트] _ Step 1.

다음 포스트

[코드 리뷰] - Day 1.

[인프런]_미니프로젝트

피드백 1.

해결 과정 1.

피드백 2.

해결 과정 2.

피드백 3.

해결 과정 3.

피드백 4.

해결 과정 4.

`@RequiredArgsConstructor` 장점과 단점

피드백 5.

해결 과정 5.

피드백 6.

해결 과정 6.

피드백 7.

해결 과정 7.

피드백 8.

해결 과정 8.

피드백 9.

해결 과정 9.

피드백 10.

해결 과정 10.

후기

[미니 프로젝트] _ Step 1.

[미니 프로젝트] _ Step 2.

0개의 댓글

[코드 리뷰] - Day 1.

[인프런]_미니프로젝트

피드백 1.

해결 과정 1.

피드백 2.

해결 과정 2.

피드백 3.

해결 과정 3.

피드백 4.

해결 과정 4.

@RequiredArgsConstructor 장점과 단점

피드백 5.

해결 과정 5.

피드백 6.

해결 과정 6.

피드백 7.

해결 과정 7.

피드백 8.

해결 과정 8.

피드백 9.

해결 과정 9.

피드백 10.

해결 과정 10.

후기

[미니 프로젝트] _ Step 1.

[미니 프로젝트] _ Step 2.

0개의 댓글

`@RequiredArgsConstructor` 장점과 단점