By SonarLint: 놓쳤던 code smell 청소하기

[Gunyoung-Kim]

Touch My Body에 대해서 그 동안 지속된 리팩토링을 통해 코드 품질을 높여가는 작업을 진행했다. 인간은 실수의 동물이란 말이 있다. 많은 고민을 통해 리팩토링을 진행했지만 코드 어딘가에는 내가 미쳐 생각하지 못했던 부분에서 code smell이 존재하지 않을까라는 생각에 sonarLint를 통해 이를 보완하기로 했다.

앞으로 SonarLint를 통해 내가 놓쳤던 사항에 대한 것은 이 issue에 기록을 해보려한다.

[Gunyoung-Kim]

1. Local variables should not be declared and then immediately returned 반영 커밋: https://github.com/Gunyoung-Kim/Touch-My-Body/commit/eb2b6eab64a32742c61db1ce7ba4bfec2b88023e

       @Bean(name="redisCacheConnectionFactory")
    public RedisConnectionFactory redisConnectionFactory() {
        LettuceConnectionFactory lettuceConnectionFactory = new LettuceConnectionFactory(redisCacheHost,redisCachePort);
        return lettuceConnectionFactory;    
    }

위 코드에서 lettuceConnectionFactory 변수에는 새로운 LettuceConnectionFactory가 할당이 되고 바로 return됐다. 이 코드에서 lettuceConnectionFactory 변수의 선언은 의미 없이 코드 수만 증가시키기에 변수 선언부분을 삭제하는 것이 타당하다.

-> 리팩토링 결과

       @Bean(name="redisCacheConnectionFactory")
    public RedisConnectionFactory redisConnectionFactory() {
        return new LettuceConnectionFactory(redisCacheHost, redisCachePort);
    }

[Gunyoung-Kim]

2. RequestMapping 어노테이션보다는 GetMapping, PostMapping, DeleteMapping, PutMapping 어노테이션을 사용하자 반영 commit : https://github.com/Gunyoung-Kim/Touch-My-Body/commit/49fb749ee070a6621a084f159f95353487fea743, https://github.com/Gunyoung-Kim/Touch-My-Body/commit/18bf2325744f6ceaaa456e81ed141b6ee1468623

@RequestMapping(value="/community",method=RequestMethod.GET)
@RequestMapping(value = "/community/post/{postId}/addLike", method = RequestMethod.POST)
@RequestMapping(value = "/community/post/{postId}/removelike", method = RequestMethod.DELETE)

@GetMapping(value="/community")
@PostMapping(value = "/community/post/{postId}/addLike")
@DeleteMapping(value = "/community/post/{postId}/removelike")

RequestMapping 어노테이션 + RequestMethod 방식보다는 후자의 방식이 좀 더 직관적이라 가독성에 도움이 된다.

[Gunyoung-Kim]

3. 'catch' clauses should do more than rethrow 반영 commit : https://github.com/Gunyoung-Kim/Touch-My-Body/commit/df20ebbc8624ca922a5b2819ae0e8b655992625a

아래 코드는 ManagerUserController의 manageUserComments 메소드다.

public ModelAndView manageUserComments(@PathVariable("user_id") Long userId, @RequestParam(value = "page", required = false, defaultValue = "1") int page
            , @RequestParam(value = "order", defaultValue = "desc") String order, ModelAndPageView mav) {
        User user = userService.findById(userId);
        if(user == null) {
            throw new UserNotFoundedException(UserErrorCode.USER_NOT_FOUNDED_ERROR.getDescription());
        }

        Page<Comment> pageResult;
        try {
            pageResult = getPageResultForUserCommentListViewForManage(order, userId, page);
        } catch(SearchCriteriaInvalidException scie) {
            throw scie;
        }
        long totalPageNum = getTotalPageNumForUserCommentListViewForManage(userId);

        List<CommentForManageViewDTO> commentListForView = CommentForManageViewDTO.of(pageResult, user);

        mav.addObject(ATTRIBUTE_NAME_OF_USER_ID, userId);
        mav.addObject("username", user.getFullName() + " : " + user.getNickName());
        mav.setPageNumbers(page, totalPageNum);
        mav.addObject("commentList", commentListForView);

        mav.setViewName("userCommentList");

        return mav;
    }

메소드 중간에 사용된 getPageResultForUserCommentListViewForManage 메소드는 전달된 order 변수가 적절한 정렬기준을 나타내지 않으면 SearchCriteriaInvalidException 예외를 던진다. 저 부분에 try-catch 문을 그동안 두었던 이유는 getPageResultForUserCommentListViewForManage 메소드에서 이러한 예외가 발생할 수 있음을 좀 더 잘 보여주기 위해 추가했지만 오히려 이 때문에 코드 가독성이 더 떨어지게 되었다. catch 문에서 단순히 예외를 던지기만 한다면 try-catch문을 삭제해도 똑같이 작동하기 때문에 try-catch문을 삭제했다.

[Gunyoung-Kim]

