Level1 체스 미션 피드백 정리

IllegalArgumentException vs IllegalStateException

@Override
public State proceed(Command command) {
	if (command.isMove()) {
		throw new IllegalArgumentException(CANNOT_MOVE);
	}
	if (command.isStatus()) {
		throw new IllegalStateException(CANNOT_GENERATE_SCORE);
	}
	// ...
}

IllegalArgumentException과 IllegalStateException을 혼용하여 사용하고 있었고 리뷰어가 이를 지적했다. 각 기능을 만들 때 이 두 예외를 언제 써야 하는지 정확한 기준을 잡지 못했던 것이다. 다음 글에서 이 둘의 차이점에 대해서 정리했다.

테스트하고자 하는 객체의 관심사를 명확히 해라 (다른 객체와의 의존 분리)

@Test
@DisplayName("RunningBlackTurn 상태에서 Move 커맨드를 받으면 RunningWhiteTurn 상태가 된다.")
void blackTurn_to_whiteTurn() {
	State state = State.create();
	state = state.proceed(new Start());
	state = state.proceed(new Move(new Position(2, 2), new Position(3, 2)));
	state = state.proceed(new Move(new Position(7, 2), new Position(6, 2)));

	assertThat(state).isInstanceOf(RunningWhiteTurn.class);
}

상태 패턴을 구현한 객체가 알맞은 구현체로 변경되는지 테스트하는 코드이다. 테스트하고자 하는 구현체는 RunningBlackTurn이라는 객체다. 하지만 Ready 객체부터 생성하여 차례차례 상태를 변화시키고 있다.

State.create()을 하고, state.proceed(new Start());를 했을 때 RunningBlackTurn으로 변경되는 건 RunningBlackTurn
의 관심사가 아닙니다. new RunningBlackTurn() 같이 명시적으로 생성해서 테스트하는게 다른 곳에 덜 의존적인 테스트가 될 것 같아요. 

위와 같은 피드백을 받고 다른 구현체를 의존하지 않고 RunningBlackTurn 객체만 테스트하도록 코드를 수정했다.

@Test
@DisplayName("RunningBlackTurn 상태에서 Move 커맨드를 받으면 RunningWhiteTurn 상태가 된다.")
void blackTurn_to_whiteTurn() {
	GameState state = new RunningBlackTurn(board);
	state = state.proceed(new Move(new Position(7, 2), new Position(6, 3)));
	assertThat(state).isInstanceOf(RunningWhiteTurn.class);
}

예외를 던지는 메서드는 예외를 던진다는 것을 예상할 수 있게 이름을 지어라

이는 소제목 그대로의 의미이다. 어떤 것을 검증하는 메서드는 validateXXX()와 같이 예외를 던지는 것을 명시적으로 표현하는 것이 좋다고 한다.

검증 메서드 네이밍

프리픽스로 validate과 check가 섞여서 사용되네요. 각각을 사용하는 기준이 있을까요? 👀

검증을 통해 예외를 던지는 메서드에서 어떤 메서드는 validate를 사용하고 어떤 메서드는 check라고 이름 지어서 사용하고 있었다. 이 질문에 아래와 같이 대답했다.

제가 나름 생각했던 기준은 아래와 같습니다.
validateXXX
 - 어떤 조건을 통과하지 않으면 무조건 지나갈 수 없다. 반환값도 없고 오로지 예외를 발생시키기 위한 메서드
checkXXX 
- 해당 조건의 총족, 미충족에 따라 분기가 나뉘거나 반환 값을 가지는 메서드.

리뷰어는 위 기준도 좋은 기준이지만 코틀린에서 검증 함수 이름을 어떻게 만들었는지 공유해주면서 다른 언어에서는 이에 관해서 어떻게 풀었는지 확인하면 도움이 될 거라고 했다.

• check(value: Boolean) - 인자로 받은 값이 false이면 IllegalStateException을 던진다.
• checkNotNull(value: T?): T - 인자로 받은 값이 null이면 IllegalStateException을 던진다.
• require(value: Boolean) - 인자로 들어온 값이 false이면 IllegalArgumentException을 던진다.
• requireNotNull(value: T?): T - 인자로 들어온 값이 null이면 IllegalArgumentException을 던진다.

이름 짓는건 항상 어려운 것 같다. 이미 만들어진 메서드명이 어떻게 되어있는지 더 관찰해봐야겠다.

find vs get

무언가를 찾거나 얻어올 때 find나 get으로 시작하는 메서드명을 짓는다. 이에 대해 리뷰어가 JPA에서는 find와 get을 어떤 규칙에 따라 명명하는지 링크를 공유해주었다.

 

