중첩 if문 멈춰 - 리팩토링

요약

인증 단계에서 인증쿠키를 재발급하는 중첩 if문을 개선했다.

before-after

============================================================================================

세션 기반의 인증체계에서 인증쿠키를 재발급해서

쿠키의 유효기간이 끝나도 새로운 인증 쿠키가 재발급되도록 했다.

DB의 사용자 테이블에는 유효기간이 존재하는 세션ID가 있다.

그래서 인터셉터의 preHandle 검증 시에 세션이 만료되었으면

1. DB에서 세션을 조회한다.

2. 서버 세션 다시 활성화 && 쿠키 재발급 && DB에도 새로운 세션 등록

위의 작업을 해야 하는데,

흐름을 글로 적어 보자면 아래와 같다.

1. 세션이 null인가?
	1. 쿠키가 비어있지 않으면
		1. 쿠키를 순회하며 세션ID를 담은 쿠키를 찾는다.
			1. 쿠키를 찾으면 해당 세션ID로 DB를 조회한다.
				1. DB 조회에 실패하였거나 세션ID 기한이 만료되었으면 재발급하지 않는다.
				2. 정상 케이스라면 재발급한다.

코드로 보면 다음과 같다.

@RequiredArgsConstructor
@Component
public class InterInterceptor implements HandlerInterceptor {
    private final PlayerService playerService;
    private final UserSessionService userSessionService;
    @Override
    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler) throws IOException {
        log.info("LoginCheckInterceptor.preHandle");
        HttpSession session = request.getSession(false); // false.로 해야 불필요한 세션생성이 없음

        // 서버에서 세션 속성을 등록하지 않았거나 제거당한(invalidate 로그아웃) 상태면 접근 불가
        if (session==null || session.getAttribute("id") == null){
            Cookie[] cookies = request.getCookies();
            if (cookies != null){
                for (Cookie cookie : cookies) {
                    if ("session_id".equals(cookie.getName())){ // session_id를 찾은 경우
                        String sessionId = cookie.getValue();
                        UserSession userSession = userSessionService.findBySessionId(sessionId);
                        if (userSession == null){
                            response.sendRedirect("/");
                            return false;
                        }
                        if (userSession.getExpirationDate().isBefore(LocalDateTime.now())){ // 쿠키 만료시간도 지났으면 다시 로그인
                            response.sendRedirect("/");
                            return false;
                        }
                        // 쿠키 만료시간이 지나지 않았으면
                        Player player = playerService.findByUserSession(userSession);
                        Long id = player.getId();
                        String name = player.getName();

                        // Set-Cookie session_id 작업.
                        HttpSession newSession = request.getSession();
                        newSession.setAttribute("id", id);
                        newSession.setAttribute("name", name);

                        userSession.setSessionId(newSession.getId()); // 세션ID 값 업데이트
                        return true;
                    }
                }
            }
            response.sendRedirect("/");
            return false;
        }
        return true;
    }

if문의 존재 자체는 어쩔 수 없다.

문제를 꼽자면

1. if문이 여러 번 중첩되어 있어서 읽기 힘들다.

2. 실패와 성공(return true || false)이 메서드 내부 곳곳에 산재해 있기 때문에

메서드의 흐름을 이해하려면 내부 코드 전체를 이해해야 한다.

강의를 듣다가 찾은 예시 코드들도 존재하고,

짧고 잘 정리된 유튜브 영상도 있어서 리팩토링에 참고했다.

https://youtu.be/PAR60O5aXTA

코딩애플 아저씨 영상

여러 번 중첩되고 일관성 없는 if문을 고치고 싶을 때 따를 규칙

1. 실패 케이스를 맨 위로 올린다

실패 케이스를 맨 위로 올리고, 마지막에서 정상처리하는 일관성을 가질 경우 코드를 분석하기 쉬워진다.

2. 검증 로직을 메서드로 분리하고, 검증이 실패할 경우 false와 같은 값을 반환한다.

if문이 가득한 로직보다 boolean을 반환하는 메서드가 존재하는 것이 읽기 좋은 코드를 만든다.

수정한 코드는 다음과 같다.

