서론

이번 포스트가 '배포'가 아닌 '개발'인 이유는 이 SimpleTodoList 프로젝트를 전반적으로 재구성하고 수정하는 중이기 때문이다. 최근 열심히 참여하고 있는 백엔드 데브코스에서 코드 리뷰를 진행하거나 개인적으로 개발 챌린지 등에 참여하면서 다른 사람의 코드를 많이 보게됐는데 확실히 혼자 우물 안에서 작성한 코드는 지금 보니 정말 맘에 안드는 부분이 많았다.

그래서 오늘은 그 중에서 어떤 부분을 수정하고 그 이유와 결과를 적어보고자 한다.

본론

먼저 대부분의 코딩 스타일 가이드는 IntelliJ의 'SonarLint' 플러그인을 따랐다. 그 외에는 개인적으로 생각한 방향이 대부분이다.

회원 엔티티 디자인

어떤 개념을 어떻게 부르냐는 논란이 될 수 있는 부분이라고 생각하지만 나는 현재로서는 사용자의 식별자는 username 이라고 생각한다. 기존에는 데이터베이스 레코드의 식별자는 id, 사용자의 아이디는 userId, 사용자의 이름은 username이라고 생각했다. 왜냐면 사용자 입장에서는 항상 아이디로 자신을 식별할 수 있고 아이디와 비밀번호로 로그인했기 때문에 사용자(user)의 아이디(id)는 userId라고 생각했기 때문이었다.

그런데 스프링 시큐리티를 적용하고 사용자 정보(UserDetails)를 다루다보니 사용자의 식별자를 userId라고 부르는게 좀 헷갈릴 수 있다는 생각이 들었다. 특히 userId가 문자열 아이디를 말하는 것인지 데이터베이스 레코드의 정수형 식별자를 말하는 것인지 나 스스로도 명칭이 정립되지 않았다.

그래서 사용자의 고유한 식별자는 username이란 이름으로 두고 기타 부가적인 정보(이름, 생일 등)를 username 대신 alias 처럼 속성에 맞게 구분되는 필드에 저장하는 방식으로 다시 디자인했다. 이런 포스트에서도 볼 수 있듯이 User ID는 흔히 생각하는 ladiesman217 같은 사용자가 입력하는 아이디가 아니라 데이터베이스 레코드의 식별자나 BACDF1551 같은 비즈니스 키를 칭하는 것 같다.

데이터베이스 컬럼은 소문자로

이 글에서도 나왔던 얘기고 데브코스 슬랙에서도 나왔던 주제인데 데이터베이스 쿼리는 대문자로, 테이블이나 컬럼같은 구성 요소 이름은 소문자로 작성하는 것이 좋다고 한다. 나는 개인적으로 쿼리의 모든 문자를 대문자로 쓰는 편인데(멋있어서...) SQL과 레코드는 좀 구분이 잘 가도록 케이스를 구분하는 것도 좋은 습관이라 생각한다.

사실 엔티티에서 컬럼명을 대문자로 작성해도 데이터베이스에는 자동으로 소문자로 변경되서 적용됐기 때문에 소문자로 작성하는 편이 맞는 것 같기도 하다.

검증은 종단에서

말이 어렵지만 검증, 즉 validation은 종단, terminal 즉 제일 마지막 단계에서 통합해서 수행한다는 것이다. 이건 데브코스 멘토님의 조언이라 내가 경험한 내용은 아니지만 충분히 공감할 수 있는 내용이었는데 예를 들어 다음처럼 엔티티의 이름을 변경하는 서비스가 있다고 하자.

만약 종단, 즉 엔티티에서 검증하지 않고 서비스에서 입력값을 검증(null이거나 빈 문자열)한다면 이 엔티티의 이름을 변경하는 모든 서비스나 다른 구성요소에서 입력값을 검증하는 로직이 그림처럼 반복되어야 한다. AOP 적으로 생각하면 횡단 관심사인 것이다.

대신 위처럼 엔티티에서 입력값을 검증하게 된다면 굳이 서비스나 다른곳에서 검증할 필요 없이 통합된 곳에서 검증을 적용할 수 있다. 물론 해당 구성 요소 자체적으로 입력값을 활용한다면 검증이 필요하겠지만 그 경우 이는 횡단 관심사가 아닐 것이다.

