코드리뷰 적응기(feat. 파일럿 프로젝트) - 우아한형제들 기술 블로그

안녕하세요. 결제정산개발팀의 권세희입니다.
저는 정산업무를 맡게 되었는데, 정산은 데이터 정합성과 트랜잭션, 도메인 주도 개발, Entity 간의 연관 관계, 대용량 데이터 처리가 핵심인 시스템입니다.
정산 도메인과 기술셋에 대한 경험이 없어 들어가기에 앞서 파일럿 프로젝트를 먼저 하게 되었습니다.
정산 시스템을 만들고 코드리뷰를 받은 경험을 공유하고자 합니다.

정산 시스템

현재 정산 시스템은 크게 2가지 시스템으로 구분됩니다.

고객서비스실, 결제정산팀 등 정산 도메인 운영을 위한 정산 어드민
실제 업주분들께 드릴 돈을 계산하고, 펌뱅킹으로 전달등을 하는 정산 핵심 비지니스 로직을 담고 있는 정산 배치 시스템

실제 돈을 지급하는 시스템이다 보니, 테스트 코드는 필수입니다.
(팀 내 가장 많은 테스트 코드를 갖고 있는 시스템입니다.)
파일럿 프로젝트는 이에 중점을 맞춘 주제로 진행됩니다.

요구사항

필수 기능
- 회원 기능
- 업주 관리 기능
- 매출 관리 기능
- 매출 상세 관리 기능
- 지급 관리 기능
필수 기술
- OOP (객체지향) 코드
- 단위 테스트
- RESTFul API
- Spring Boot 2.x
- JPA
- Gradle
- Lombok
- Git & Github
- Git Flow 브랜치 전략
- JIRA
- H2

여기에 선택 기술인 Querydsl, Spring Security를 추가했습니다.
이 이후에는 피드백을 반영하고 멀티프로젝트를 만들고 집계기능을 Spring Batch로 만드는 순으로 진행했습니다.

프로젝트 하면서 …

파일럿 프로젝트로 사용해야 하는 기술셋은 제가 해왔던 업무와 겹치는 것이 적어 처음엔 너무 막막했습니다.
2주라는 일정내에 처음 접하는 기술을 공부하면서 하기에는 역부족이 아닐까 생각했습니다.

만드는 것에만 신경 쓸 뿐 아니라 Git Flow브랜치 전략을 위해 다음과 같은 이쁜 그림을 만들면서 진행했습니다.
(저만 이쁜가요? ㅎㅎ)

대망의 코드리뷰

일정 내에 완성을 하긴 했습니다. 하지만 (공포의) 코드리뷰가 있었고, 이렇게 많이 지적받아도 되나 싶을 정도로 코드는 형편없었습니다.
만드는 것에 급급하여 제가 짠 소스를 다시 리팩토링 할 시간조차 없었던 것도 사실이지만, 제가 평소에 코딩습관이 좋았더라면 결과는 달랐을 거라고 생각합니다.

이 때 제가 받았던 피드백들을 공유하고자 합니다. 보시기에 당연한 것을 썼다고 생각하실 수도 있지만, 저처럼 코드리뷰가 익숙하지 않은 분들은 참고로 봐주시면 좋을 것 같습니다.

Entity

다음은 Sales(매출 데이터)Entity 입니다. 아래의 Entity 에서만 많은 피드백을 받았었습니다. 피드백 내용을 봐주세요.

AS-IS

@Data
@Entity
@NoArgsConstructor
public class Sales {

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

  @ManyToOne
  @JoinColumn(name="owner_id", referencedColumnName = "id")
  private Owner owner;

  private long salesPrice;

  @OneToMany(fetch = FetchType.LAZY)
  @JoinColumn(name = "sales_id")
  private List<Payment> paymentList = new ArrayList<>();

  private LocalDateTime paymentDate;

  @JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss")
  @CreationTimestamp
  private LocalDateTime createdDate;

  @JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss")
  @UpdateTimestamp
  private LocalDateTime updatedDate;

  private String deleted;

}