@Slf4j
@RequiredArgsConstructor
@Component
public class LoginCheckInterceptor implements HandlerInterceptor {
    private final UserSessionService userSessionService;
    private final PlayerService playerService;

    @Override
    public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object handler){
        log.info("LoginCheckInterceptor.preHandle");
        HttpSession session = request.getSession(false); // false.로 해야 불필요한 세션생성이 없음

        // 세션 유효하면 바로 통과
        if (session != null){
            return true;
        }

        /** 아래에서는 세션이 없는 경우 재발급 가능 여부를 체크한다.*/
        // 1. 쿠키 자체가 없는지 체크
        Cookie[] cookies = request.getCookies();
        if (cookies == null) {
            return redirectToSignIn(request, response);
        }

        // 2. 쿠키는 있는데 session_id 쿠키가 없는지 체크
        String sessionId = findSessionId(cookies);
        if (sessionId == null){
            return redirectToSignIn(request, response);
        }

        // 3. 재발급 기한이 지난 세션ID인지 체크
        boolean isSuccess = reassure(request, response, sessionId);
        if (! isSuccess) {
            return redirectToSignIn(request, response);
        }
        // 4. 재발급 성공시 통과
        return true;
    }

    private boolean reissue(HttpServletRequest request, HttpServletResponse response, String sessionId) {
        UserSession userSession = userSessionService.findBySessionId(sessionId);
        if (userSession.getExpirationDate().isBefore(LocalDateTime.now())){ // 쿠키 만료시간도 지났으면 다시 로그인
            return false;
        }

        Player player = playerService.findByUserSession(userSession);
        Long id = player.getId();
        String name = player.getName();

        // Set-Cookie: session_id 작업.
        HttpSession newSession = request.getSession();
        newSession.setAttribute("id", id);
        newSession.setAttribute("name", name);

        userSession.setSessionId(newSession.getId()); // 세션ID 값 업데이트
        return true;
    }


    private String findSessionId(Cookie[] cookies) {
        for (Cookie cookie : cookies) { // 세션은 만료되었지만 쿠키는 있다면, 쿠키 중에 session_id 쿠키가 있는지 확인
            if ("session_id".equals(cookie.getName())) { // session_id를 찾은 경우
                return cookie.getValue();
            }
        }
        return null;
    }

    private boolean redirectToSignIn(HttpServletRequest request, HttpServletResponse response) {
        log.info("미인증 사용자 요청");
        try {
            response.sendRedirect("/login?redirectUrl="+ request.getRequestURI());
            return false;
        } catch (IOException e) {
            log.error("로그인 페이지로 리다이렉트 실패"+e.getMessage());
            throw new RedirectException(ErrorCode.REDIRECT_IOEXCEPTION);
        }
    }
 }

코드의 라인 수는 길어졌지만, 흐름을 파악하기 훨씬 쉬워졌다.

내가 집중해야 할 것은 인터셉터의 public method인 preHandle()메서드를 읽는 사람이

로직의 흐름을 이해하기 쉽게 하는 것이다.

public method의 큰 그림을 이해한다면 private method에서의 상세 로직 이해도 쉬울 것이다.

public method의 흐름 이해를 위해 3가지를 고려했다.

적절한 주석과 메서드 이름
중첩되지 않은 if문
성공 케이스(true 반환) 맨 아래에 배치
(하지만 세션이 유효할 경우 더 진행하는 것이 의미가 없으며, '세션이 존재함' 시나리오가
케이스의 대부분을 차지하므로
세션이 존재할 경우 true를 반환하는 로직을 메서드 시작부분에 삽입했다.)

물론 상세 로직을 이해하려면 메서드 모두를 읽어봐야 하겠지만,

흐름이 이해되었다면 상세 로직을 이해하는 것 또한 어렵지 않을 것이다.

특히 인터셉터와 같이 어떤 검증 로직이 주를 이루는 경우는 적절한 주석과 함께

검증로직을 여러 개의 메서드로 묶는 것이 코드 가독성에 큰 도움이 되는 것 같다.

읽어주셔서 감사감사합니다.

고민되는 점

위와는 조금은 다른 예시일 수 있으나,

최근 Service 계층에서 코드를 작성하다가

예외상황 시 throw Ex를

