잘못된 부분들 하나씩 추적하기

 

아샬님으로부터 프로젝트에 존재하는 소스코드 중 게임에 참가를 신청하는 소스코드의 많은 부분을 리뷰받았다. 디스코드에 올려주신 리뷰와 Pull Request로 보내주신 리뷰를 하나씩 추적하면서 고쳐야 할 점들을 정리했다.

 

에러 메시지를 이용해 어떤 처리를 하지 않는다.

// Anti-pattern

@ExceptionHandler(RegisterGameFailed.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public RegisterGameFailedErrorDto registerGameFailed(RegisterGameFailed exception) {
    Integer errorCode = setCodeFromMessage(exception.getMessage());
    String errorMessage = errorCode.equals(DEFAULT)
            ? "알 수 없는 에러입니다."
            : exception.getMessage();
    return new RegisterGameFailedErrorDto(
            errorCode,
            errorMessage,
            exception.getGameId()
    );
}

 

한 종류의 에러에 메시지를 넣는 게 아니라, 적절한 예외를 만든다.

훨씬 쉽게 에러 핸들링을 할 수 있다.

@ExceptionHandler(GameNotFound.class)
@ResponseStatus(HttpStatus.NOT_FOUND)
public String gameNotFound() {
    return "Game not found";
}

@ExceptionHandler(AlreadyGameJoined.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public String alreadyGameJoined() {
    return "...";
}

@ExceptionHandler(GameIsFull.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public String gameIsFull() {
    return "...";
}

@ExceptionHandler(UserNotFound.class)
@ResponseStatus(HttpStatus.BAD_REQUEST)
public String userNotFound() {
    return "...";
}

 

에러 코드는 사용하지 않고, 예외처리의 검증은 적절한 Response Header를 반환하는지 정도로 확인한다.

mockMvc.perform(MockMvcRequestBuilders.post("/registers/games/" + gameId)
                        .header("Authorization", "Bearer " + token))
                    .andExpect(MockMvcResultMatchers.status().isNotFound())
    //              .andExpect(MockMvcResultMatchers.content().string(
    //                        containsString("100")
    //              ))
;

 

도메인 모델이 처리해야 하는 로직을 Application Layer 서비스가 처리하지 않는다.

도메인 로직이 처리하는 로직의 예시: game.join(currentUser, registers)

@Entity
@Table(name = "games")
public class Game {
    @Id
    @GeneratedValue
    private Long id;
    
    // ...
    //
    
    public Register join(User currentUser, List<Register> registers) {
        this.registers = registers;

        if (alreadyJoined(currentUser)) {
            throw new AlreadyGameJoined(id);
        }

        if (isFull()) {
            throw new GameIsFull(id);
        }

        Register register = new Register(currentUser, this);

        return null;
    }
    
    private boolean alreadyJoined(User user) {
        return registers.stream()
                .filter(Register::active)
                .anyMatch(register -> register.match(user));
    }

    private boolean isFull() {
        long count = registers.stream().filter(Register::accepted).count();
        return targetMemberCount.reach(count);
    }
}

@Embeddable
public class GameTargetMemberCount {
    @Column(name = "target_member_count")
    private Integer value;
    
    // ...
    //
    
    public boolean reach(long value) {
        return value >= this.value;
    }
}

• Game 객체가 join이라는 동작을 수행해 Register 객체를 생성한다.
  
  
• Entity 도메인 모델이 어떤 값이나 List를 받아 동작을 처리해야 할 때, 주어지는 값을 받아 갖고 있으면서 동작에는 사용해야 하지만 DB에는 들어가면 안 되는 필드에는 @Transient 어노테이션을 이용한다.
  
  
• 이미 register가 존재하는지 판별하는 로직은 alreadyJoined(currentUser) 메서드에서 registers 컬렉션이 포함하고 있는 개별 Register들에게 핵심 로직을 위임하는 식으로 이루어진다.
  
  
• 게임에 참가 정원이 가득 찼는지 판별하는 로직은 isFull() 메서드에서 GameTargetMemberCount 객체에게 핵심 로직을 위임하는 식으로 이루어진다: targetMemberCount.reach(count)로 참가자 수와 목표 정원을 비교
  
  
• cf. GameTargetMemberCount 객체에게 reach 메서드로 역할을 부여함으로써 값 객체일 수 있는 의미를 찾았지만, 그렇지 않았을 때에는 getter가 필요한 이유를 고민해야 한다.
  
  

이런 식으로 Service 로직을 작성할 경우, find로 필요한 데이터들을 모으는 것과 핵심 도메인 로직을 완전히 분리할 수 있다. 이렇게 할 경우 기존의 지나치게 비대한 stub을 생성해야 했던 Service의 테스트도 깔끔하게 수행할 수 있다.

 

예시 2: register

@Entity
@Table(name = "registers")
public class Register {
    @Id
    @GeneratedValue
    private Long id;
    
    private Long userId;
    
    private Long gameId;
    
    @Embedded
    private RegisterStatus status;
    
    // ...
    //
    
    public boolean match(User user) {
        return user != null && userId.equals(user.id());
    }

    public boolean active() {
        return status.equals(RegisterStatus.processing())
                || status.equals(RegisterStatus.accepted());
    }

    public boolean accepted() {
        return status.equals(RegisterStatus.accepted());
    }

    public boolean processing() {
        return status.equals(RegisterStatus.processing());
    }
}

Register 객체가 match, active, accepted, processing 동작을 통해 자신의 상태를 파악하게 한다.

 

Entity의 생성자에 다른 객체의 id가 필요한 경우 @EmbeddedId를 이용하거나, 다른 객체를 직접 주입하고 생성자 내에서는 주입된 객체의 id를 꺼내 사용하는 식으로 타입을 드러낸다.

@Entity
@Table(name = "registers")
public class Register {
    @Id
    @GeneratedValue
    private Long id;
    
    private Long userId;
    
    private Long gameId;
    
    @Embedded
    private RegisterStatus status;
    
    private Register() {

    }

    // public Register(Long id,
    //                 Long userId) {
    public Register(User user, Game game) {
        this.userId = user.id();
        this.gameId = game.id();
        this.status = RegisterStatus.processing();
    }
    
    // ...
    //
}

 

값 객체 자체는 immutable하게 사용한다.

@Entity
@Table(name = "registers")
public class Register {
    // ...
    //
    
    @Embedded
    private RegisterStatus status;

    // ...
    //

    public RegisterStatus status() {
        return status;
    }

    public void cancelRegister() {
        // status.changeToCanceled();
        status = RegisterStatus.canceled();
    }

    public void acceptRegister() {
        // status.changeToAccepted();
        status = RegisterStatus.accepted();
    }

    public void rejectRegister() {
        // status.changeToRejected();
        status = RegisterStatus.rejected();
    }
}

acceptRegister를 할 때, register가 가진 값 객체의 value를 바꾸는 것이 아니라 값을 가진 값 객체 팩토리 메서드를 통해 값 객체를 만들어서 register가 가진 객체를 재할당한다.

 

public static final은 절대로 사용하지 않는다.

@Embeddable
public class RegisterStatus {
    // public static final String PROCESSING = "processing";
    // public static final String CANCELED = "canceled";
    // public static final String ACCEPTED = "accepted";
    // public static final String REJECTED = "rejected";
    private static final RegisterStatus PROCESSING =
            new RegisterStatus("processing");
    private static final RegisterStatus CANCELED =
            new RegisterStatus("canceled");
    private static final RegisterStatus ACCEPTED =
            new RegisterStatus("accepted");
    private static final RegisterStatus REJECTED =
            new RegisterStatus("rejected");

    @Column(name = "status")
    private String value;
    
    private RegisterStatus() {

    }

    private RegisterStatus(String value) {
        this.value = value;
    }
    
    // public String value() {
    //     return value;
    // }

    public static RegisterStatus processing() {
        return PROCESSING;
    }

    // public void changeToCanceled() {
    //     this.value = RegisterStatus.CANCELED;
    // }
    public static RegisterStatus canceled() {
        return CANCELED;
    }

    public static RegisterStatus accepted() {
        return ACCEPTED;
    }

    public static RegisterStatus rejected() {
        return REJECTED;
    }
}

지금 사용하는 방식대로 사용하고 싶을 때는 private static final에서 값 객체에 어떤 값을 부여한 생성자를 받게 하고, 값 객체 메서드가 해당 private static final 값을 사용하도록 한다.

 

값 객체의 toString을 Override할 때, value.toString()을 반환하게 하면 value를 드러내어 사용할 수 있으므로 좋다.

@Override
public String toString() {
    return value.toString();
}

 

Application Layer 서비스의 이름은 기능을 나타내야 한다.

PostRegisterGameService >> JoinGameService

 

 

객체의 네이밍은 객체의 타입을 기반으로 한다. 전혀 다른 이름을 사용하지 않는다.

List<Register> membersOfGame
    = registerRepository.findAllByGameId(game.id())
        .stream()
    //   .filter(member -> member.status().value()
    //   .equals(RegisterStatus.ACCEPTED))
        .filter(Register::accepted)
        .toList();

 

하나의 Controller에 너무 많은 Service가 의존성으로 주입되고 있다면 좋지 않은 징후이다.

하나의 Controller에 너무 많은 Service가 물리게 되면, 그 많은 Service를 Mocking하는 과정에서 Controller의 테스트가 너무 비대해지면서 유지보수성이 저해된다.

 

다른 리소스라면 Controller를 나눈다.

• /games/{gameId}/members >> MemberController
  
  
• /games/{gameId}/applicants >> ApplicantController
  
  

 

리소스가 구체적으로 어떤 리소스인지 경로를 통해 의미를 드러낸다.

• /games/{gameId}/members
  
  

 

Code Indentation을 지킨다.

• Tab size: 4
  
  
• Indent: 4
  
  
• Continuation Indent: 8
  
  

 

IntelliJ IDEA가 자동으로 만들어주는 소스코드라 하더라도 Convention에 위배되는 부분은 맞게 수정해준다.

@Override
// public boolean equals(Object o) {
//      if (this == o) return true;
//      if (o == null || getClass() != o.getClass()) return false;
//      GameTargetMemberCount that = (GameTargetMemberCount) o;
// }
public boolean equals(Object other) {
    if (this == other) {
        return true;
    }
    if (other == null || getClass() != other.getClass()) {
        return false;
    }
    GameTargetMemberCount that = (GameTargetMemberCount) other;
    return Objects.equals(value, that.value);
}

 

사용자 계정을 나타내는 명칭으로는 Username을 사용한다.

@Embeddable
public class UserAccount {
    @Column(name = "identifier")
    private String identifier;

    @Column(name = "password")
    private String encodedPassword;

    public UserAccount() {

    }
    
    // ...
    //
}

 

프론트엔드에서 특별한 처리를 해줘야 하는 방식으로 DTO에 값을 담아 전달하지 않는다.

public GameDetailDto findTargetGame(Long accessedUserId,
                                    Long targetPostId) {
    // ...
    //
    
    // Anti-pattern
    Long registerId
        = myRegister == null
            ? -1
            : myRegister.id();
            
    // ...
    //
    
    return new GameDetailDto(
                game.id(),
                game.exercise().name(),
                game.dateTime().joinDateAndTime(),
                game.place().name(),
                members.size(),
                game.targetMemberCount().value(),
                registerId,
                registerStatus
        );
}