아무튼 이런 식으로 변경자에 다음처럼 검증 로직을 추가하는 방식으로 변경하였고 좀 더 안전한 코드를 작성할 수 있었다.

public void changeTeamName(@NonNull String teamName) {
    if(teamName.isBlank()) { 
        throw new IllegalArgumentException("Changed team name cannot be blank.");
    }
    this.teamName = teamName;
}

예외 클래스 디자인

SonarLint S1165 사항을 반영했다.

@Getter
@Setter
@NoArgsConstructor
@AllArgsConstructor
public class DuplicatedMemberException extends RuntimeException{
    private String error = Member.DUPLICATED_MEMBER_FOUND;
    private String message = "Try with different user identification value.";
}

SimpleTodoList는 회원제로 운영되고 있기 때문에 이미 가입된 아이디로는 가입할 수 없다. 이 경우 중복된 ID가 있다는 것을 나타내는 예외를 정의해서 에러의 이름과 세부 사항을 나타내는 필드 error, message로 활용하고 있다.

그렇지만 플러그인에서는 예외 클래스를 불변(immutable) 클래스로 작성하는 것을 권장하고 있다. 그 이유는 예외, Exception이라는 것은 단순히 텍스트를 담는게 아니라 문제가 발생한 상황을 포착해서 기록하고 있어야 하기 때문이다.

만약 위의 @Setter 어노테이션처럼 필드에 대한 setter 메서드를 추가했다면 나중에 예외 처리기나 다른 구성 요소에서 이를 변경할 수 있다. 그렇다면 이 예외 클래스가 담고 있는 기록이 변조될 수 있기 때문에 이를 변경할 수 없도록 불변 클래스로 디자인해야 하는 것이다.

그리고 굳이 RuntimeException의 message 필드를 활용하지 않고 별도의 필드를 정의할 필요가 없다고 생각해서 불필요한 필드를 삭제하고 생성자로 필요 파라미터를 받아서 메시지를 생성하면 좋을 것 같다고 생각했다.

그래서 이를 반영하면 다음과 같다.

@Getter
public class DuplicatedMemberException extends RuntimeException {
    public DuplicatedMemberException(String username) {
        super(String.format("Username %s already exists. Try another id.", username));
    }
}

불필요한 필드와 변경자도 줄어들고 예외의 목적에 맞는 파라미터를 받도록 변경해서 좀 더 깔끔하게 작성할 수 있었다.

@RestControllerAdvice, @ControllerAdvice

내 첫 개인 프로젝트였던 SimpleBBS는 API로 통신하지 않고 전통적인 폼 제출 방식을 사용했기 때문에 일반 @Controller 였으며 예외 처리기도 @ControllerAdvice를 사용했다. 이 습관을 따라서 API로 통신하던 SimpleTodoList에서도 예외 처리기에 @ControllerAdvice를 활용했는데 이 프로젝트에서는 요청과 응답을 모두 JSON 바디로 처리해야 했기 때문에 @ExceptionHandler에 일일히 @ResponseBody를 붙여두었다. 그리고 HTTP 상태 코드도 설정하기 위해 @ResponseStatus 어노테이션을 사용했다.

@ControllerAdvice
public class MemberExceptionHandler {
    @ExceptionHandler(NoMemberFoundException.class)
    @ResponseStatus(HttpStatus.NOT_FOUND)
    @ResponseBody
    public ExceptionResponseDTO noMemberFound(NoMemberFoundException exception) {
        return new ExceptionResponseDTO(exception.getError(), exception.getMessage());
    }
    ...
}

그런데 좀 더 알아보다 보니 @RestControllerAdvice라는 어노테이션이 존재하는 것을 알 수 있었다. 공식 문서를 참고하니 이 어노테이션은 @ControllerAdvice에 @ResponseBody가 붙어있는 것과 동일하다고 한다. 즉 위의 코드처럼 일일히 @ResponseBody 어노테이션을 붙일 이유가 없는 것이다.

그리고 스프링 MVC에서는 ResponseEntity라는 클래스를 제공하는데 공식 문서에서는 HTTP 상태 코드와 헤더, 바디를 직접 설정할 수 있는 HTTP 요청, 응답용 객체라고 한다. 즉 이를 활용하면 위처럼 일일히 @ResponseStatus, @ResponseBody 어노테이션을 사용하지 않아도 되는 것이다.

