나는 카카오 테크 캠퍼스 활동 간 멘토님께 어떻게 하면 더 높은 품질의 코드를 작성할 수 있는지에 대해서 여쭈어봤었다. 멘토님께서는 여러 이야기들을 해주시면서 반드시 “sonarLint”를 사용해보라고 말씀해주셨다.
카카오 테크 캠퍼스 1기를 마무리하면서 멘토 선생님은 이제 안계시지만 SonarLint라는 새로운 선생님이 나를 잘 때려? 주실 것이다.
🧑🏫SonarLint 선생님의 회초리
내 코드
public class WalkResDTO {
@Getter @Setter
public static class WalkStatus {
private String walkStatus;
public WalkStatus(Walk walk) {
this.walkStatus = walk.getWalkStatus().toString();
}
}
@Getter @Setter
public static class StartWalk {
private Long id;
private String walkStatus;
private LocalDateTime startTime;
public StartWalk(Walk walk) {
this.id = walk.getId();
this.walkStatus = walk.getWalkStatus().toString();
this.startTime = walk.getStartTime();
}
}
@Getter @Setter @Builder
public static class EndWalk {
private Long userId;
private Long receiveMemberId;
private String profile;
private Long walkId;
private LocalDateTime walkStartTIme;
private LocalDateTime walkEndTime;
private Long notificationId;
private BigDecimal coin;
public static EndWalk of(Member member, Member receiver, Walk walk, Notification notification){
return EndWalk.builder()
.userId(member.getId())
.receiveMemberId(receiver.getId())
.profile(member.getProfileImage())
.walkId(walk.getId())
.walkStartTIme(walk.getStartTime())
.walkEndTime(walk.getEndTime())
.notificationId(notification.getId())
.coin(notification.getCoin())
.build();
}
}
@Getter @Setter
public static class FindByUserId {
private List<WalkStatusDTO> walkStatusDTOS;
public FindByUserId (List<Walk> walks){
this.walkStatusDTOS = walks.stream().map(WalkStatusDTO::new).collect(Collectors.toList());
}
@Getter @Setter
public static class WalkStatusDTO {
private Long id;
private String walkStatus;
public WalkStatusDTO(Walk walk) {
this.id = walk.getId();
this.walkStatus = walk.getWalkStatus().toString();
}
}
}
@Getter @Setter
public static class FindNotEndWalksByUserId {
private List<NotReviewedWalkDTO> walkStatusDTOS;
public FindNotEndWalksByUserId (List<Walk> walks, Long memberId){
this.walkStatusDTOS = walks.stream().map(walk -> new NotReviewedWalkDTO(walk, memberId)).collect(Collectors.toList());
}
@Getter @Setter
public static class NotReviewedWalkDTO {
private Long userId;
private Long receiveMemberId;
private Long walkId;
private String walkStatus;
private Long notificationId;
private boolean isReviewed;
private boolean isMaster;
public NotReviewedWalkDTO(Walk walk, Long memberId) {
this.userId = walk.getMaster().getId();
this.receiveMemberId = walk.getMaster().getId();
this.walkId = walk.getId();
this.walkStatus = walk.getWalkStatus().toString();
this.notificationId = walk.getNotification().getId();
this.isReviewed = walk.isReviewed();
this.isMaster = walk.getMaster().getId().equals(memberId);
}
}
}
}1️⃣ 첫번째 회초리 : Add a private constructor to hide the implict one.
Whenever there are portions of code that are duplicated and do not depend on the state of their container class, they can be centralized inside a "utility class". A utility class is a class that only has static members, hence it should not be instantiated.
Exceptions
When a class contains public static void main(String[] args) method it is not considered as a utility class and will be ignored by this rule.
SonarLint 선생님 회초리 요약
유틸 클래스는 오직 정적 멤버들만 가지고 있어야 하며, 객체로 생성되면 안된다.
클래스가 public static void main(String[] args) method를 포함하고 있으면, 유틸 클래스로 고려되지 않고 위 규칙은 무시된다.
SonarLint 선생님 추천 예시 코드
class StringUtils { // Compliant
private StringUtils() {
throw new IllegalStateException("Utility class");
}
public static String concatenate(String s1, String s2) {
return s1 + s2;
}
}내 생각 및 적용 후 코드
해당 DTO를 유틸 클래스라고 칭하기에는 무리가 있다고 생각한다.
유틸 클래스라 함은 인스턴스 메서드와 인스턴스 변수를 일절 제공하지 않고, 정적 메서드와 변수만을 제공하는 클래스이며, ‘비슷한 기능의 메서드와 상수를 모아서 캡슐화한 것’이다.
허나 DTO는 기능을 수행하는 것이 아니라 데이터를 담기 위한 목적의 POJO로 간주되기에 유틸 클래스로 보기 어렵다.
그러나 위 DTO 코드에서는 정적 inner 클래스를 통해서 외부 클래스를 인스턴스화 하지 않으므로, 명시적으로 외부 클래스의 인스턴스화를 막는 방법도 좋은 방법인 것 같다.
public class WalkResDTO {
private WalkResDTO(){
}
}2️⃣ 두번째 회초리 : A field should not duplicate the name of its containing class
It’s confusing to have a class member with the same name (case differences aside) as its enclosing class. This is particularly so when you consider the common practice of naming a class instance for the class itself.
Best practice dictates that any field or member with the same name as the enclosing class be renamed to be more descriptive of the particular aspect of the class it represents or holds.
SonarLint 선생님 회초리 요약
클래스와 같은 이름을 가진 인스턴스 멤버는 혼란을 불러 일으킨다.
모범 사례에서는 클래스와 동일한 이름을 가진 멤버를 해당 클래스가 나타내거나 특정 부분을 더 잘 설명하도록 바꾸는 것을 추천
내 생각 및 적용 후 코드
해당 정적 Inner 클래스 DTO를 구현할 때 조급한 시간에 급하게 작성하여서, 최대한 간단하게 작성하고자 하였다. 이를 다른 사람이 보게 될 생각까지는 못하였던 것 같다.
선생님의 말씀대로 해당 멤버를 클래스의 특정 부분을 더 잘나타내도록 네이밍을 해야겠다.
@Getter @Setter
public static class WalkStatus {
private String walkStatusField;
public WalkStatus(Walk walk) {
this.walkStatusField = walk.getWalkStatus().toString();
}
}3️⃣ 세번째 회초리 : "Stream.toList()" method should be used instead of "collectors" when unmodifiable list needed
In Java 8 Streams were introduced to support chaining of operations over collections in a functional style. The most common way to save a result of such chains is to save them to some collection (usually List). To do so there is a terminal method collect that can be used with a library of Collectors. The key problem is that .collect(Collectors.toList()) actually returns a mutable kind of List while in the majority of cases unmodifiable lists are preferred. In Java 10 a new collector appeared to return an unmodifiable list: toUnmodifiableList(). This does the trick but results in verbose code. Since Java 16 there is now a better variant to produce an unmodifiable list directly from a stream: Stream.toList().
This rule raises an issue when "collect" is used to create a list from a stream.
SonarLint 선생님 회초리 요약
이러한 코드의 핵심 문제는 .collect(Collectors.toList())가 실제로 변경 가능한 종류의 컬렉션을 반환하지만, 대부분이 수정 불가능한 종류의 컬렉션이 반환되기를 원한다.
SonarLint 선생님 추천 예시 코드
List<String> list1 = Stream.of("A", "B", "C").toList(); // Compliant
List<String> list2 = Stream.of("A", "B", "C")
.collect(Collectors.toList()); // Compliant, the list2 needs to be mutable
list2.add("X");내 생각 및 적용 후 코드
이 부분은 한번도 생각 못해봤는데, DTO 내부에서 반환되는 컬렉션은 반드시 수정이 불가능한 컬렉션이어야 한다. 그렇기에 선생님의 말씀대로 unmodifiable하게 작성하였다.
@Getter @Setter
public static class FindByUserId {
private List<WalkStatusDTO> walkStatusDTOS;
public FindByUserId (List<Walk> walks){
this.walkStatusDTOS = walks.stream().map(WalkStatusDTO::new).toList();
}
@Getter @Setter
public static class WalkStatusDTO {
private Long id;
private String walkStatus;
public WalkStatusDTO(Walk walk) {
this.id = walk.getId();
this.walkStatus = walk.getWalkStatus().toString();
}
}
}위 코드에서의 walkStatusDTOS가 수정이 불가능한 컬렉션인 이유는 Stream.toList()가 Collectors.UnmodifiableList 또는 Collectors.UnmodifiableRandomAccessList를 반환하기 때문이다.
두 구현체들은 수정이 불가능한 컬렉션이다.
JAVA 16에서부터 생긴 Stream.toList()에 대해서는 아래 레퍼런스를 참고하여 공부했다.
