[Java] AI한테 코드리뷰를 받아보자… (1)

ChatAI

바야흐로 AI의 시대, AI가 다 해먹는 세상이 점점 와감에 따라, 얼마전에는 그림을 대신 그려주는 AI가 굉장히 화제가 되면서 그림업계에 큰 파장을 불러 일으켰다.

나도 코딩할때 코파일럿을 약간 대신 코딩해주는 조수 느낌으로 잘 사용하고 있었는데, 이번에는 OpenAI에서 성능이 뛰어난 자연어처리 AI가 나왔다고 해서 한번 사용해보려고 한다.

[링크] OpenAI - ChatGPT

특히 코드관련해서는 정말 사람이랑 비슷하거나 그 이상(?)의 실력을 보여준다고 해서, 항상 코드를 작성하면서 이게 맞는지 긴가민가 했던 부분들이 있었는데 이번 기회에 인간의 자존심을 조금 버리고 인공지능에게 코드리뷰를 좀 받고 내 코드를 리팩터링 해보는 시간을 가져보려고 한다.

사실 면접때마다 깃허브를 들여다보시는 채용담당자 분들이 많으셔서, 언젠가 리팩터링을 해야지 해야지 하고 있었는데, 이번 기회에 싹 한번 더 나은 코드로 바꿔보려고 한다.

너무 인공지능이 하는 말을 신뢰하면 또 안되니까, 하나하나 왜 인공지능이 그 방식이 더 낫다고 생각했는지를 찾아보고 나도 스스로 고민해보며 리팩터링 해볼 생각이다.

이 게시물에서 리팩터링 하는 코드가 궁금하다면 아래 링크에서 접속 가능하다.

[링크] 해당 코드 레포

오늘은 가장 핵심 비즈니스 로직이 담겨있는 Service단을 리팩터링 해보도록 하겠다.

ChatAI에게 물어보기

먼저 ChatAI에게 해당 코드를 리뷰해달라고 부탁했고, 아래와 같은 답변을 받았다.

1. Use the @AllArgsConstructor annotation instead of @RequiredArgsConstructor. This will generate a constructor that includes all fields, including those marked as @NonNull. This will make the code more readable and maintainable.

2. Use a Pageable object instead of using from and to parameters to paginate the results. This will make the code more readable and maintainable, and will allow for more flexibility in pagination options.

3. Use the @Log4j2 annotation instead of @Slf4j to import the log4j2 logger, as it is more efficient and has better performance.

4. Use the @JsonInclude annotation on the ApplyListDto, FavoriteDto, and CommentDto classes to only include non-null fields in the JSON response, to reduce the size of the response and improve performance.

5. The findAll method is annotated with @Transactional, which indicates that the method should be executed within a transaction. However, the method does not perform any database updates or deletions, so a transaction is not actually needed. Removing the @Transactional annotation would make the code more efficient.

10개정도의 리팩터링 추천 사항이 있었지만, 이번 게시물에서는 5가지의 사항에 대해서 왜 AI가 이것을 추천하는지 찾아보고, 이유가 합당하다면 리팩터링을 진행해보려고 한다.

@RequiredArgsConsructor 대신에 @AllArgsConstructor를 사용하세요

1. Use the @AllArgsConstructor annotation instead of @RequiredArgsConstructor. This will generate a constructor that includes all fields, including those marked as @NonNull. This will make the code more readable and maintainable.

AllArgsConstructor 어노테이션을 사용하면 @NonNull 어노테이션을 가진 필드를 포함한 모든 필드를 포함하는 생성자를 생성해주기 때문에, 더 읽기 쉽고 유지보수가 잘되게 할 수 있습니다.

둘다 롬복의 어노테이션으로, AllArgsConstructor의 경우는 존재하는 모든 필드가 포함된 생성자를 만들어주고, RequiredArgsConstructor의 경우는 final로 선언된 필드만을 포함한 생성자를 만들어준다.

나는 RequiredArgsConstructor를 사용하는게 더 적절하다고 생각해서 한번 더 물어봤는데, 그래도 AI는 AllArgsConstructor를 사용하는게 더 좋다고 하며 예시까지 주었다.

// Instantiate with only the required dependency
MyClass myClass = new MyClass(requiredDependency);

// Instantiate with both the required and optional dependencies
MyClass myClass = new MyClass(requiredDependency, optionalDependency);