하지만 현재 예외 응답에는 HTTP 헤더를 설정할 일은 없기 때문에 @RestControllerAdvice로 응답을 JSON 바디로 변환한 후 @ResponseStatus로 HTTP 상태 코드만 붙이는 편이 더 깔끔하다고 생각했다. 이를 반영해서 예외 처리기를 다음처럼 변경할 수 있었다.

@RestControllerAdvice
public class MemberExceptionHandler {
    @ExceptionHandler(NoMemberFoundException.class)
    @ResponseStatus(HttpStatus.NOT_FOUND)
    public ApiResponse<Object> noMemberFound(NoMemberFoundException exception) {
        return ApiResponse.fail(exception.getMessage());
    }
    ...
}

여기에 위의 예외 클래스 디자인 변경 사항과 통합된 API 응답 객체를 활용해서 좀 더 일관적인 코드를 작성할 수 있었다.

불필요한 예외 제거

애플리케이션은 인증과 인가가 적용되어 있기 때문에 권한이 없는 사용자가 제한된 리소스(팀, 할 일 리스트 등)를 조회하거나 수정하는 경우 예외를 정의해서 발생시켰다. 그러다보니 예외가 다음처럼 많이 정의되었다.

• 수정 불가능한 할 일 접근 예외(LockedTodoException)
• 할 일 작성자 권한 필요 예외(NotTodoWriterException)
• 수정 불가능한 팀 접근 예외(LockedTeamException)
• 팀장 권한 필요 예외(NotTeamLeaderException)
• ...

이 외에도 비즈니스 로직에 따라 발생할 일 없는 예외(중복된 팀 생성 예외 등)도 몇가지 정의되어 있었는데 이런 예외들을 전부 제거하거나 통합하기로 마음먹었다.

일단 접근 권한 관련된 예외는 도메인(회원, 할 일, 할 일 리스트 등)에 따라 각각 AccessException 타입으로 통일했다.

• NotTeamLeaderException -> TodoAccessException
• LockedTodoException, NotTodoWriterException -> TodoAccessException

그리고 발생할 수 없는 예외나 이미 다른 곳에서 정의된 예외는 전부 제거했다. 예를 들어 회원이 팀에 가입할 때 이미 가입된 팀이라면 예외가 발생하는데 나는 이걸 회원 관점에서 발생하는 예외와 팀에서 회원을 초대해서 가입시킬 때 발생하는 예외 두 가지로 중복 작성했었다. 그렇기 때문에 서비스 측에서도 어떤 예외를 사용해야 하는지 모호했던 감이 있었기 때문에 둘 중 하나만 남기는 방식으로 통일했었다.

요즘 읽고 있는 이펙티브 자바에도 예외 활용과 관련된 내용이 있는 것 같다. 나중에 읽어보고 참고해야겠다.

API 응답 클래스 정의

기존에는 API마다 응답 형태가 조금씩 달랐는데 사용자를 조회한다면 사용자 아이디, 이름 등이 반환되고 할 일 리스트를 조회한다면 할 일 리스트의 이름, 포함된 할 일 목록 리스트가 반환되는 식이었다. 이를 좀 더 추상화하여 응답 객체와 메시지 필드를 포함하는 최상위 응답 클래스를 정의해서 사용했다.

왜냐면 성공이든 실패든 API 응답에는 실제 데이터와 별도 안내 사항을 담는 메시지 필드가 필요하다고 생각했기 때문이다. 요청이 성공한 경우 실제 데이터에 기능별 데이터를 담고 메시지에는 선택적으로 API 관련 안내 사항을 담을 수 있을 것이다. 요청이 실패한 경우 실제 데이터에는 선택적으로 상세 오류 정보(상황, 원인, 해결책 등)를 담고 메시지에 오류 메시지를 담을 수 있을 것이다.

이를 구현한 클래스는 다음과 같다.

@Getter
@AllArgsConstructor
public class ApiResponse <T> {
    private final boolean success;
    private final T result;
    private final String message;

    public static <U> ApiResponse<U> success(U result) {
        return new ApiResponse<>(true, result, "");
    }