Lombok 의 어노테이션으로 @Data 를 사용했는데요. 많은 기능이 포함되어 있어서 사용했었습니다. 하지만 실무에서는 @Data를 지양합니다. 무분별한 Setter의 남용를 막고, 도메인 기능에 대한 메소드들만 사용하도록 합니다.
paymentDate 는 결제일시를 나타내기 위해 타입을 LocalDateTime으로 했는데요. paymentDate 라는 네이밍으로는 타입과 일치하지 않아 맞추는 것이 좋다고 합니다. LocalDateTime 이면 paymentDateTime, LocalDate 이면 paymentDate 이렇게 하면 필드명만 보고도 타입을 유추할 수 있습니다.
LocalDateTime 데이터를 웹에서 보여줄 때 yyyy-MM-dd HH:mm:ss 형태로 보여주고 싶어서 JsonFormat 을 사용했는데요. 하지만 Entity를 건드리기보다 이 Entity의 ResponseDto 에서 JsonFormat을 사용하여 데이터를 조작해야합니다. Entity는 우리가 지켜야할 고유의 영역입니다. 만약 이 Entity를 쓰는 API가 신규로 추가되고 그 API에서는 yyyyMMdd HH:mm:ss 로 포맷이 필요하면 어떻게 해야할까요? Entity를 변경하기 보다는 이 Entity를 사용하는 DTO를 사용하여 최대한 변경에 대응하기 쉽도록 합니다.
deleted 라는 필드는 삭제여부를 나타내고, “Y”, “N”으로 데이터를 두었었습니다. 하지만 이런 플래그를 사용할 때는 String 을 사용하지 않습니다. 다른 문자도 들어갈 수 있기때문에 boolean 타입을 사용해야 합니다.
결제정보인 Payment Entity와, 업주정보인 Owner Entity 는 Sales Entity와 연관관계를 맺고 있는데요. 위의 코드 와 같이 중간 중간에 위치하고 있어 가독성이 떨어집니다. 실제로 확인할 컬럼을 보기가 어려운데요. 연관관계는 하단으로 내려서 가독성을 좋게 하는 것이 좋습니다.
NoArgsConstructor 접근권한을 주지 않았었는데요. JPA에서는 프록시를 생성을 위해서 기본 생성자를 반드시 하나를 생성해야합니다. 이때 접근 권한을 AccessLevel.PROTECTED로 설정하여 JPA에서의 Entity 클래스 생성만 허용해줍니다.

다음의 소스는 이렇게 변했습니다.

TO-BE

@Getter
@Entity
@NoArgsConstructor(access = AccessLevel.PROTECTED)
public class Sales extends AbstractTimeEntity {

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

    private Long salesPrice;

    private LocalDateTime paymentDateTime;

    private Boolean deleted;

    @ManyToOne
    @JoinColumn(name="owner_id", referencedColumnName = "id")
    private Owner owner;

    @OneToMany(mappedBy = "sales", cascade = CascadeType.ALL)
    @JoinColumn(name = "sales_id")
    private List<Payment> paymentList = new ArrayList<>();

...
}

확실히 이 전보다 가독성이 좋고 어떤 필드가 있는지 명확해 보입니다.

동적 QueryDsl

정산어드민의 요구사항 중에 검색 상세페이지를 구현하기 위해 셀렉트박스를 마구 넣었습니다. 데이터를 업주아이디로 검색하거나 업주명, 업소이름으로 검색할 수 있고, 기간 검색, 금액 검색 등등을 할 수 있게 만들었습니다.(일단 프론트는…)
하지만 JPA 로는 이 부분을 어떻게 구현할 지 몰라 다음과 같은 소스를 만들었습니다.
(사실 이것도 한번 개선한 건 안비밀…)

AS-IS

public class SalesRepositoryImpl extends QuerydslRepositorySupport implements SalesRepositoryCustom {
...
    public BooleanBuilder addSearchBuilder(BooleanBuilder builder, QSales sales, SearchType searchType, String searchText) {

        if (!StringUtils.isEmpty(searchText)) {
            if (SearchType.OWNER_ID.equals(searchType)) {
                Long ownerId = Long.valueOf(searchText);
                builder.and(sales.owner.id.eq(ownerId));
            } else if (SearchType.OWNER_NAME.equals(searchType)) {
                builder.and(sales.owner.name.contains(searchText));
            } else if (SearchType.SHOP_NAME.equals(searchType)) {
                builder.and(sales.owner.shopName.contains(searchText));
            }
        }
        return builder;
    }
...
}

Null인지 체크하고 검색타입에 따라 SQL문을 추가하듯이 작성했는데요. 한 분은 JPA를 mybatis 처럼 사용했다는 의견을 주셨습니다… if문 안에 또 if문이 있고 전체적으로 가독성이 떨어지는 소스입니다.

TO-BE

public class SalesAdminRepositoryImpl extends QuerydslRepositorySupport implements SalesAdminRepositoryCustom {
...

