woowacourse-teams/2020-saebyeok

SonarLint를 기반하여 code smell 줄이기

Closed this issue · 3 comments

hwanghe159 commented

목적

SonarLint의 경고메시지를 기반한 리팩토링을 통해 버그 발생 가능성을 줄인다.

체크리스트

  • code smell Major -> Minor 순으로 해결하기
  • 해결한 이슈마다 경고메시지가 뜬 원인, 개선한 점, 기대효과 등을 정리하여 문서화한다.

주의 사항

관련 이슈

일정

hwanghe159 commented

해결한 나쁜코드들

  1. [Major] Generic exceptions should never be thrown (Generic exception들을 절대 던지지 마라)

    • Error, RuntimeException, Throwable, Exception 같은 예외를 던지면 예외를 제대로 처리하지 못하는 일이 발생 할 수 있다고 하네요
    • 지워진 addLike메서드에 throw new RuntimeException();가 있었음. 이 메서드는 지워진 거라서 지금 코드에선 노상관!

  2. [Major] Raw types should not be used (raw 타입을 사용하지 마라)

    • List myList; //이렇게 쓰지 말고
Set mySet; //이렇게 쓰지 말고

List<String> myList; //이렇게 써라
Set<? extends Number> mySet; //이렇게 써라
    • //CustomOAuth2UserService.loadUser()

//변경 전
OAuth2UserService delegate = new DefaultOAuth2UserService();
//변경 후
OAuth2UserService<OAuth2UserRequest, OAuth2User> delegate = new DefaultOAuth2UserService();

  3. [Major] Null pointers should not be dereferenced (NPE 방지하라는 뜻)

    • //LoginMemberArgumentResolver.resolveArgument()

//변경 전
String methodName = parameter.getMethod().getName();

//변경 후
String methodName = Objects.requireNonNull(parameter.getMethod()).getName();

  4. [Minor] "throws" declarations should not be superfluous ("throws"를 불필요하지 않게 하라.)

    • 메서드 선언 뒤에 붙는 throws문은 다음과 같은 경우에 불필요하대요
      • void foo() throws MyException, MyException {} 같이 여러 번 나열하는 경우
      • void bar() throws Throwable, Exception {} 같이 다른 예외의 하위 예외인경우
      • RuntimeException 또는 그 하위클래스 중 하나
      • 절대 던져질 수 없는 예외
    • public OAuth2User loadUser(OAuth2UserRequest userRequest) throws OAuth2AuthenticationException 여기서 OAuth2AuthenticationException가 RuntimeException이라고 지우래요. 그래서 지움

  5. [Minor] Return of boolean expressions should not be wrapped into an "if-then-else" statement (boolean을 반환하는 메서드를 if-else로 감싸지 말라는 뜻)

    • //변경 전
if (claims.getBody().getExpiration().before(new Date())) {
    return false;
}
return true;
    • //변경 후
return !claims.getBody().getExpiration().before(new Date());

  6. [Minor] Unnecessary imports should be removed (불필요한 import를 지워라)

    • 불필요한 import문이 여기저기 있었음
    • 이건.. 왜 남아있는지 모름.. 일단 지움

  7. [Info] JUnit5 test classes and methods should have default package visibility (테스트 클래스의 접근제어자는 제거해도 된다는 뜻)

    • JUnit4은 테스트 클래스가 public이어야 하는 반면, JUnit5는 좀 더 접근제어자에 대해 관대하다고 합니다.

    • 가독성을 위해 접근제어자는 제거하는게 권장사항인가 봅니다.

    • //변경 전
 public class AnalysisServiceTest { ... }
 //변경 후
 class AnalysisServiceTest { ... }

  8. [Info] Consecutive AssertJ "assertThat" statements should be chained (연속적인 "assertThat"문은 연결되어야한다.)

    • //변경 전
assertThat(savedLike).isNotNull();
assertThat(savedLike).isEqualTo(like);

//변경 후
assertThat(savedLike)
        .isNotNull()
        .isEqualTo(like);

    • 굳이..? 라는 생각은 들지만 연결되면 좋다니까.. 고침..

해결못한 나쁜코드

  1. [Minor] Test classes should comply with a naming convention (테스트코드 네이밍 컨벤션 맞추라는 뜻)
    • 테스트코드 파일명은 뒤에 ...Test가 붙는 게 컨벤션이라네요!
    • ArticleDocumentation.java 같은 테스트코드는 개인적으로 Test 안붙이는게 낫다고 생각해서 안고침
  2. [Info] Complete the task associated to this TODO comment. (TODO 처리하라)
    • TODO는 일단 냅둠!

논의해보고 싶은 것

  • [Critical] Persistent entities should not be used as arguments of "@RequestMapping" methods (컨트롤러의 메소드의 인자는 엔티티를 사용하지 말라.)

    • SonarLint를 한번 쭉 돌리면 이 메시지가 가장 많이 뜸.

    • 심지어 Critical임!

    • 사실 이걸 해결하고 싶어서 이 이슈를 시작한 점도 없지않아 있음 ㅎㅎ

    • 이 메시지의 내용은 우리가 그동안 컨트롤러의 메서드의 인자로

      @PostMapping("/articles")
public ResponseEntity<Void> createArticle(@LoginMember Member member, @Valid @RequestBody ArticleCreateRequest request) {
    //..생략
}

      위와 같이 Member 엔티티를 사용중이니까 엔티티말고 DTO를 사용하라는 뜻임 (예를들어 security 패키지 안에 있는 User.java같은 애들 사용)

    • 즉, @LoginMember와 연결된 리졸버가 Member을 반환하지 않고, User을 반환해야 한다는 말.

    • 근데 문제는 이렇게 하면 리졸버에서 완전한 Member을 반환해주지 않고, User을 반환해주기 때문에

      Member member = memberRepository.findById(user.getId())
        .orElseThrow(() -> new MemberNotFoundException(user.getId()));

      위와 같이 user -> member 변환 로직을 member을 사용하는 모든 서비스 메서드에 다 붙여줘야 함...

    결론 : 중복 코드를 감수하고 Critical한 코드 스멜을 줄이자 VS 지금 하던대로 가자. => 지금 하던대로 ㄱㄱ(10/20 데일리에서 결정)

  • 👍1
icyMojito commented

SonarLint는 컨트롤러 메서드 인자로 엔티티를 사용하면, 사용자 마음대로 DB에서 필드의 내용을 변경할 수 있는 가능성이 있기 때문에 바꾸라고 한 것으로 나오네용 (하단 캡처)
히로가 말한대로, JSON 형태에서 바로 엔티티가 만들어져 컨트롤러에 들어올 때는 SonarLint가 지적한 문제가 생길 수 있는데
LoginMemberArgumentResolver에서 검증된 Member 엔티티를 만들어 주는 것이기 때문에 이 메세지는 반영하지 않아도 될 것 같다고 했었죠?!
또 중복 코드가 다시 생기는 것 + 기타 비용 발생 등등 ㅎㅎ
혹 잘못 이해한 부분 있으면 말씀 부탁쓰~.~

스크린샷 2020-10-20 오후 11 06 30

hwanghe159 commented

@icyMojito 저도 그렇게 이해했어요!!

  • 🚀1