    public static <U> ApiResponse<U> fail(String message) {
        return new ApiResponse<>(false, null, message);
    }

    public static <U> ApiResponse<U> fail(U result, String message) {
        return new ApiResponse<>(false, result, message);
    }
}

사실 이건 예전에 네이버 웹툰 개발 챌린지에 참가한 적이 있었는데 거기서 본 코드에 영감을 받아서 적용한 부분이다. 따로 뚜렷한 근거나 경험으로 결정한 것은 아니지만 좋은 습관이라 생각했기 때문에 적용했다.

그래도 이 클래스를 활용해서 정적 제너릭스 메서드로 좀 더 직관적인 응답을 생성할 수 있었다. 나중에 프론트 엔드 사이트를 개발하게 될 때 실제로 사용해보고 보완해야겠다고 생각하고 있다.

읽기 전용 DTO

예전에 읽었던 어느 책에서 엔티티는 서비스와 리포지토리 영역을 벗어나면 안 된다는 얘기를 들었다. 왜냐면 서비스까지는 비즈니스 로직이라 그렇다 쳐도 컨트롤러부터는 사용자 요청을 받고 응답을 생성해주는 역할이기 때문에 비즈니스 로직 외에서 비즈니스 객체가 변하면 안 된다는 것이었다.

이 외에도 엔티티를 컨트롤러에서 응답으로 직접 활용하고 있을 때 필드, 즉 컬럼이 추가되거나 삭제된다면 수많은 클라이언트에게 직접적으로 영향을 끼칠 수 있다. 즉 API 스펙 자체가 변경되어 버릴 수 있기 때문에 애플리케이션 내부에서 처리하는 객체와 클라이언트에게 응답으로 표현하는 방식을 달리 두자는 것이 DTO 구성 요소의 목적이라고 본 적이 있다.

JPA는 트랜잭션 내부에서 동작하기 때문에 컨트롤러가 값을 변경한다고 반영될 일은 없을 것이다. 하지만 수많은 클라이언트가 사용할 수 있는 API 스펙을 안전하게 유지하자는 아이디어는 좋은 방안이라고 생각해서 DTO를 아래처럼 작성해서 활용했었다.

@Getter
@Setter
@NoArgsConstructor
public class MemberDTO {

    @Builder
    @Getter
    @Setter
    @NoArgsConstructor
    @AllArgsConstructor
    public static class Response {
        @JsonProperty("id")
        long id;
        ...
    }

    @Builder
    @Getter
    @Setter
    public static class LoginRequest {
        @NotBlank(message = "사용자 ID는 비워둘 수 없습니다")
        @Length(max = 32, message = "아이디는 32 글자를 초과할 수 없습니다.")
        @JsonProperty("userId")
        String userId;

        @NotBlank(message = "사용자 비밀번호는 비워둘 수 없습니다.")
        @Length(max = 64, message = "비밀번호는 64 글자를 초과할 수 없습니다.")
        @JsonProperty("password")
        String password;
    }
    ...

그런데 위의 LoginRequest 클래스는 왜 여기 있는걸까? 이게 첫번째 실책이었다. DTO의 Data Transfer Object라는 이름 그대로의 의미에만 집착해서 클래스를 단순히 필드를 담는 용도로만 생각했기 때문이다.

그래서 위의 내부 클래스처럼 별 생각없이 비슷한 느낌의 클래스를 한 곳에 몰아서 적용했는데 컨트롤러에서 필요한 바인딩 클래스(LoginRequest)를 엔티티 DTO에서 정의하는 것도 이상하다는 생각이 들었다.

또다른 문제는 상황에 따라 다른 필드를 가진 DTO를 작성해서 활용하자는 것이었다. 예를 들어 로그인했을 때는 사용자 정보와 JWT를 담는 필드를 반환하고 단순 계정 정보 조회때는 사용자 정보만 반환하는 식으로 분리하는 식이었다.

하지만 이것도 모든 상황에 따라 정의하려면 불필요한 필드 중복이 많아지고 상속으로 해결하기에도 어떨때는 필요없고 어떨때는 필요한 경우가 많아서 까다로웠다. 결정적으로 얕은 프론트엔드 경험(단순 AJAX)으로 API 스펙을 디자인했기 때문에 어떤 화면에서 어떤 식으로 데이터가 필요한 지 스스로도 정의하지 못하고 있다는 생각이 들었다.

그래서 일단 DTO 클래스는 엔티티의 필수 요소만 가지도록 변경하였다. 예를 들어 기존에는 멤버를 조회하면 해당 멤버가 속한 팀의 목록도 같이 DTO에 담아 반환했지만 이번에는 네트워크 요청을 조금 더 쓰더라도 직접 팀 서비스에 조회하도록 간소화한 것이다.

그리고 DTO는 엔티티의 값을 그대로 표현해야 하기 때문에 수정의 여지를 두면 안 된다고 생각했다. 그래서 모든 필드를 final로 두고 생성자로 초기화한 후 getter로 읽기만 가능하도록 아래처럼 변경했다.

@Getter
public class MemberDTO {
    @JsonProperty("id")
    private final long id;