    private BooleanExpression conditionalOwnerId(SalesSearchRequestDto dto) {
        if (dto.isEmptySearchType() || dto.isNotOwnerId()) {
            return null;
        }
        if (dto.isEmptySearchText()) {
            return null;
        }
        return sales.owner.id.eq(Long.valueOf(dto.getSearchText()));
    }

    private BooleanExpression conditionalOwnerName(SalesSearchRequestDto dto) {
        if (dto.isEmptySearchType() || dto.isNotOwnerName()) {
            return null;
        }
        if (dto.isEmptySearchText()) {
            return null;
        }
        return sales.owner.name.eq(dto.getSearchText());
    }

    private BooleanExpression conditionalShopName(SalesSearchRequestDto dto) {
        if (dto.isEmptySearchType() || dto.isNotShopName()) {
            return null;
        }
        if (dto.isEmptySearchText()) {
            return null;
        }
        return sales.owner.shopName.eq(dto.getSearchText());
    }
...
}

mybatis 같았던 소스는 위 처럼 바뀌게 됩니다. 어떤 것을 검색하고자 하는지 더 세부적으로 쪼개고, 목적에 맞게 네이밍하고, early exit 구문을 통해 간결하게 바뀌었습니다.
Querydsl의 BooleanExpression을 null값으로 리턴하면 where 조건문에서 제거됩니다. 이것을 이용하여 동적 쿼리를 쉽게 작성할 수 있습니다.

@Autowired 지양

의존 관계를 설정할 때 대부분을 @Autowired를 사용했었습니다. 이렇게 할 경우 Spring Framework에 종속적이기 때문에 @Autowired 는 지양하고, 아래와 같은 생성자를 통해 주입합니다.

public class RestSalesController {
    private final SalesService salesService;

    public RestSalesController(SalesService salesService) {
        this.salesService = salesService;
    }   
}

생성자의 인자가 많아지는 경우에 final을 받는 파라미터를 생성자로 만들어주는 @RequiredArgsConstructor을 사용해도 됩니다. 다만 이때는 인자의 추가나 순서 변경시 생성자 클래스가 변경되는 점에 주의해야합니다.

@RequiredArgsConstructor
public class RestSalesController {
    private final SalesService salesService;
}

친절한 테스트 코드

테스트 코드 작성중에 데이터를 저장하기 위해 메소드 내에 builder 를 사용했습니다. 이 때, 어떤 것을 저장해서 테스트하고자 하는지 다른 사람이 봤을 때 이해하기 쉽지 않다는 피드백을 받았습니다.
테스트 결과를 확인하는 부분도 메소드로 고쳐야 할 것 같습니다.

AS-IS

@Test void PaymentType별_금액_집계() throws Exception {
  paymentRepository.save(Payment.builder().sales(sales).paymentType(PaymentType.CARD).price(15000L).cardType("LGUPLUS").build());
        paymentRepository.save(Payment.builder().sales(sales).paymentType(PaymentType.POINT).price(5000L).build());
        paymentRepository.save(Payment.builder().sales(sales2).paymentType(PaymentType.CARD).price(13000L).cardType("LGUPLUS").build());
        paymentRepository.save(Payment.builder().sales(sales2).paymentType(PaymentType.POINT).price(8000L).build());
        paymentRepository.save(Payment.builder().sales(sales2).paymentType(PaymentType.COUPON).price(9000L).couponType(CouponType.OWNER).build());

        JobParameters jobParameters =
                new JobParametersBuilder()
                        .addString("aggregateDate", localDateToString(sumDate))
                        .toJobParameters();

        JobExecution jobExecution =  jobTestUtils.getJobTester(JOB_NAME).launchJob(jobParameters);

        assertThat(jobExecution.getStatus(), is(BatchStatus.COMPLETED));
        List<PaymentTypeSum> list = paymentTypeSumRepository.findAll();

        long allPrice = 0L;
        long cardPrice = 0L;
        long pointPrice = 0L;
        long couponPrice = 0L;

        for (PaymentTypeSum temp : list) {
            allPrice += temp.getSumPrice();
            if (PaymentType.CARD == temp.getPaymentType()) cardPrice = temp.getSumPrice();
            if(PaymentType.POINT == temp.getPaymentType()) pointPrice = temp.getSumPrice();
            if (PaymentType.COUPON == temp.getPaymentType()) couponPrice = temp.getSumPrice();
        }

        assertThat(allPrice, is(50000L));
        assertThat(cardPrice, is(28000L));
        assertThat(pointPrice, is(13000L));
        assertThat(couponPrice, is(9000L));        
}

