최근 회사에서 프로젝트에 소나큐브를 설정함에 따라, 중간 중간 시간이 남을 때 코드 악취를 제거하는 작업을 했었다. 오늘은 이 악취들을 제거하면서 배웠던 것들을 정리한다.
1. 추상 클래스의 공개 생성자를 제거해라
Abstract classes should not have public constructors. Constructors of abstract classes can only be called in constructors of their subclasses. So there is no point in making them public. The protected modifier should be enough.
추상 클래스의 생성자는 서브 클래스들에 의해서만 호출된다. 따라서 공개 생성자가 있을 필요가 없다. public이어도 컴파일과 실행을 잘되지만 최대 protected까지만 사용하자.
2. 유틸 클래스의 공개 생성자를 제거하라
Utility classes should not have public constructors
유틸 클래스란 정적 메소드들로만 이루어진, 유틸성 목적을 가진 클래스이다. 특정 객체를 유틸 클래스로 사용할지 빈으로 사용할지 선택하는 것은 또 다른 고민거리지만, 유틸 클래스를 사용하기로 했다면 주의할게 있다. 공개 생성자를 제거하자.
// Bad!!
// naive한 유틸 클래스. 상속과 인스턴스화가 가능하다
public class UtilClass {
public static add(int a, int b) {
return a + b;
}
}
// Good!!
// 상속을 막고 인스턴스화를 막은 유틸 클래스
public final class UtilClass {
private UtilClass() {
throw new IllegalStateException("must not intantiate util class");
}
public static add(int a, int b) {
return a + b;
}
}
// Good!!
// lombok을 사용한다면 간단하게 애노테이션 하나를 붙여 위와 동일한 코드를 생성할 수 있다
@UtilityClass
public class UtilClass {
public static add(int a, int b) {
return a + b;
}
}
"이렇게까지 해야하나?"라고 생각이 들 수 있으나 유틸 클래스의 특성상 워낙 다양한 곳에서 사용할 수 있는 점을 생각하자. 만든사람은 당연히 잘 알고 이상하게 사용하지 않겠지만, 위 클래스를 사용하는 모두가 그럴거라고는 보장할 수 없다.
3. 테스트에서 public 제어자를 제거하자
JUnit5 is more tolerant regarding the visibilities of Test classes than JUnit4, which required everything to be public.
In this context, JUnit5 test classes can have any visibility but private, however, it is recommended to use the default package visibility, which improves readability of code.
JUnit4는 테스트의 실행을 위해 클래스와 메소드가 public이어야 했다. 하지만 JUnit5로 오면서 리플렉션을 활요하기에 더이상 그럴 필요가 없다. 다른 테스트에서 사용할 목적이 아니라면 public 제어자를 제거하고 패키지 비공개 제어자를 사용하도록 하자. 덤으로 제어자를 작성하지 않아도 되니 코드의 가독성도 올라간다.
// Bad!!
public class Test {
@BeforeEach
public void setup() {
...
}
@Test
public void test() {
...
}
}
// Good!!
class Test {
@BeforeEach
void setup() {
...
}
@Test
void test() {
...
}
}4. 다양한 Assertion API를 활용하자
assertThat().containsEntry() 이나 assertThat().isZero() 같은 더 특수한 경우의 메소드를 통해 가독성을 높이자
// Bad!!
assertThat(sum).isEqualTo(0L)
assertThat(formDataRequest.get("file")).isEqualTo(mockUrl);
assertThat(formDataRequest.get("command")).isEqualTo(CommandType.SOME_COMMAND.name());
// Good!!
assertThat(upload).isZero();
assertThat(bizhowsDesignResourceSaveRequest)
.containsEntry("file", mockUrl)
.containsEntry("command", CommandType.SOME_COMMAND.name());5. 상수는 항상 정적으로 선언하자
Making a constant just final as opposed to static final leads to duplicating its value for every instance of the class, uselessly increasing the amount of memory required to execute the application.
상수를 static으로 선언하지 않는다면, 객체 인스턴스마다 해당 필드를 가지게 된다. 이는 불필요한 메모리 사용으로 이어지기 때문에 상수라면 항상 final을 붙이도록 하자.
// Bad!!
public class Myclass {
public final int THRESHOLD = 3;
}
// Good!!
public class Myclass {
public static final int THRESHOLD = 3;
} 6. Optional을 단순 null 체크를 위해 사용하지 말자
Optional은 메소드의 결과가 없음을 나타내기 위한 목적을 가지고 만들어 졌다. 단순 null 체크를 위해 사용하는 것은 과용이 아닐지 생각해보자
Optional을 제대로 사용해보고 싶다면? -> Java Optional 제대로 사용하기
// Bad!!
public void someMethod(SomeObject object) {
Optional.ofNullable(object).orElseThrow(() -> new NullPointerException());
...
}
// Good!!
public void someMethod(SomeObject object) {
if (object == null) {
throw new NullPointerException();
}
...
}위 코드는 Optional의 의도와 다르게 사용하고 있을 뿐만 아니라, null이 아닌 경우 객체를 반환함에도 이를 사용하지 않고 있다. 위 같은 상황에서는 그냥 명시적으로 null 체크를 하자
7. assert문을 최소 1개 이상 작성하자
테스트 코드에서 assert문을 작성하는 것은 당연하다고 생각할 수도 있으나, 그렇지 않은 경우가 있었다. 만약 검증할 사이드 이펙트가 없고, 아무 예외도 던지지 않는 것을 테스트하고 싶다면 어떻게 해야할까? Junit5의 assertDoesNotThrow()를 활용하자. 이름만으로도 뭘하는 메소드인지는 명확하다. assert문이 없는 것과 비교해서 테스트를 읽는 사람에게 의도를 명확히 전달할 수 있다.
class Sut {
void method(int param) {
if (param == 0) {
throw new RuntimeException();
}
}
}
// Bad!!
@Test
void test() {
int param = 1;
sut.method(param);
}
// Good!!
@Test
void test() {
int param = 1;
Executable execSut = () -> sut.method(int);
assertDoesNotThrow(execSut);
}8. assertThrows에는 메소드 하나만 호출하자
assertThrows()에 전달한 코드가 여러 곳에서 예외가 발생할 수 있다면, 거짓 양성이 나타나기 쉽고 무엇을 테스트하는지도 알기 어렵다.
// Before
@Test
void test() {
assertThrows(SomeException.class, () -> {
SomeObject some = new SomeObjcet();
some.setIdx(0);
some.checkValidRequest(1234); // 실제 테스트하려는 코드
});
}
// After
private final SomeObject sut = new SomeObjcet();
@Test
void test() {
sut.setIdx(0);
assertThrows(SomeException.class,
() -> sut.checkValidRequest(1234));
}9. 테스트 대상 시스템은 sut로 명명하자
이거는 스멜은 아니고 개인적으로 해보니 좋았던 점이다. 필드명을 지을때 테스트 대상 시스템은 sut로 명명하고, 목은 그냥 이름을 쓰자. 코드가 한결 간결해진다. 목 객체의 변수명은 mock으로 시작하는 것을 선호한다면 그렇게 해도 좋다. sut로 바꾸는 것만으로도 어떤 코드가 실행부인지 읽기 편해진다. 특히 테스트 코드가 길때 도움이 되는 경험을 했다.
// 이름이 길고 테스트 대상 시스템을 구분하기 어렵다
class Test {
@InjectMocks
SomeObject someObjectTryingToTest;
@Mock
DependObject1 mockDependObject1;
@Mock
DependObject2 mockDependObject2;
@Test
void test() {
given(mockDependObject1.getOne()).willReturn(Object);
given(mockDependObject2.getList()).willReturn(List.of(Object));
someObjectTryingToTest.doWork();
assertThat(...);
assertThat(...);
}
}
// Good!!
class Test {
@InjectMocks
SomeObject sut;
@Mock
DependObject1 dependObject1;
@Mock
DependObject2 dependObject2;
@Test
void test() {
given(dependObject1.getOne()).willReturn(Object);
given(dependObject2.getList()).willReturn(List.of(Object));
sut.doWork();
assertThat(...);
assertThat(...);
}
}