    @JsonProperty("username")
    private final String username;
    ...
}

이런 식으로 간단하게 작성하고 LoginRequest 같은 클래스는 원래 목적인 별도의 바인딩 클래스로 취급해서 컨트롤러 쪽으로 옮겨두는 쪽으로 정리할 수 있었다. 바인딩 클래스의 경우도 @NotNull 같은 validation 어노테이션의 message 필드를 그렇게 많이 활용할 필요가 없다고 하는데 왜냐면 이런 오류 응답에 대한 렌더링은 프론트엔드 쪽에 위임해도 괜찮다는 조언을 들었기 때문이다.

SimpleBBS 같은 전통적인 MVC 방식에서는 조금 중요했겠지만 API 방식에서는 너무 백엔드에서 모든걸 다 하려고 하지 않고 프론트엔드에 조금 위임해도 괜찮을 것 같다.

확실히 느낀 것은 프론트엔드 개발을 진행하지 않고 API만 디자인하다보면 어떤 환경에서 어떤 식으로 API 스펙이 요구되는지를 가늠할 수가 없다. 곧 SimpleBBS 프로젝트에도 Vue.js를 학습해서 적용해보고 싶은데 그 때 좀 많이 고민해봐야 할 것 같다.

반복되는 코드 제거

이런 부분까지 클래스나 메서드를 정의해야 할까? 싶은 부분이 간혹 있었다. 예를 들어 Spring Data JPA의 repository 에서는 findById 같은 메서드로 엔티티를 찾아오면 Optional 객체로 반환하기 때문에 이를 사용하는 서비스 측에서는 아래와 같은 코드가 반복된다.

Member member = memberRepository.findById(request.getId())
                 .orElseThrow(() -> new UserNotFound(request.getId()));

물론 어쩔 수 없는 과정이지만 모든 엔티티 객체를 매번 찾을 때마다 이렇게 직접 orElseThrow 로 예외를 설정하고 객체를 얻어오는 과정이 반복된다고 느껴졌다. 생성자에서 식별자를 받도록 예외 구조를 변경한 이후에도 그 형태는 거의 고정됐기 때문에 이 과정을 단순화해주는 유틸리티 클래스가 있으면 좋겠다고 생각했다. 그래서 다음처럼 필요한 메서드를 제공하는 컴포넌트를 작성해서 활용했다.

public class EntityFinder {
    private final MemberRepository memberRepository;
    private final TeamRepository teamRepository;
    private final TodoRepository todoRepository;
    private final TodoListRepository todoListRepository;

    public Team findTeamById(long teamId) {
        return teamRepository.findById(teamId).orElseThrow(() -> new NoTeamFoundException(teamId));
    }
    ...

적용하기 전에는 굳이 해야하나 고민했지만 막상 적용하니 코딩이 더욱 편해졌다고 느낄 수 있었다. 다른 프로젝트에도 적극적으로 도입해서 활용하는 방안을 고려해봐야겠다.

결론

며칠동안 수많은 수정사항을 적용하고 반영하고 테스트 코드를 업데이트하느라 진이 다 빠져버렸다. 그렇지만 이런 고민과 시행착오를 통해 내 코드가 한 단계 더 높은 품질을 얻었다고 확신할 수 있었다. 이제 좀 더 기능을 보완하고 매력있는 프로젝트로 나아가기 위한 계획을 세워봐야겠다.