본문 바로가기
PMD

[한글화 시리즈-08] Design Rules(2)

배씨 2013. 7. 5. 17:42

Design Rules(2)

이 룰셋은 클래스 디자인 시 고려할만한 룰을 포함한다.


AvoidSynchronizedAtMethodLevel

메소드 단위의 동기화는 메소드 내에 새로운 코드가 추가되거나 할 경우 역효과를 낼 수 있다. 그러므로, 필요한 부분만 동기화 블록을 이용하는 것이 더욱 효율적이다.

public class Foo {
  // 이런 방식은 피하자!
  synchronized void foo() {
  }
  // 선호되는 방식
  void bar() {
    synchronized(this) {
    }
  }
}

MissingBreakInSwitch

switch 내의 case는 꼭 break;로 끝내자. 버그를 만드는 가장 쉬운 방법이다.

public class Foo {
  public void bar(int status) {
    switch(status) {
      //이런식으로 break; 가 없는 case들은 아래의 모든 코드가 실행될 수 있다.
      case CANCELLED:
        doCancelled();
      case NEW:
        doNew();
      case REMOVED:
        doRemoved();
    }
  }
}

UseNotifyAllInsteadOfNotify

Thread.notify()는 객체를 모니터링하는 스레드를 깨우는 역활을 한다. 만약 객체를 모니터링 중인 thread가 하나 이상 여러개라면 notify()는 무작위로 하나의 thread를 선택하여 깨운다. 그러므로 더욱 안전한 방식은 notifyAll()을 이용하여 모든 thread들에가 알리는 것이 더 좋다.

public class Foo {
  void bar() {
    //여러 개의 thread가 모니터링 중이라도 불특정의 단 하나의 thread만 알 수 있다. 
    x.notify();
    // 그러므로 여러 개의 thread가 모니터링 중이라면 다음을 사용하자.
    x.notifyAll();
  }
}

AvoidInstanceofChecksInCatchClause

catch절 내에서 발생된 exception은 그 안에서 처리되어야 한다. catch(Exception)으로 모든 오류를 전달 받아서 instanceof를 이용하여 처리하는 방식은 사용하지 말아야 한다.

// 이런 방식의 오류 처리는 피해야 한다.
try { 
  ...
} catch (Exception ee) {
  if (ee instanceof IOException) {
    cleanup();
  }
}
// 이런 방식으로 오류를 처리해야 한다.
try {  
  ...
// 각 오류별 예외 처리를 하자.
} catch (IOException ee) {
  cleanup();
}

AbstractClassWithoutAbstractMethod

추상화 클래스(abstract class)는 하나 이상의 abstract methods를 포함해야 한다. 추상화 클래스는 완전한 클래스가 아닌, 클래스 구조를 자식 클래스가 상속 받아 완성시키는 형태이다. 그러므로 만약 클래스가 추상화 클래스가 아닌 오직 기본 클래스(base class)로 사용되도록 의도되었다면 public이 아닌 protected로 접근 제한된 생성자를 제공하여 외부에서 직접적으로 클래스를 인스턴스(instantiation)화 하는 것을 막아야 한다.

public abstract class AbstractClass {
  void int method1() { ... }
  void int method2() { ... }
  // 추상화 클래스는 abstract method를 제공해야 한다.
  void abstract int method3();
}

// 그렇지 않고 단순 기본 클래스(base class)로 사용할 것이라면 
// abstract 선언을 지우고  protected 생성자를 제공하자. 
public class BaseClass {
  void int method1() { ... }
  void int method2() { ... }
  protected BaseClass() {}
}

SimplifyConditional

instanceof 이전에 null을 체크할 필요는 없다. instanceof 키워드는 인수(argument)로 null을 전달 받으면 false를 반환하기 때문이다.

class Foo {
  void bar(Object x) {
    if (x != null && x instanceof Bar) {
      // x != null은 필요 없다. 삭제하자!
    }
  }
}

CompareObjectsWithEquals

객체의 비교는 ==이 아닌 equals()를 이용하여 비교하자. 

class Foo {
  //나쁜 방법
  boolean bar(String a, String b) {
    return a == b;
  }

  //좋은 방법
  boolean bar(String a, String b) {
    return a.equals(b);
  }
}

PositionLiteralsFirstInComparisons