// Instantiate with the required dependency and set the optional dependency to null
MyClass myClass = new MyClass(requiredDependency, null);

위 코드처럼 AllArgsConstructor를 사용하면 필수 의존성과 선택적 종속성을 여러가지 방법으로 인스턴스화 할 수 있다는 장점이 있다고 했다.

하지만 나의 경우는 final로 선언해둔 의존성 주입이 필요한 필드만이 스프링 컨테이너로부터 주입을 받는 생성자를 만드는게 목적이기 때문에 오히려 AllArgsConstructor로 모든 필드에 접근 할 수 있는 생성자를 만드는것이 더 안좋은 방법이라고 생각했다.

실제로 여러 검색을 통해서 알아본 결과 나와 같은 이유로 다들 @RequiredArgsConstructor를 사용하는게 맞다는게 정론이었다.

그래서 이 부분은 패스

from과 to 매개변수를 사용하는 대신 Pageable을 사용해보세요

기존 방식은 내가 임의로 api 요청자로부터 from과 to를 받아 이를 기반으로 페이지네이션하는 로직을 일일히 직접 구현하였다.

하지만 JPA에서는 Pageable이라는 기능을 지원하는데, 이를 사용하면 페이지네이션을 간편하고 가독성있게 사용할 수 있다고 한다.

사실 이 부분은 얼마전에 면접에서 질문을 받았던 부분의 코드인데, 가독성이 떨어져 내 코드임에도 제대로 대답하지 못했던 기억이 난다.

진작 이것을 알고 리팩터링했더라면 더 잘 대답할 수 있었을 텐데…

from과 to를 사용하여 페이지네이션 하는 부분을 JPA의 Pageable을 활용하여 더 가독성있고 효율적으로 리팩터링 하였다.

페이징 부분에 코드가 꽤나 많아서 엄청나게 많은코드를 삭제할 수 있었고, 훨씬 가독성 있는 코드가 되었다.

[링크] 리팩터링 완료한 커밋

slf4j 대신에 log4fj2를 사용하면 더 효율적이고 나은 성능을 얻을 수 있습니다

이 부분에 대해서는 면접때 질문을 받은 적이 있어서 찾아본적이 있었다.

정확히 질문은 slf4j를 사용한 이유가 무엇인가요? 였는데, 사실 그냥 다 로깅 이렇게 하길래 아무 생각없이 따라했던거여서 대답을 못했었다.

그래서 면접이 끝나고 왜 slf4j를 보통 많이 사용하는지를 찾아봤는데, 바로 slf4j는 인터페이스이기 때문이었다.

로깅 라이브러리에는 log4j나 자바에서 지원하는 java.util.logging, logback 등의 라이브러리가 있는데, slf4j는 라이브러리가 아닌 추상화된 인터페이스이기 때문에 설정에 따라 어떠한 로깅 라이브러리로도 교체가 가능하다.

이러한 의존성이 적다는 이점때문에 slf4j를 주로 사용하는것이 그 이유였다.

그래서 이미 알고 있는 내용이라 다시 AI에게 왜 log4j의 사용을 추천하는지 물어봤는데 그 이유는 다음과 같았다.

slf4j의 경우는 컴파일 시점에 인터페이스를 생성하는 과정을 한번 거치기 때문에 log4j2를 사용하는 것이 속도가 더 빠르다는 것

하지만 나의 경우는 속도가 중요한 프로젝트는 아니기 때문에, 속도가 빠른 log4j 보다는 의존성이 적고 유연한 slf4j를 사용하기로 했다.

JsonInclude 어노테이션을 사용하여 null이 아닌 필드만 포함하여 성능을 향상시키세요

@JsonInclude 어노테이션은 사용해본 적이 없어서, 검색을 통해 찾아봤다.

@JsonInclude는 보통 ResponseDto에서 사용하는 어노테이션으로 json으로 데이터를 직렬화할때 값들에 대해 옵션을 줄 수 있는 어노테이션이다.

사용가능한 옵션은 다음과 같다.

Always

모든 값을 출력한다

NON_NULL

null 값인 값은 제외하고 출력한다.

NON_ABSENT

null 값인 값은 제외하고,
변수중에 AtomicReference 타입으로 된 변수가 있다면, 값이 있는지 확인하고 null이라면 마찬가지로 제외한다.

여기서 AtomicReference는 동시성을 보장하기 위한 wrapper 클래스이다. 자세한 설명은 아래의 링크를 참조하면 된다.