알아보기 힘든 위의 테스트코드에서 핵심값만 저장하는 메소드를 만들고, 결과를 비교하는 부분도 메소드를 만들었습니다.
어떤 것을 저장해서 확인하려고 하는지 이 전보다는 잘 보입니다.

TO-BE

@Test
public void PaymentType별_금액_집계() throws Exception {
    //given
    savePayment(PaymentType.CARD, 15000L, sales);
    savePayment(PaymentType.CARD, 13000L, sales2);
    savePayment(PaymentType.POINT, 5000L, sales);
    savePayment(PaymentType.POINT, 8000L, sales2);
    savePayment(PaymentType.COUPON, 9000L, sales2);

    JobParameters jobParameters =
            new JobParametersBuilder()
                    .addString("aggregateDate", localDateToString(sumDate))
                    .toJobParameters();

    //when
    JobExecution jobExecution =  jobTestUtils.getJobTester(JOB_NAME).launchJob(jobParameters);

    //then
    List<PaymentTypeSum> list = paymentTypeSumRepository.findAll();
    assertThat(jobExecution.getStatus(), is(BatchStatus.COMPLETED));
    assertThat(getPaymentTypeSum(list, PaymentType.CARD), is(28000L));
    assertThat(getPaymentTypeSum(list, PaymentType.POINT), is(13000L));
    assertThat(getPaymentTypeSum(list, PaymentType.COUPON), is(9000L));
}

또한, 위처럼 테스트 코드를 짤 때 JUnit에서는 강제하지 않지만 given, when, then 주석을 달고 명확하게 작성해야 합니다.
//given 은 테스트에 필요한 상황, //when 은 테스트하려는 일의 동작, //then 은 when에서 발생한 일의 결과라고 보시면 됩니다.

매번 달기가 귀찮다고 생각할 수 있지만, IntelliJ의 File Templates를 사용하면 메소드 생성시 자동으로 주석이 추가되니 번거롭지 않습니다.

테스트코드에서 추가적으로 @Before이나 @After 가 붙은 메소드는 모르는 사람이 봤을 때, 테스트의 전과 후에 어떤 일을 하는지 명확히 하기 위해 맨 위에 작성해주는 것이 좋습니다.

DTO 잘 사용하기

Dto는 사용에 따라 최대한 쪼개야 합니다. 예를 들어 하나의 Entity를 용도에 따라 보여주는 부분이 다를 때, 화면에서의 리스트 컬럼과 엑셀 다운로드 시 보여줄 컬럼들이 다를 때 필드가 1-2개 달랐지만 하나의 Dto로 사용했었습니다. 각각의 목적에 따라 Dto를 만들어야 합니다.
또한, Dto는 용도와 레벨에 맞는 곳에 위치해야 합니다. 저는 dto란 패키지를 만들고 그 안에 모든 dto를 두었었습니다. 하지만 어느 dto는 controller 단까지, 어느 dto는 서비스 레벨까지 사용할 수 있습니다. 이런 용도와 레벨에 맞게 위치시켜야 합니다.
Payment Entity를 저장하기 위해 웹에서 값을 받는 PaymentRequestDto 를 만들었습니다. 여기서 금액을 저장하는 필드를 long 타입으로 정했었는데요. 이렇게 원시타입으로 하게 되면 Null값이 들어와도 0으로 들어와 문제를 감지하지 못합니다. 사용자에게 입력받는 RequestDto는 레퍼런스타입으로 지정해야합니다.

등등 이 외에도 많은 내용이 있었습니다. 여러 차례 받았던 리뷰를 한번에 정리하려다 보니 내용이 중구난방한 점 양해 부탁드립니다.

마치며

파일럿 프로젝트를 하면서 처음에는 막막하고 부담감을 가지고 진행했었고, 완성 후에는 완성도가 많이 떨어지는 것 같아 많이 아쉬웠습니다.
하지만 돌이켜 봤을 때 다시는 겪지 못할(?) 뜻깊은 시간이었던 것 같습니다.

프로젝트를 진행 중 삽질을 해도 질문을 하면 안되는 저는 이 블로그의 도움을 정말 많이 받았습니다. jojoldu님 짱!
또한, 애정으로 리뷰해주신 팀원분들께도 감사드립니다!
그럼 읽어주셔서 감사합니다. 앞으로 코드컨벤션을 잘 지키는 정산쟁이로 거듭나겠습니다.