들어가며
- 책을 탈고하는 것처럼 코드도 탈고의 과정을 거친다. 이를 리팩토링이라 한다.
- 현재 운영 중인 프로젝트 리뷰하자(https://doreview.ga)이 기능 구현에만 초점을 맞춰서 리팩토링을 하지 못했다. 이번 기회에 하고자 한다.
- 인터넷이나 유튜브 등 다양한 매체로 나름 리팩토링에 대하여 학습하고 정리했다. 다음과 같이 리팩토링을 정리할 수 있었다.
리팩토링이란?
- 리팩토링이란 외부 동작을 바꾸지 않으며 내부 구조를 개선하는 방법.
- 협업하는 동료가 이해할 수 있는 코드를 작성.
- 그 외 성능을 높히거나 결합도를 낮추거나, 마이그레이션을 하거나, 중복을 제거하거나, 유지보수의 편의성을 높히거나, 새로운 기능을 추가하기 위해서 등 매우 다양한 이유로.
The Two Hats
- The Two Hats : 리팩토링과 기능 추가는 동시에 되어서는 안된다. 기능을 추가한 후 리팩토링을 해야 한다. 빠르게 기능을 추가하고 다시 빠르게 리팩토링하는 것이 더 효과적이다.
- 마이크로소프트의 리팩토링 기간 : 리팩토링 기간에는 기능 추가 등 업무를 일절 하지 아니함. 기술 구현을 위해 하지 못했던 리팩토링을 함(기술부채).
리팩토링의 방법들
명칭의 변경
- 이해할 수 있는 명칭으로 변경.
과다한 매개변수
- 매개변수가 복잡하면 객체를 통해 값을 넘긴다.
높은 결합도, 모듈이 다른 모듈에 크게 의존할 때
- 데이타의 통신 방식을 매개변수로 변경
하나의 기능이 여러 곳에 산재하여 체인 현상이 발생할 때
- 기능을 모아서 하나의 클래스나 매서드로 변경.
중복의 제거
- 여러 개의 메서드에 중복되는 코드가 발생할 경우 -> 중복되는 메서드를 하나의 메서드로 만든다.
- 자식 클래스 간 중복 코드가 있을경유 -> 부모 클래스 해당 값을 삽입하며, 중복되는 값을 부모클래스로부터 오버라이드 하도록 한다.
역할의 과소 과대
- 역할이 너무 작은 메서드와 클래스는 제거하고 역할이 너무 큰 메서드와 클래스는 나눈다.
- 하나의 클래스가 지나치게 많은 변수를 가지도록 하지 않는다.
- 하나의 메서드는 하나의 기능을 하도록 만든다.
상속 구조가 말못 되었을 때
- 상속이 잘못되거나 필요없는 상속이 있을 경우 삭제.
임시변수의 제거
- int c = a + b; return c;
- return a+b;
- 변수를 최소화 한다.
재작성
- 변경해야할 수준이 너무 많으면 새로운 메서드, 새로운 클래스로 작성하고 대체함.
시간 엄수
- 시간이 촉박할 때 기능이 작동한다면 리팩토링은 포기해야할 수도 있음.
if문을 사용할 때
- 가능한 긍정으로 시작함.
- 가능한 중요한 것부터 시작함.
- else문을 사용하지 않는다.
들여쓰기를 최소화한다
- 들여쓰기를 2번 이상 할 경우 메서드로 변경한다.
boolean과 XXXException
- 어떤 한 기능의 결괏값으로 true, null, 1, throw new exception 등 다양한 방식으로 반환한다. 어떤 방식이 가장 이상적일까?
- 하나의 기능의 완료로서 값을 반환 할 때는, throw new XXXException으로 처리한다. 이를 통해 결괏값의 성공과 실패를 분명하게 한다.
- 어떤 기능의 흐름 속에서 존재하는 한 분기의 성공과 실패를 나타낼 경우 boolean으로 반환한다.
실제 리팩토링의 과정
매서드를 하나로 통합 (Optional)
@Query(" select br " +
" from Borrow br " +
" where br.member.id =:memberId " +
" and br.book.id in " +
" (select bk.id" +
" from Book bk " +
" where bk.isbn =:isbn)")
boolean existBorrowByMemberAndIsbn(@Param("memberId")Long memberId, @Param("isbn") Long isbn);
@Query(" select br " +
" from Borrow br " +
" where br.member.id =:memberId " +
" and br.book.id in " +
" (select bk.id" +
" from Book bk " +
" where bk.isbn =:isbn)")
Borrow getBorrowByMemberAndIsbn(@Param("memberId")Long memberId, @Param("isbn") Long isbn);@Query(" select br " +
" from Borrow br " +
" where br.member.id =:memberId " +
" and br.book.id in " +
" (select bk.id" +
" from Book bk " +
" where bk.isbn =:isbn)")
Optional<Borrow> getBorrowByMemberAndIsbn(@Param("memberId")Long memberId, @Param("isbn") Long isbn);- 기존의 메서드는 해당 값을 객체(Borrow)와 boolean 두 개로 리턴을 하는 메서드가 있었다. 그러나 자바에서는 Optional 객체를 통해 해당 값이 존재하는지(isPresent()) 반환하는 메서드를 제공한다. 그래서 Optional 객체 하나를 반환하는 메서드로 통합했다.
return, if, exception 처리
(이전)
@Override
public BookDTO getBook(Long bookId) {
BookDTO bookDTO = null;
Optional<Book> byId = bookRepository.findById(bookId);
if(byId.isPresent()){
Book book = byId.get();
bookDTO = entityToDTO(book);
}else {
System.out.println("존재하지 않는 책");
}
return bookDTO;
}
(이후)
@Override
public BookDTO getBook(Long bookId) {
Optional<Book> byId = bookRepository.findById(bookId);
if(byId.isPresent()){
return entityToDTO(byId.get());
}
throw new IllegalStateException("DB에 해당 책이 존재하지 않습니다.");
}- 조건문이나 반복문, 혹은 try-catch 문을 통해 리턴할 값을 처리할 때, 습관적으로 해당 값을 밖에서 선언하고 밖에서 리턴했다. 리턴 값이 가장 하단에 있는 것이 뭔가 절차지향적이고 안정감 있어 보였던 것 같다. 그리고 습관적으로 if와 else는 항상 같이 작성했다.
- 아래의 코드를 보면 return을 if문 내부로 만들어서 외부에서 해당 값을 선언할 필요가 없다.
- else가 없어도 작동상 문제가 없다.
- throw new XXException을 통해 해당 명령이 실패했음을 분명하게 표현한다. 아무래도 BookDTO 객체를 null로 반환하는 것보다는 좀 더 분명한 응답이라 생각한다. 예외 처리에는 사용자 메시지를 넣을 수 있고, 사용자 예외 클래스를 만들어 활용할 수 있다.
지역변수 없이 바로 리턴하기
@Override
public List<BorrowedBookDTO> getHottestBooks() {
List<Object[]> result = bookRepository.getHottestBooks();
List<BorrowedBookDTO> result1 = result.stream().map(objects ->
entitiesToDTO((Book) objects[0], (Borrow) objects[1], (Long) objects[2])).collect(Collectors.toList());
return result1;
}
@Override
public List<BorrowedBookDTO> getHottestBooks() {
return bookRepository.getHottestBooks().stream()
.map(objects -> entitiesToDTO((Book) objects[0], (Borrow) objects[1], (Long) objects[2]))
.collect(Collectors.toList());
}- 위의 코드는 entity를 dto로 변환하는 과정이다.
- 굳이 지역변수를 만들 필요가 없이 바로 return 해도 된다.
매서드로 변환하기
@PostMapping(value = "", produces = MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity<Long> registerBookApi(@RequestBody BookDTO bookDTO, @RequestAttribute("jwtEmail") String jwtEmail) throws Exception {
(수정1)
int indexUnderbar = bookDTO.getTempIsbn().indexOf(" ");
String targetIsbn = bookDTO.getTempIsbn().substring(indexUnderbar + 1);
log.info("targetIsbn : " + targetIsbn);
(수정2)
DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd");
LocalDate dateTime = LocalDate.parse(bookDTO.getDateTime(), formatter);
bookDTO.setPubliDate(dateTime);
bookDTO.setIsbn(Long.parseLong(targetIsbn));
bookService.register(bookDTO);
Long borrowId = borrowService.registerByEmailAndIsbn(jwtEmail, bookDTO.getIsbn());
return new ResponseEntity<>(borrowId, HttpStatus.OK);
}
@PostMapping(value = "", produces = MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity<Long> registerBookApi(@RequestBody BookDTO bookDTO, @RequestAttribute("jwtEmail") String jwtEmail) {
bookDTO.setIsbn(getISBN13(bookDTO.getTempIsbn()));
bookDTO.setPubliDate(getLocalDate(bookDTO.getDateTime()));
bookService.register(bookDTO);
Long borrowId = borrowService.registerByEmailAndIsbn(jwtEmail, bookDTO.getIsbn());
return new ResponseEntity<>(borrowId, HttpStatus.OK);
}
- 수정1과 2를 보자. 해당 공공 api에서 isbn 값을 {isbn13}+" "+{isbn10} 형태로 리턴한다. 나에게 필요한 것은 isbn13이다. 해당 값을 isbn13으로 변환하는 과정이다. 수정2의 경우 LocalTime이 String 값으로 반환되는데, 이를 LocalDate로 변환하는 과정이다. 이 코드의 경우 YYYYMMDD만을 필요로 하기 때문이다.
- 한번에 이해하기 복잡한 코드를 메서드로 추출하고, 메서드명 하나로 간단하게 처리했다.
그 외 (JPA 사용 과정에서 경험들)
- 리팩토링에서 중복을 제거할 때 가장 좋은 것은 하나의 메서드가 다양한 조건을 수용할 수 있을 때라 생각한다. JPQL과 Spring Data JPA의 경우 그러한 기능을 제공한다.
- predicate : 두 개의 경우 predicate를 매개변수로 넣는 것을 허용한다. QueryDSL의 BooleanBuilder를 삽입할 수 있다. 이를 통해 where문을 넣는 효과를 만들어낸다.
- Paging : Spring Data JPA의 경우 매개변수에 페이징을 넣어, 페이징 처리도 가능하다. JPQL의 경우 리턴값을 미리 설정하기 때문에 불가능한 것 같다. (사족이지만, 만약 JPQL에서 페이징처리를 한다면 해당 메서드에 countQuery="select count(*) ...." 형태의 요소값을 추가하여 총 갯수를 밝혀야 한다. )
JAVA web developer