4. "entrySet()" should be iterated when both the key and value are needed 반영 commit : https://github.com/Gunyoung-Kim/Touch-My-Body/commit/18bf2325744f6ceaaa456e81ed141b6ee1468623 , https://github.com/Gunyoung-Kim/Touch-My-Body/commit/6cd5107c203c97771f14c90860e04c4bdd5f8e1d

public List<ExerciseInfoBySortDTO> getExercisesByNameAndTarget() {
        List<ExerciseInfoBySortDTO> resultList = new ArrayList<>();
        Map<String ,List<String>> exerciseSortResultMap = exerciseService.getAllExercisesNamewithSorting();

        // 변경 전 
        for(String targetTypeName: exerciseSortResultMap.keySet()) {
            resultList.add(new ExerciseInfoBySortDTO(targetTypeName, exerciseSortResultMap.get(targetTypeName)));
        }

        // --> 변경 후 
        for(Entry<String, List<String>> entry: exerciseSortResultMap.entrySet()) {
            String targetTypeName = entry.getKey();
            List<String> listOfExerciseName = entry.getValue();
            resultList.add(new ExerciseInfoBySortDTO(targetTypeName, listOfExerciseName));
        }

        return resultList;
}

기존 방식은 for-each문에서 exerciseSortResultMap의 keySet으로 순환했다. 하지만 위의 경우는 for문 안에서 Map의 key 뿐만 아니라 value 값도 참조하기때문에 가독성을 위해 for-each문에서 exerciseSortResultMap 의 entrySet으로 순환하도록 리팩토링하였다.

[Gunyoung-Kim]

5. Lambdas should be replaced with method references 반영 commit : https://github.com/Gunyoung-Kim/Touch-My-Body/commit/dca8a03df7b8711840949ccd29e3fd57ab196bfd

       @Override
    public void checkIsMineAndDelete(Long userId, Long commentId) {
        Optional<Comment> comment = commentRepository.findByUserIdAndCommentId(userId, commentId);

                // 변경 전
        comment.ifPresent((c) -> {
            delete(c);
        });

                // 변경 후 
        comment.ifPresent(this::delete);
    }

위와 같이 단순히 하나의 메소드 호출만을 하는 람다식은 메소드 참조로 변경해주는 것이 가독성이 훨씬 좋아진다. 중괄호는 가독성의 천적이기 때문이다. 그리고 위 변경 전 코드에서 c 를 소괄호로 무의식적으로 감쌌는데 인자가 하나이기에 이는 없애는 것이 맞다.

단순하고 당연한 원칙이었지만 그동안 간과하고 있었고 실제로 TouchMyBody 코드 곳곳에 이러한 이유로 code smell을 풍기고 있었다. 주의해야한다.

[Gunyoung-Kim]

6. JUnit5 test classes and methods should have default package visibility 반영 commit : https://github.com/Gunyoung-Kim/Touch-My-Body/commit/d39fb4ec1462a99d5634c258dae923a9b741ece4 , https://github.com/Gunyoung-Kim/Info/commit/795656d2aff6940a8861d893e35c79a24459eab6

JUNIT4까지는 테스트 클래스 및 메소드 모두에 비해 public 접근제어자만을 허용했다. 그런데 JUNIT5로 넘어오면서 private 을 제외한 public, protected, default 접근 제어자에 대해서 허용되었다.

아래는 JUNIT5의 코드 일부분이다.

abstract class IsTestableMethod implements Predicate<Method> {

    private final Class<? extends Annotation> annotationType;
    private final boolean mustReturnVoid;

    IsTestableMethod(Class<? extends Annotation> annotationType, boolean mustReturnVoid) {
        this.annotationType = annotationType;
        this.mustReturnVoid = mustReturnVoid;
    }

    @Override
    public boolean test(Method candidate) {
        // Please do not collapse the following into a single statement.
        if (isStatic(candidate)) {
            return false;
        }
        if (isPrivate(candidate)) {
            return false;
        }
        if (isAbstract(candidate)) {
            return false;
        }
        if (returnsVoid(candidate) != this.mustReturnVoid) {
            return false;
        }

        return isAnnotated(candidate, this.annotationType);
    }

}

public final class ReflectionUtils {
    public static boolean isPrivate(Member member) {
        Preconditions.notNull(member, "Member must not be null");
        return Modifier.isPrivate(member.getModifiers());
    }
}

위 코드를 보면 접근제어자가 private인 경우는 테스트 대상에서 제외되는 것을 볼 수 있다.

그리고 3가지중 default를 사용해야하는 이유는 단순히 코드의 간결성 때문이다.

[Gunyoung-Kim]

7. Persistent entities should not be used as arguments of "@RequestMapping" methods 반영 commit : https://github.com/Gunyoung-Kim/Info/commit/0ffd91387c327873ccb877d33554dc0cf4cfa6be

Entity 어노테이션 적용 객체는 DB에 바로 정보 수정을 가할 수 있는 객체이기 때문에 Request Parameter가 바인딩 되는 ReqeustMapping 어노테이션이 적용된 메소드의 인자로 사용하는 것은 적절치 못하다. 대신 DTO 객체를 이용하는 것이 더 좋다.