find는 값을 '찾는다.' 이 경우 값이 없을 수도 있고 이는 자연스러운 상태이다. 주로 반환 값을 Optional로 감싸서 반환하고 값이 있고 없고의 상황을 호출자가 핸들링해야 한다.

public Optional<Piece> findPieceByPosition(Position position) {
	return Optional.ofNullable(pieces.get(position));
}

위 메서드는 체스판의 해당 포지션(위치)에 말 있는지 찾고 있으면 반환하는 메서드이다. 체스 게임 도중 해당 위치에 말이 있을 수도 없을 수도 있기 때문에 find 이름을 사용하고 반환을 Optional로 감싸서 하고 있다.

 

get은 값을 '꺼내온다.' 이 경우 값이 없을 때 예외가 발생한다. 반드시 있어야 하는 값을 꺼내올 때 사용하기 때문에 값이 없다는 건 자연스럽지 않은 상황인 것이다. 호출자가 예외를 반드시 핸들링할 필요는 없다.

public Piece getPieceByPosition(Position from) {
	return findPieceByPosition(from)
		.orElseThrow(() -> new NoSuchElementException(NO_PIECE));
}

반드시 값이 있어야 하는 상황에서는 값이 없는 경우 예외를 던지면서 get 이름을 쓸 수 있다.

URL을 상수로 관리해야 할까?

보통 숫자나 문자열을 하드 코딩하지 않고 상수로 관리하는 것이 좋다고 배워왔다. 그래서 웹을 구현할 때 URL도 상수로 처리했었다. 하지만 이 부분에 대해서 URL을 상수로 관리하는 것의 장단점은 무엇 일지에 대해 고민해보라는 제안을 받았다.

private static final String NEW_GAME_URL = "/new_game";
private static final String START_URL = "/start";
private static final String MOVE_URL = "/move";
private static final String DELETE_URL = "/delete";

우선 하드 코딩을 하지 않는 이유는 해당 값이 어떤 맥락에서, 어떤 의미로 사용되는지 단순한 숫자나 문자열이면 파악하기 힘들기 때문이다.

하지만 하드 코딩된 값을 봐도 무슨 의미인지 명확하다면 (for 반복문에서의 0이라던지) 불필요한 상수 처리는 하지 않았던 적도 있었다.

URL은 하드 코딩되어 있어도 의미가 명확하기 때문에 하지 않아도 될 것 같다는 생각도 들었다.

게다가 URL 값이 "/start"이고 상수 이름이 START_URL인데 극단적으로 말해서 같은 단어를 사용하고 있기에 중복이 아니냐는 의견도 있었는데 듣고 보니 맞는 말 같다.

 

정답은 없겠지만 추후 프로젝트를 진행할 때 팀원들 간에 합의를 통해 상황에 맞는 해답을 찾아야 할 듯하다. 이 문제에 대해선 다음 링크를 참고했다.

예외를 무시하지 마라

public ChessGame updateGame(Command command, String name) {
	ChessGame updatedGame = findGame(name).execute(command);
	try {
		gameRepository.updateGame(updatedGame, command);
	} catch (IllegalStateException ignored) {

	}
	return updatedGame;
}

위와 같은 코드를 작성했었다. 간단히 설명하면 try 블록 안의 코드에서 예외가 발생한다면 해당 로직은 진행하지 않고 넘어가도 됐기에 예외를 잡아주기만 하고 그대로 코드를 진행시켰다.

 

이펙티브 자바 아이템 77을 보면 예외를 무시하지 말라고 한다. 우선 catch 블록을 비워두면 예외가 존재할 이유가 없어진다. 프로그램이 오류를 내재한 채 동작하는 것이기 때문에 오류의 원인과 아무 상관없는 부분에서 프로그램이 죽어버릴 수도 있다.

 

그리고 위에서 내가 사용한 방법은 예외 처리를 한 것이 아니라 try 안에 메서드가 호출될 시점이 맞는지 안 맞는지에 따라 분기하는 로직이라고도 볼 수 있는데 분기를 예외로 처리하고 있는 것이다. 이는 예외를 예외답게 쓰지 않고 제어 흐름용으로 사용하고 있는 것과도 같다. (아이템 69 예외는 진짜 예외 상황에만 사용하라)

 

따라서 위 로직은 아래와 같이 if로 처리하는 것이 훨씬 자연스럽다.

@Override
public void updateGame(ChessGame game, Command command) {
	chessGameDao.updateState(game);
	if (command.isMove()) {
		updatePositionOfPiece(game, command.getFromPosition(), command.getToPosition());
	}
}

 

https://github.com/woowacourse/java-chess/pull/283

https://github.com/woowacourse/java-chess/pull/354