이번에 일본어 학습 사이트 만들면서 1달의 기간동안 대략 1만줄의 코드를 작성했습니다. 순수 백엔드만이며, 프론트, 관리자 페이지, 인프라 영역 코드까지 포함하면 더 길어갑니다
제법 코드 구조를 생각하면서 작성했지만 막상 다시 읽어보면 코드가 500줄 넘어가고, 유지 보수 측면에서 조정해야할 필요를 느끼고 리펙토링 과정을 작성해보았습니다.
프로젝트 전체 코드를 수정했지만, 공개는 Auth 부분만 진행합니다.
@Service@RequiredArgsConstructor@Transactionalpublic class AuthService {
private final MemberRepository memberRepository; private final PasswordEncoder passwordEncoder; private final JwtProvider jwtProvider; private final AuthenticationManager authenticationManager; private final UsernameValidator usernameValidator;
private final RedisTemplate<String, String> authRedisTemplate; private final NotificationService notificationService;
private final EmailService emailService; private final LoginHistoryRepository loginHistoryRepository;
@Transactional public TokenResponse signUp(SignUpRequest requestDto, String ipAddress, String userAgent) {
usernameValidator.validate(requestDto.getUsername());
final String encryptedPassword = passwordEncoder.encode(requestDto.getPassword()); final LocalMember newMember = new LocalMember( requestDto.getEmail(), requestDto.getUsername(), encryptedPassword, Role.USER );
(...)
return new TokenResponse(accessToken, refreshToken, userName, refreshTokenValidityMs); }
public TokenResponse login(LoginRequest requestDto, String ipAddress, String userAgent) {
String lockoutKey = LOGIN_FAIL_PREFIX + requestDto.getEmail(); String currentFailCountStr = authRedisTemplate.opsForValue().get(lockoutKey);
if (currentFailCountStr != null) { int failCount = Integer.parseInt(currentFailCountStr); if (failCount >= MAX_LOGIN_ATTEMPTS) { long expireTimeMinutes = authRedisTemplate.getExpire(lockoutKey, TimeUnit.MINUTES); long remainTime = expireTimeMinutes > 0 ? expireTimeMinutes + 1 : LOCKOUT_DURATION_MINUTES;
throw new CustomException(ErrorCode.ACCOUNT_LOCKED, remainTime); } }
(...)
return new TokenResponse(accessToken, refreshToken, userName, refreshTokenValidityMs); }로그인 로직을 짜면 저처럼 복잡한 분도 있고, 애초에 깔끔하게 잘 작성한 분도 있으실텐데 단기간에 빠르게 작성하다보면 의식은 하고 있지만 이게 말처럼 잘 안되더라고요.
리펙토링 시작
- 우선은 뭐부터 해야할까? 생각해보면, 이름 규칙부터 설정해야겠더라고요
네이밍 컨벤션 시작
@Transactional public TokenResponse signUp(SignUpRequest requestDto, String ipAddress, String userAgent) {사실 ‘카멜 케이스’나 폴더는 ‘스네이크 케이스’ 그런 네이밍 적인 요소는 다 통일이 되어 있습니다.
다만, dto의 경우 SignUpRequestDto로 되어있는 파일도 있고, SignResponse로 되어있는 파일도 있습니다. 파일에는 과연 ‘dto’를 붙이는 게 좋냐? 안붙이는 게 좋냐? 는 단순한 생각부터 시작합니다.
사용할 때는 그래도 있는 편이 네이밍에서 헷갈림 없지 않냐?는 생각도 들지만, Request, Response가 있는 시점부터 굳이? 붙일 필요는 없더라고요.
우테코 프리코스에 참여하다보니 폴더명에 이미 포함되어있다면, 이게 무엇인 지 인지할 수 있다면 굳이 필요없다. 라는 말을 듣고 dto를 전부 제거하는 것부터 시작했습니다.
호출할 때도 requestDto가 아닌 request로 변환했습니다.
그러면 Request가 2개인 경우라면? 정말 그런 경우가 있을까 싶은데, 접속한 사용자의 PC정보가 IP, 언어 기반을 받을 때 우연찮게 2개가 겹치는 경우가 있긴 했었습니다.
public ResponseEntity<AccessTokenResponse> signUp( @Valid @RequestBody SignUpRequest request, HttpServletResponse httpServletResponse, HttpServletRequest httpServletRequest ) { ... return authResponseHelper.createTokenResponse(token, response); }그런 경우에는 얘는 내가 만든 거니, httpServletRequest처럼 이름 그대로 유추 가능하도록 이름을 붙이기로 했습니다.
컴포넌트 분리
‘AuthService에는 너무 많은 역할을 부여했다`고 생각합니다.
그런 생각이 든건 ‘로그인 로직을 없애는 일’은 없겠지만, ‘수정하거나 추가’는 있을 수 있는데, 이걸 그대로 쓰면 이후의 코드 구조는 난잡해질 거 같았습니다.
당장 리펙토링 시작 시점에는 OAuthService와 AuthService 그리고 AdminAuthService 정도로 분리되었고 따로 component는 없는 상태입니다. 그래서 분리가 중요한 시점이더라고요.
우선 분리하는 기준은 어떻게 잡는 게 좋을까?
- 기능
- 역할
- 규칙
가볍게 생각하니 딱 3가지 정도로 떠오르네요.
줄이면 얘가 어떤걸 하는 놈인가?입니다.
Email을 담당하는지? SSE를 보내는 지? OAuth에서만 쓰는지? 토큰을 발송하는 지? 뭐 그런 요소로 componet를 나눠보았습니다.
- LoginHistoryRecorder
- LoginLockManager
- OAuth2Client
- OAuthMemberManager
- PasswordResetManager
- TokenManager
우선 이렇게 나눠봤는데, 분리하는 기준은 우테코에서 정말 좋아하는 ‘15줄 이하, indent 2 이하, else 금지’ 정도만 지켜서 분리했는데 생각보다 나쁘지 않더라고요.
/** * 로그인 시도 전, 계정이 잠겨있는 지 확인 * @param email Email */ public void validateNotLocked(String email) { String key = getKey(email); String currentFailCountStr = authRedisTemplate.opsForValue().get(key);
if (currentFailCountStr != null && Integer.parseInt(currentFailCountStr) >= MAX_LOGIN_ATTEMPTS) { long expireTime = authRedisTemplate.getExpire(key, TimeUnit.MINUTES); long remainTime = expireTime > 0 ? expireTime + 1 : LOCKOUT_DURATION_MINUTES;
throw new CustomException(ErrorCode.ACCOUNT_LOCKED, remainTime); } }예로 해당 코드는 로그인 계정이 잠겨있는 지 확인하는 코드입니다. 기존에는 Service에 각각 담겨서, 코드 중복이 3번이나 있었던 것을 공통 로직으로 줄여서 유지 비용을 줄였습니다.
값들은 매번 코드에서 찾아서 넣기보다, 상수처리했습니다.
- 분리 결과
public TokenResponse login(LoginRequest request, String ipAddress, String userAgent) { loginLockManager.validateNotLocked(request.getEmail());
Member member = authenticateUser(request);
loginLockManager.validateMemberStatus(member);
loginLockManager.resetFailureCount(request.getEmail()); loginHistoryRecorder.save(member.getId(), ipAddress, userAgent);
return tokenManager.issueTokens(member); }기존 로그인 로직의 길이가 100줄에서 15줄 미만으로 줄었다는 점이 개발자 입장에선 정말 칭찬할만하더군요.
뭐가 좋아졌냐? 묻는다면 제가 추후에 뭐 수정해야할 때 그냥 스트레스가 적어질 거 같더라고요. 이런 형태로 싹다 바꾸면 단위 테스트 작성하는 것도 복잡성이 줄어서 깔끔하게 되는 거 같습니다.
주석
분리를 하고보니 고칠 걸 바로 찾고 싶은데 과연 바로 찾아질까? 하는 의문이 생겼습니다.
- 예로 당장은 괜찮지만 추후에 무언가 추가하면 ‘여기’도 진행해야한다.
- 이 곳은 무언가 빠졌는데 당장 우선순위는 낮지만 꼭 해야한다.
이런 게 조금 있더라고요. 보통 바로 바로 처리할 수 없고 다른 로직이 개발되어야 할 수 있는 부분이 있어서 머리로만 기억하는 건 안그래도 머리 안좋은데 어떻게 합니까..
/** * 시험 시작 * TODO: 여긴 왜 DTO안쓰고 그냥 값들 가져오고 있지? */인텔리제이는 TODO로 주석을 걸 수 있는데 이걸 모아서 볼 수 있습니다. 이거 말고 또 다른 방식도 있었는데 보통 TODO를 많이 쓴다고 하더라고요
그래서 리펙토링 과정에서 ‘추가’ 또는 ‘수정’이 필요한 건 주석으로 일단 표기하고 이후에 이어서 작업하고 있습니다.
Transactional은 만능이 아님
사실 처음에는 Service에 무작정 Transactional을 달았습니다. 근데 그게 만능은 아니라고하네요.
무결성, 일관성을 보장해야한다면 롤백 과정때문에 필요하지만, 그렇지 않다면 오히려 DB 연결을 물고 있어서 성능 저하가 발생할 수 있다고합니다.
사실 이건 테스트를 하고 싶어도 부하 테스트로는 N100으로는 CPU가 먼저 한계점에 도달해버리기에.. 차차 생각해보려고 하는데 일단 Auth 영역은 ‘읽기 작업’만 필요한 경우 Transactional(readOnly = true)로 조회용으로 선언하고 메모리를 절약하도록 했습니다.
완료
사실 이 정도면 리펙토링 적절하게 진행하지 않았나 싶습니다.
성능 개선도 이후에 할 거 생각하면 할 게 많네요. 부지런히 해야할 거 같습니다