단순 문자열 비교와 String 객체를 비교할 경우 문자열을 앞에 두자. String 객체가 앞에 있을 때 이 객체가 null이면 NullPointerException을 발생시키지만, 문자열이 앞에 있다면 단순히 false만 반환한다.

class Foo {
  boolean bar(String x) {
    //x가 null이면 NullPointerException이 발생
    return x.equals("2"); 
    //return "2".equals(x);을 사용하자.
  }
}

UnnecessaryLocalBeforeReturn

불필요한 지역변수들을 생성하지 말자.

public class Foo {
  public int foo() {
    int x = doSomething();
    return x;  
    // 그냥 'return doSomething();' 으로 불필요한 생성을 줄이자.
  }
}

NonThreadSafeSingleton

스래드에 안전하지 않은 singletons은 좋지 않은 상태 변화를 초래할 수 있다. 객체를 직접적으로 인스턴스화 할 수 있는 상태의 static singletons는 제거하자. Static singleton은 일반적으로 오직 하나의 인스턴스만 존재하도록 요구하지는 않는다. 다른 가능한 수정방법은 메소드 전체를 동기화하거나 요청 클래스에서 초기화하는 방법이 있다. 자세한 내용은 책 Effective Java의 48번 항목을 확인 하거나, 이 블로그의 Design Rules(1)의 UseSingleton 룰을 확인하자.

private static Foo foo = null;

//멀티 스레드 상황에서 이 메소드가 동시에 호출되면 부분적으로 초기화된 객체가 반환 될 수 있다. 
//자세한 내용은 이 블로그의 UseSingleton에서 확인하자.
public static Foo getFoo() {
    if (foo==null)
        foo = new Foo();
    return foo;
}

UncommentedEmptyMethod

비어있는 메소드가 주석이 있으면 어떤 의도로 비어 놓은 메소드로 인식할 수 있지만, 주석이 없다면 의도되지 않은 메소드로 구분될 수 있다. 의도적으로 비워둔 메소드에는 꼭 주석을 달자.

//아래의 주석이 없는 메소드는 의도하지 않은 실수로 비워둔 메소드로 인식된다.
//의도적으로 비워둔 메소드라면 주석을 달자.

public void doSomething() {
}

UncommentedEmptyConstructor

비어있는 생성자가 주석이 있으면 어떤 의도로 비어 놓은 생성자로 인식할 수 있지만, 주석이 없다면 의도되지 않은 생성자로 구분될 수 있다. 의도적으로 비워둔 생성자에는 꼭 주석을 달자.

//아래의 주석이 없는 생성자는 의도하지 않은 실수로 비워둔 생성자로 인식된다.
//의도적으로 비워둔 생성자라면 주석을 달자.

public Foo() {
  super();
}

AvoidConstantsInterface

인터페이스(interface)는 오직 클래스의 행위를 정의하는 모델로서의 역할로만 사용되어야 한다. 만약 인터페이스를 상수를 담는 그릇을 사용하는 것은 매우 나쁜 방식이다.

//아래와 같이 상수를 interface에 담는 것은 매우 나쁜 방식이다.
public interface ConstantsInterface {
  public static final int CONSTANT1=0;
  public static final String CONSTANT2="1";
}

UnsynchronizedStaticDateFormatter

SimpleDateFormat은 동기화되어 있지 않다. 그러므로 Sun은 각 스레드마다 개별적인 SimpleDateFormat 인스턴스를 사용하도록 권고하고 있다. 만약 멀티 스레드가 반드시 static SimpleDateFormat를 접근해야 한다면, 메소드 단위 또는 블럭 단위의 동기화가 필요하다.

//개별적인 SimpleDateFormat 인스턴스를 사용하는 것이 가장 좋은 방법이다.
//만약 불가피하게 SimpleDateFormat를 static으로 사용해야 한다면,
//아래의 방식을 참고하자.
public class Foo {
  private static final SimpleDateFormat sdf = new SimpleDateFormat();
  void bar() {
    sdf.format(); // 불안정한 사용
  }
  synchronized void foo() {
    sdf.format(); // 스레드 동기화 되어 안정된 사용
  }
}

PreserveStackTrace

catch절에서 새로운 exception instance를 생성할 때 원래의 exception을 전달 인자로 받지 않고 생성하지 않으면, 디버깅을 위한 stack trace 정보를 잃어버릴 수 있다. 그러므로, 효과적인 디버깅을 어렵게 만들 수 있다.

