SonarLint를 기반하여 code smell 줄이기
Closed this issue · 3 comments
목적
SonarLint의 경고메시지를 기반한 리팩토링을 통해 버그 발생 가능성을 줄인다.
체크리스트
- code smell Major -> Minor 순으로 해결하기
- 해결한 이슈마다 경고메시지가 뜬 원인, 개선한 점, 기대효과 등을 정리하여 문서화한다.
주의 사항
관련 이슈
일정
해결한 나쁜코드들
-
[Major] Generic exceptions should never be thrown (Generic exception들을 절대 던지지 마라)
- Error, RuntimeException, Throwable, Exception 같은 예외를 던지면 예외를 제대로 처리하지 못하는 일이 발생 할 수 있다고 하네요
- 지워진 addLike메서드에
throw new RuntimeException();
가 있었음. 이 메서드는 지워진 거라서 지금 코드에선 노상관!
-
[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();
-
-
[Major] Null pointers should not be dereferenced (NPE 방지하라는 뜻)
-
//LoginMemberArgumentResolver.resolveArgument() //변경 전 String methodName = parameter.getMethod().getName(); //변경 후 String methodName = Objects.requireNonNull(parameter.getMethod()).getName();
-
-
[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
이라고 지우래요. 그래서 지움
- 메서드 선언 뒤에 붙는 throws문은 다음과 같은 경우에 불필요하대요
-
[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());
-
-
[Minor] Unnecessary imports should be removed (불필요한 import를 지워라)
- 불필요한 import문이 여기저기 있었음
- 이건.. 왜 남아있는지 모름.. 일단 지움
-
[Info] JUnit5 test classes and methods should have default package visibility (테스트 클래스의 접근제어자는 제거해도 된다는 뜻)
-
JUnit4은 테스트 클래스가 public이어야 하는 반면, JUnit5는 좀 더 접근제어자에 대해 관대하다고 합니다.
-
가독성을 위해 접근제어자는 제거하는게 권장사항인가 봅니다.
-
//변경 전 public class AnalysisServiceTest { ... } //변경 후 class AnalysisServiceTest { ... }
-
-
[Info] Consecutive AssertJ "assertThat" statements should be chained (연속적인 "assertThat"문은 연결되어야한다.)
-
//변경 전 assertThat(savedLike).isNotNull(); assertThat(savedLike).isEqualTo(like); //변경 후 assertThat(savedLike) .isNotNull() .isEqualTo(like);
-
굳이..? 라는 생각은 들지만 연결되면 좋다니까.. 고침..
-
해결못한 나쁜코드
- [Minor] Test classes should comply with a naming convention (테스트코드 네이밍 컨벤션 맞추라는 뜻)
- 테스트코드 파일명은 뒤에 ...Test가 붙는 게 컨벤션이라네요!
- ArticleDocumentation.java 같은 테스트코드는 개인적으로 Test 안붙이는게 낫다고 생각해서 안고침
- [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 데일리에서 결정)
-
SonarLint는 컨트롤러 메서드 인자로 엔티티를 사용하면, 사용자 마음대로 DB에서 필드의 내용을 변경할 수 있는 가능성이 있기 때문에 바꾸라고 한 것으로 나오네용 (하단 캡처)
히로가 말한대로, JSON 형태에서 바로 엔티티가 만들어져 컨트롤러에 들어올 때는 SonarLint가 지적한 문제가 생길 수 있는데
LoginMemberArgumentResolver에서 검증된 Member 엔티티를 만들어 주는 것이기 때문에 이 메세지는 반영하지 않아도 될 것 같다고 했었죠?!
또 중복 코드가 다시 생기는 것 + 기타 비용 발생 등등 ㅎㅎ
혹 잘못 이해한 부분 있으면 말씀 부탁쓰~.~
@icyMojito 저도 그렇게 이해했어요!!