Service Layer에서 할지,

도메인 객체 내부에서 할지에 대해서 고민했던 적이 있다.

다시 말해 도메인 객체의 검증용 public method를 설계할 때

1. void 검증 메서드 내부에서 검증이 실패하면 throw Ex

2. 검증 메서드 내부에서 검증이 실패하면 false 반환 후 Service Layer에서 throw Ex

둘 중에 한 가지 방법을 선택하고 싶었던 것.

아래 기능은 질문답변 웹 서비스의 '채택' 기능의 일부다.

해당 기능 내부에서 구현할 기능은

1. 게시글 상태(답변완료여부) 변경

2. 답변 상태(채택여부) 변경

3. 답변자 포인트 증가

이다.

Service 계층의 메서드는 다음과 같다.

@Transactional
public void acceptAnswer(Long questionId, Long answerId, String email) {
    /** 아래 작업들은 하나의 트랜잭션 안에서 실행되므로 예외시 롤백*/
    // 1.토큰 소유자와 질문글의 작성자가 일치하는지 확인.
    Question question = findByIdFetchOwner(questionId);
    question.checkOwner(email);

    // 2. 질문글 QuestionState Done으로 전환(내부에서 이미 Done인지 확인)
    question.setStateDone();

    // 3. 답변글 Status Accepted로 전환(내부에서 이미 Accepted인지 확인)
    Answer answer = answerService.findByIdFetchOwner(answerId);
    answer.setStatusAccepted();

    // 4. 답변글의 답변자 포인트 증가
    Student answerOwnerStudent = answer.getOwnerStudent();
    answerOwnerStudent.getPointStatus().increasePoint(PointAmount.ANSWER_ACCEPTED);
}

위 코드에서 사용된 domain 객체들의 메서드 대부분은 void 메서드이며, 내부적으로 예외처리를 한다.

1번 과정인 checkOwner의 예시

public void checkOwner(String email) {
    String writerEmail = ownerStudent.getEmail();
    if (! writerEmail.equals(email)) {
        log.error("토큰 소유자와 질문글의 작성자가 일치하지 않아 채택 불가");
        throw new BusinessException(ErrorCode.NOT_OWNER_OF_QUESTION);
    }
}

2번 과정인 setStateDone의 예시

/** 이미 Done 상태인 질문을 In_Progress 로 만들 일이 없으므로
 *   setStatus(QuestionState questionState) 으로 만들지 않는다.*/
public void setStateDone() {
    if (this.questionState == QuestionState.DONE) {
        throw new BusinessException(ErrorCode.QUESTION_ALREADY_DONE);
    }
    this.questionState = QuestionState.DONE;
}

함수를 작성할 때, 가능하면 반환 타입을 적는 게 좋다는 이야기를 들었다.

파라미터에 무언가를 넣으면 반환값이 나오는 것이 함수의 정의에 알맞고,

적절한 반환값과 함수 이름의 조화는 내부를 확인하지 않고 함수 호출부만 봐도 코드의 흐름을 이해할 수 있게 해 준다.

그런데 위의 예시에서는 대부분 void 메서드를 사용했다.

이유는 다음과 같다.

단순 검증이 아닌, 객체 내부의 상태를 변경하는 로직이 함께 들어있기 때문
로깅과 예외 전환(throw)과 같은 핵심적이지 않은 코드들을 service layer에서 작성하면
service 계층의 코드만 보고 흐름을 이해하기 힘들어질 것이라 생각함.

인터셉터에서의 예시처럼 위의 메서드들의 반환타입을 boolean으로 하고 호출부에서 값을 받는 형식으로 변경할지를

고민했으나, 위의 2가지 예시 때문에 일단은 현 상태(void 메서드 내부에서 예외처리)를 유지하기로 했다.

24.02.28 추가

글 작성 후 8개월이 지났다.

8개월 후의 내가 던지는 추가 의견

지금의 내가 보기에도 의견이 크~게 달라지진 않았지만

내린 결정에 대한 근거는 좀 더 풍부하게 댈 수 있게 되었다.

간단하게 말하자면

'권한 체크' 는 boolean을 return하게 하고

'답변 상태 변경'은 유지해도 괜찮을 것 같다.