[링크] AtomicReference 사용방법

NON_EMPTY

1. null 제외
2. absent도 제외 (NON_ABSENT와 같은 역할)
3. Collection, Map일 때 isEmpty()가 true여도 제외
4. Array는 length가 0이면 제외
5. String도 length가 0이면 제외

NON_DEFAULT

empty를 제외 (NON_EMPTY와 같은 역할)
primitive 타입이 default면 제외 (ex. int/Integer: 0, boolean/Boolean: false)

위와같이 JsonInclude를 사용하여 옵션을 주면, 불필요한 데이터를 제거할 수 있다.

그럼 RespnseDto에 JsonInclude를 주어 Null인 데이터는 굳이 불러오지 않도록 하여 성능을 향상시켜보도록 하자.

따로 동시성에 대한 이슈는 없으니 NON_NULL 정도의 옵션만 주면 될 것 같다.

[링크] 리팩터링 완료한 커밋

findAll 메서드는 트랜잭션이 필요하지 않으므로 @Transactional 어노테이션을 제거해주세요

이 부분도 사실 면접관한테 질문을 받은 적이 있다…

사실 지금까지도 해당 질문에 대한 의도를 잘못 파악하고 있었던 것 같다.

그 질문은 Transactional을 붙인 이유에 대해서 설명해달라는 질문이었는데, 그냥 Service 로직 안에는 모든 메서드에 붙여야 한다고 대답했었다.

아마 면접관분께서 이 부분을 보고 “이 사람이 트랜잭션을 굳이 안써도 되는 곳에 트랜잭션을 사용했는데 이유가 있는걸까?” 를 물어본게 아닐까 싶다.

findAll은 전체 데이터를 받아오는 메서드이다.

즉, 데이터의 수정 또는 삭제가 이루어지지 않으므로 중간에 문제가 생겨도 그냥 데이터가 안받아와질 뿐이기 때문에 트랜잭션이 필요없다. 오히려 트랜잭션을 위해 사용되는 비용만 발생될 뿐이다.

그래서 트랜잭션이 필요없는 곳에 @Transactional을 삭제해주려고 했는데, readOnly 속성에 대해 알게되었다.

읽기전용인 부분에서는 @Transactional을 아예 사용하지 않는 것보다 readOnly 옵션을 true로 해주는 것이 성능상에 더 이점이 있다고 한다.

그 이유는 다음과 같다.

1. 조회한 데이터를 return만 한다고 해도, 의도치 않게 데이터가 변경되는 혹~~시나 하는 상황을 미연에 방지해준다.

2. readOnly = true 옵션인 경우는 CUD 관련한 트랜잭션 작업, 스냅샷 저장, 변경감지 등의 작업을 수행하지 않기 때문에 성능상에서도 손해를 보지 않을 수 있다.

3. 만약 DB가 이중화를 통한 master - slave 구조로 되어 있다면 추가 설정을 통해 CUD 는 masterDB에서, Read는 slave에서 하는 식으로 workload를 분산할 수 있다.

4. 마지막으로 @Transactional(readOnly=true) 를 하는것이 좀더 가독성이 좋다. 한눈에 읽기 전용 작업인 것을 인지할 수 있다.

이러한 이유 때문에, Transactional 어노테이션을 사용하되, 읽기 전용 작업에서는 해당 옵션을 ture로 해주는 것이 좋다는 것을 알게 되었다.

[링크] 리팩터링 완료한 커밋

마무리

오늘은 자존심(?)이 상하지만 AI에게 코드 리뷰를 받아보았다.

사실 생각한 것 보다도 더 날카롭고 진짜 생각해볼만한 거리를 여러개 던져주어서, 신입 개발자라서 주변에 코드리뷰를 도와줄 누군가가 없다면 진짜 큰 도움이 될 기술이 하나 나왔다는 생각이 들었다.

그렇다고 AI가 시키는 대로만 변경해서는 안될 것 같고, AI가 추천해준 내용들에 대해서 “왜 이렇게 하라고 할까?” 라는 의문을 갖고 하나씩 공부하다보면 어느새 더 좋은 코드를 쓰는 개발자가 될 수 있을 것같다.

꾸준히 AI의 힘을 빌려 코드리뷰를 받아보도록 해야겠다는 생각이 들었다.

남은 AI 추천 개선점은 다음 게시물에서 마저 리팩터링 해보도록 하겠다.