public class Foo {
  //좋은 방식
  void good() {
    try{
      Integer.parseInt("a");
    } catch(Exception e) {
      //stack trace가 가능하다
      throw new Exception(e);
    }
  }
  //나쁜 방식
  void bad() {
    try{
      Integer.parseInt("a");
    } catch(Exception e){
      //stack trace가 불가능하다.
      throw new Exception(e.getMessage());
    }
  }
}

UseCollectionIsEmpty

java.util.Collection은 collection이 비어있는지 확인 하는 메소드로 isEmpty()를 제공하고 있다. 그러므로 .size() == 0과 같은 방식으로 collection이 비어 있는지를 비교하는 방식은 기존에 존재하는 기능을 중복하여 잘못 사용하는 것이다.

public class Foo {
  //좋은 방식
  void good() {
    List foo = getList();
    //정확한 메소드를 사용하여 비어있는지 확인
    if (foo.isEmpty()) {
      ...
    }
  }

  void bad() {
    List foo = getList();
    //잘못 사용된 메소드
    if (foo.size() == 0) {
      ...
    }
  }
}

ClassWithOnlyPrivateConstructorsShouldBeFinal

클래스가 오직 private 생성자만을 포함한다면 내부 클래스(inner class)가 호출하지 않는 이상, 이 클래스는 final을 갖고 있어야 한다.

public class Foo {  //public final class Foo가 되어야 한다.
  private Foo() { }
}

EmptyMethodInAbstractClassShouldBeAbstract

추상화 클래스(abstract class)의 비어있는 메소드는 abstract로 설정되어야 한다. 개발자들은 간혹 이런 비어있는 구현을 적절하게 구현된 코드보다 더 신뢰한다.

public abstract class ShouldBeAbstract{
  // abstract로 설정하자.
  public Object couldBeAbstract() {
    return null;
  }

  // 이것도 abstract로 설정하자.
  public void couldBeAbstract(){}
}

SingularField

만약 클래스 맴버 변수가 하나의 메소드에서만 사용되고 이 맴버 변수의 처음 사용이 값을 등록하는 것이라면, 이 맴버 변수는 아마도 로컬 변수로 변환하는 것이 더 나은 방법이다.

public class Foo {
  private int x; //왜 클래스 맴버 변수로 등록해야하지?
  public void foo(int y) {
    x = y + 5;
    return x;
  }
}

ReturnEmptyArrayRatherThanNull

어떠한 메소드라도 array를 반환한다면, null보다 비어있는 array를 반환하는 것이 더 나은 방법이다. 비어 있는 array가 예외 사항을 처리하기에 더욱 간편하다.

public class Example {
  //좋지 않은 아이디어
  public int []badBehavior() {
    // ...
    return null;
  }
  // 좋은 방식
  public String[] bonnePratique() {
    //...
    return new String[0];
  }
}

AbstractClassWithoutAnyMethod

만약 추상화 클래스(abstract class)가 어떠한 메소드도 제공하지 않는다면, 이 클래스는 인스턴스화 되지 않는 단순 데이터 컨테이너이다. 이런 경우, 클래스를 추상 클래스로 만드는 것 보다, 생성자를 private 또는 protected로 접근제한한 구현을 막는 방식이 더 나은 방법이다.

//단순 데이터 컨테이너라면 abstract가 필요 없다.
public class abstract Example {
  String field;
  int otherField;
}

//데이터 컨테이너로써는 이 방식이 더 나은 방법이다.
public class Example {
  String field;
  int otherField;
  protected Example(){}
}

TooFewBranchesForASwitchStatement

switch는 복잡한 분기점을 위하여 고안되었으며, 각 분기점이 처리를 공유하도록 되어있다. 3개 미만의 분기점을 위해서 switch를 사용하는 것은 좋은 방법이 아니며, if와 같이 가독성이 높지 않다. 이런 경우, if문을 이용하는 것이 최소한 가독성을 높일 수는 있다.

//switch를 사용하려면 최소한 3개 이상의 분기점들이 필요하다.
public class Foo {
  public void bar() {
    switch (condition) {
      case ONE:
        instruction;
        break;
      default:
        break; //이렇게 분기점이 적을 경우 if문이 더욱 적합하다.
    }
  }
}

해당 URL: http://pmd.sourceforge.net/pmd-4.2.6/rules/design.html

저작자표시 비영리 변경금지

'PMD' Related Articles

티스토리툴바