추가 근거는 다음과 같다.

1. 이미 명백하고 중요한 오류 상황인지를 생각해보면 된다.

이미 채택완료된 질문을 완료처리하거나

데이터 변경자가 데이터에 대한 접근권한이 없는 경우는

서비스 내에서 명백한 예외상황이며, 더 이상 로직을 진행할 필요가 없다.

그리고

'명백한 예외상황에 대해 throw Ex를 하고 흐름을 종료한다' 는 결정은

애플리케이션의 도메인 로직상 매우 중요한 '결정'이자 '제약' 이며

이러한 제약은

'전체적인 하나의 use case를 담고 외부 서비스와의 연동 등을 도와주는'

Service Layer가 아니라

'도메인 로직상의 정책을 정의하는'

도메인 객체가 들고 있는 것이 옳다고 판단했다.

하지만 권한 체크의 경우에는 '2번'의 이유로 boolean return 후

service Layer에서 throw Ex를 하는 것이 좋아 보인다.

2. 상황마다 다른 예외를 던지거나 에러 메시지를 바꿔야 할지를 생각하면 된다.

-> 추가 설명을 하자면

내가 1번에서 '권한 체크' 메서드를

'질문 주인 체크' 메서드는 다른 맥락(질문 수정 삭제 등)에서도 호출될 가능성이 있어서

이 친구는 좀 더 고쳐봐야 할 것 같다.

특히 이 로그가 마음에 들지 않는다.

log.error("토큰 소유자와 질문글의 작성자가 일치하지 않아 채택 불가");

로그 메시지- 메서드 이름- 예외정보가 일치하지도 않고

어차피 전역 예외처리 단계에서 예외에 대한 로그가 찍히기 때문에

로깅이 정말 필요한지도 좀 의문이다.

이 친구는 단순히 boolean값을 return하게 하고

각각의 case(Service Layer)에서

상황에 따른 예외 메시지를 던지게 하는 게 좋을 것 같다.

('~가 일치하지 않아 수정 불가' 이런 식으로.)

'답변 상태 채택으로 변경' 메서드는 그대로 둬도 될 것 같다. 다른 사용 케이스가 없을 듯.

3. 검증 메서드 호출자가 메서드 이름(validate, check, verify 등) 을 보고 내부적으로 예외가 던져질 것을 예측 가능하다.

-> 지금 보니까 'check' 라는 메서드 이름은 boolean 혹은 다른 정보를 return해서

체크한 정보를 가지고 다른 로직을 진행하는 케이스에 알맞을 것 같다.

검증 후 예외를 던져버리는 메서드는 'verify' 'validate' 이 쪽이 좀 더 어울릴...텐데

2번에서 '메서드 내부에서 예외를 던지지 않기'로 결정했으니

'boolean'을 반환하는 'checkOwner' 메서드로 변경하면 될 것 같다.

마지막으로, 이건 코딩 스타일 문제는 아닌데

'답변자 포인트 증가'는

여러 스레드에서 동시에 처리할 경우 첫 번째 트랜잭션의 변경사항이 무시될 수 있을 것 같다.

회원 데이터를 읽어올 때 Lock이 필요할 것으로 생각된다.

'비난 스터디' 카테고리의 다른 글

[DB]인덱스에 대한 이해- 2부(다중 컬럼 인덱스, 커버링 인덱스, 선행 와일드카드) (0)	2023.08.07
[DB]인덱스에 대한 이해- 1부(기본적인 원리와 실습) (0)	2023.07.27
[Spring]직렬화를 하는 이유 - Serializable과 JSON 직렬화 (0)	2023.07.20
Java - equals와 hashCode란? 왜 둘을 함께 재정의해야 할까? (0)	2023.07.05

앉아줘요

중첩 if문 멈춰 - 리팩토링

읽어주셔서 감사감사합니다.

고민되는 점

24.02.28 추가

'비난 스터디' 카테고리의 다른 글

티스토리툴바

중첩 if문 멈춰 - 리팩토링

읽어주셔서 감사감사합니다.

고민되는 점

24.02.28 추가

'비난 스터디' 카테고리의 다른 글

관련글

티스토리툴바