[JAVA] 나쁜 코드 사례

원문 출처: http://www.odi.ch/prog/design/newbies.php#31
번역 출처: http://jnylove.tistory.com/192

 

# String concatenation

- very bad code
String s = "";
for (Person p : persons) {
    s += ", " + p.getName();
}
s = s.substring(2); //remove first comma

이건 정말 멍청한 짓이다. loop 안에서 String의 concatenation을 반복하는 것은 쓰잘데기 없는 array copy와 garbage를 남발하는 것이다.
게다가, 마지막에 콤마를 제거하는 연산을 한 번 더 해줘야 한다.

- better code
StringBuilder sb = new StringBuilder(persons.size() * 16); // well estimated buffer
for (Person p : persons) {
    if (sb.length() > 0) sb.append(", ");
    sb.append(p.getName);
}

# Lost StringBuffer performance

- not so good code
StringBuffer sb = new StringBuffer();
sb.append("Name: ");
sb.append(name + '\n');
sb.append("!");
...
String s = sb.toString();

좋아 보임에도 불구하고 위 코드는 문제가 있다. 가장 명백하게 눈에 띄는 실수는 3번째 라인의 String concatenation 이다.
4번째 라인의 char를 append 하는 것은 String을 append 하는 것 보다 빠르다.
또한 하나 간과한 것이 buffer의 사이즈를 초기화하지 않음으로해서, 불필요한 resizing(array copy)이 일어날 수 있다는 것이다.
JDK 1.5에서는 synchronization이 필요없는 로컬 변수에는 StringBuilder가 StringBuffer 대신에 사용된다.

- good code
StringBuilder sb = new StringBuilder(100);
sb.append("Name: ");
sb.append(name);
sb.append('\n');
sb.append('!');
String s = sb.toString();

# Testing for string equality

- not perfect
if (name.compareTo("John") == 0) ...
if (name == "John") ...
if (name.equals("John")) ...
if ("".equals(name)) ...

위 비교는 잘못된 결과를 초래할 수 있다. 
compareTo() method는 좀 지나치고, == 연산자는 객체 동치에 대한 연산을 수행하는데, 이는 아마도 원치 않는 결과를 초래할 것이다.
equals() method를 사용해야 할 것인데, 변수와 상수의 위치를 바꾸면 NullPointerException을 피할 수 있는 부가적인 안정성도 얻을 수 있고,
loop 안에서 equals() method가 호출될 때 equals() method가 같은 object로부터 호출이 되기 때문에 수행 속도의 향상도 가져올 수 있다.
빈 문자열을 체크할 때는 문자열의 길이를 체크하는 것이 가장 빠른데, 이는 equals() method가 hash code를 먼저 계산하기 때문이다.

if ("John".equals(name)) ...
if (name.length() == 0) ...

# Converting numbers to Strings

"" + set.size()
new Integer(set.size()).toString()

Set.size()의 반환값은 int 이다. 결과를 String 으로 바꾸려고 하는데, 위의 두 줄의 코드를 이용하여 String으로의 변환을 수행할 수 있으나,
첫번째 라인에서 String concatenation의 문제가 있고, 두번째 라인에서는 toString() method를 수행하기 위한 Integer class를 생성하기까지 한다.
이는 아래와 같이 간단하게 수정할 수 있다.

String.valueOf(set.size())

# Not taking adavantage of immutable objects

- code wastes resources
zero = new Integer(0);
return Boolean.valueOf("true");

Integer class와 Boolean class는 immutable이다. 즉, 불변객체이다. 쓸데없이 객체를 생성할 필요가 없다.
위 코드에서 사용된 Integer와 Boolean class 들은 자주 사용하는 instance들에 대한 built-in cache가 존재한다.
Boolean class의 경우는 값이 오로지 true/false 두가지 뿐이다.

- conservative code
zero = Integer.valueOf(0);
return Boolean.TRUE;

# XML parsers are for sissies

- native code
int start = xml.indexOf("<name>");
int end = xml.indexOf("</name>");
String name = xml.substring(start, end);

위에서 사용한 XML parsing은 오로지 정말 단순한 XML 문서에서만 동작을 한다.
다음과 같은 경우의 XML 문서에서는 오동작을 할 것이다.
a) 문서상에서 name element가 유일하지 않을 때
b) name의 내용이 문자데이터로만 이루어지지 않았을 때
c) 문서에서 XML namespace를 사용할 경우

XML은 문자열 연산을 하기에 너무 복잡하다. Xereces 같은 XML parser의 경우 jar 파일이 1 MB 가 넘는데는 다 그만한 이유가 있는 것이다.

- more professional code
SAXBuilder builder = new SAXBuilder(false);
Document doc = doc = builder.build(new StringReader(xml));
String name = doc.getRootElement().getChild("name").getText();

# The XML encoding trap

- very bad code
String xml = FileUtils.readTextFile("my.xml");

XML 파일의 내용을 읽어서 String class에 저장하는 것은 멍청한 짓이다.
XML 파일은 XML header에 해당 내용에 대한 encoding을 지정하고 있다.
XML 파일을 읽기 전에 해당 파일에 대한 encoding 정보를 알아야 한다.
또한, XML 파일의 내용을 String class에 저장하는 것은 메모리 낭비다.
XML parser들은 XML 파일을 parsing하는데 InputStream을 이용하여 내용을 읽어 들이고, encoding이 올바로 되었는지 확인한다.

# Undefined encoding

- not portable code
Reader r = new FileReader(file);
Writer w = new FileWriter(file);
Reader r = new InputStreamReader(inputStream);
Writer w = new OutputStreamWriter(outputStream);
String s = new String(byteArray); // byteArray is a byte[]
byte[] a = string.getBytes();

위 코드들은 각각 기본 platform encoding 정보를 사용하여 byte와 char를 변환하고 있다.
위 코드들은 어떤 platform에서 실행되느냐에 따라서 다른 결과를 초래한다.
이는 다른 platform 간의 데이터 전송에 있어서 문제를 유발할 수 있다.
platform의 기본 encoding 정보를 이용하는 것은 나쁜 습관이다.
변환은 encoding 정보를 지정하여 수행되어야 한다.

- portable code
Reader r = new InputStreamReader(new FileInputStream(file), "ISO-8859-1");
Writer w = new OutputStreamWriter(new FileOutputStream(file), "ISO-8859-1");
Reader r = new InputStreamReader(inputStream, "UTF-8");
Writer w = new OutputStreamWriter(outputStream, "UTF-8");
String s = new String(byteArray, "ASCII");
byte[] a = string.getBytes("ASCII");

# Unbuffered streams

- performance sinking
InputStream in = new FileInputStream(file);
int b;
while ((b = in.read()) != -1) {
   ...
}

위 코드는 파일을 byte 단위로 읽는다.
read() method가 호출 될때마다 매번 JNI call이 일어난다.
JNI call은 비용이 비싸다. native call의 횟수를 stream을 warpping한 BufferedInputStream을 사용하여 획기적으로 줄일 수 있다.
노트북에서 /dev/zero 로 부터 1MB의 데이터를 읽어들이는데 위 코드는 1초가 걸린 반면 BufferedInputStream을 사용한 경우는 
60 ms로 줄었음을 확인할 수 있다. 자그만치 94%의 속도 증가이다. 이 stream wrapping 은 output stream에 대해서도 적용이 가능하며,
file system 에서 뿐만 아니라 socket 에 대해서도 적용가능하다.

- reasonable performance
InputStream in = new BufferedInputStream(new FileInputStream(file));

# Infinite heap

- resource waster
byte[] pdf = toPdf(file);

파일의 내용을 읽어서 byte array로 반환하여 PDF 파일을 만드는 method가 있다.
이 코드는 파일의 내용이 적을 경우에만 유효하다. Heap에 파일 크기만한 여유가 있을 경우에만 가능하다는 얘기다.
그렇지 않을 경우 OOE(OutOfMemoryException)에 직면하게 된다.
Bulk 데이터는 절대로 byte array로 취급되면 안된다. Stream을 이용해야 하고 데이터는 DB나 disk에 저장되어야 한다.

- conservative code
File pdf = toPdf(file);

# Catch all: I don't know the right runtime exception

- wrong code
Query q = ...
Person p;
try {
    p = (Person) q.getSingleResult();
} catch(Exception e) {
    p = null;
}

이는 J2EE EJB3의 query 이다. getSingleResult() method는 아래와 같은 경우에 RuntimeException을 야기한다.
a) 결과가 유일하지 않을 때
b) 결과가 없을 때
c) query가 DB 오류등으로 인해 실행되지 못했을 때

위 코드는 모든 exception을 잡을 수 있다. 전형적인 catch-all 블럭이다.
null 이라는 결과를 사용하는 것은 b) 의 경우에는 유효할 지 모르나, a)나 c)의 경우에는 그렇지 않다.
일반적으로 필요한 exception 이외에는 더 잡을 필요도 없다.

- correct code
Query q = ...
Person p;
try {
    p = (Person) q.getSingleResult();
} catch(NoResultException e) {
    p = null;
}

# Exceptions are annoying

- hard to debug code
try {
    doStuff();
} catch(Exception e) {
    log.fatal("Could not do stuff");
}
doMoreStuff();

위 코드에는 두가지 문제점이 있다.
첫째, 이 코드에서 정말로 치명적인 오류가 발생한다면, 이를 호출한 쪽에 알리고, 적당한 exception을 발생시켜야 한다.
그렇지 않으면, 치명적인 오류 이후에도 동작을 계속하게 된다.
둘째, 문제가 생겼을 경우에 exception에 대한 정보가 사라지기 때문에 debug 하기가 힘들다.
exception 객체는 문제가 생기면 어디서 어떤 문제가 생겼는지 상세한 정보들을 담고 있다.
exception이 발생한다면 적어도 오류 메세지와 stack trace 정도는 log에 남겨야 한다.

- better code
try {
    doStuff();
} catch(Exception e) {
    throw new MyRuntimeException("Could not do stuff because: "+ e.getMessage, e);
}

# Catching to log

- stupid code
try {
    ...
} catch(ExceptionA e) {
    log.error(e.getMessage(), e);
    throw e;
} catch(ExceptionB e) {
    log.error(e.getMessage(), e);
    throw e;
}

위 코드를 보면 catch 절에서 단지 log만 남기고 원래 exception을 throw 하는 것을 볼 수 있다.
이는 멍청한 짓이다. 이 코드가 속한 method를 호출한 곳에서 해당 exception에 대해서 처리하도록(log를 남기던지 등) 
try/cat 전체를 옮기는 것이 낫겠다.

# Incomplete exception handling

- this instable code can leak file handles
try {
    is = new FileInputStream(inFile);
    os = new FileOutputStream(outFile);
} finally {
    try {
        is.close();
        os.close();
    } catch(IOException e) {
        /* we can't do anything */
    }
}

stream이 close 되지 않으면, 해당 OS 에서는 사용했던 resource를 해제할 수 없게 된다.
이 코드에서는 stream 두개를 닫기 위해서 finally 절에 close 를 넣었는데,
만약에 is.close()에서 IOException이 발생하면 os.close()는 수행되지 않게 된다.
두개의 close 는 각각 try/cat 절로 쌓여야 한다.
게다가, input stream을 생성하다가 exception이 발생하면 os 가 null이 되므로 os.close() 에서는 NullPointerException이 발생할 것이다.

- stable codes that frees all resources
try {
    is = new FileInputStream(inFile);
    os = new FileOutputStream(outFile);
} finally {
    try { if (is != null) is.close(); } catch(IOException e) {/* we can't do anything */}
    try { if (os != null) os.close(); } catch(IOException e) {/* we can't do anything */}
}

# The exception that never happens

- bad code
try {
  ... do risky stuff ...
} catch(SomeException e) {
  // never happens
}
... do some more ...

위 코드에서 개발자는 귀차니즘으로 인해 이 코드를 호출한 곳으로 어떤 exception을 다시 던지기를 원치 않는다.
게다가 exception이 절대 발생하지 않을 거라는 자만심에 빈 catch 절에 주석까지 달아놓았다.
하지만, exception이 절대 발생하지 않는다고 어떻게 장담할 수 있을까?
호출하는 method의 내용이 변경되기라도 한다면, 특정 경우에 exception이 발생하는데도 그런 경우에 대해서 생각을 못하고 있다면 어떨까?
만약 문제가 생기면 try/catch 이후의 코드들이 수행이 되면서 잘못된 결과를 초래할 것이다.  exception은 절대로 잡을 수 없게 될 것이다.
위 코드는 runtime exception을 던짐으로해서 보다 안정적이 될 필요가 있다.
그럼으로써 assertion의 역할도 하고 "crash early" 원칙을 따르는 것이기도 하다.
개발자는 모든 경우에 대해서 생각해야 하고, 작성한 코드가 완벽하다는 가정은 애시당초 틀렸다는 것을 가정해야 하며,
오류가 발생한 지점의 try/catch 이후의 코드는 실행되지 않도록 해야 한다. 
이렇게 처리한 후에 exception이 나지 않으면 그만이고, exeption이 발생하면 다행인 것이다.

- more reliable code
try {
  ... do risky stuff ...
} catch(SomeException e) {
  // never happens hopefully
  throw new IllegalStateException(e.getMessage(), e); // crash early, passing all information
}
... do some more ...

# The transient trap

- wrong code
public class A implements Serializable {
    private String someState;
    private transient Log log = LogFactory.getLog(getClass());
    
    public void f() {
        log.debug("enter f");
        ...
    }
}

Log 객체는 직렬화되지 않았다. 개발자도 이를 알고 직렬화되지 않도록 log 필드를 transient로 정의했다.
그러나, 이 변수의 초기화는 class의 initializer에서 이루어진다.
역직렬화시에는 initializers와 constructors는 수행이 되지 않는다.
역직렬화된 객체에는 log 변수가 null로 되어 있고, f() method에서는 NullPointerException을 야기할 것이다.
transient 변수는 절대 class 초기화에 사용하지 말아야 하며, 이 문제는 static 변수나 로컬 변수로 사용함으로써 해결할 수 있다.

- correct code
public class A implements Serializable {
    private String someState;
    private static Log log = LogFactory.getLog(getClass());
    
    public void f() {
        log.debug("enter f");
        ...
    }
}

public class A implements Serializable {
    private String someState;
    
    public void f() {
        Log log = LogFactory.getLog(getClass());
        log.debug("enter f");
        ...
    }
}

# Overkill initialization

- overkill
public class B {
    private int count = 0;
    private String name = null;
    private boolean important = false;
}

위 코드를 작성한 사람은 C로 프로그래밍하는데 익숙한 사람이어서, 모든 변수가 적절한 값으로 초기화 되기를 바란다.
Java 에서는 member 변수의 경우에 0, null, false 등으로 자동으로 초기화가 된다.
따라서 위 코드는 아래와 깉이 고치는 것이 좋다.

- faster and slicker and still the same
public class B {
    private int count;
    private String name;
    private boolean important;
}

# Too much static

- prevents class collection
private static Log LOG = LogFactory.getLog(MyClass.class);
많은 개발자들은 Log instance를 static 멤버 변수로 사용하는데 이는 디자인 관점에서 보면 맞는다.
하지만, 이는 절대 그럴 필요가 없다.
LogFactory class는 이미 Log 객체에 대한 static 참조를 갖고 있어서 getLog() 를 호출하면 hash table에서 찾게 되고, 비용도 비싸지 않다.
하지만, 모든 class 들이 static 멤버 변수를 갖도록 하는 것은 class들에 대한 gc를 방해하기 때문에 몹쓸 짓이다.
단지, 드물게 위에 위에 설명했던 직렬화 class 같은 경우에는 static 변수를 사용하는 것이 방법이다.
이는 클래스명을 찾기 위해 getClass()를 사용할 수 없게 만든다. 
대신에 클래스명을 하드코딩 해야만 하고, 로그 개체를 상속 클래스와 공유할 수 없어서 이를 private으로 선언해야만 한다.

- lets the class unload when not used
protected Log log = LogFactory.getLog(getClass());

# Chosing the wrong class loader

- rarely uses the right class loader
Class clazz = Class.forName(name);

위 코드는 현재 class를 load하는 class loader를 사용한다.
이것은 다른 class를 동적으로 load 하는데 맘대로 되지 않는다.
Servlet engine, Java Webstart 또는 Application server 같은 환경에서는 확실히 잘못된 것이다.
이 코드는 실행되는 환경에 따라 다르게 동작한다.
컨텍스트 클래스 로더를 사용하는 환경에서는 어플리케이션이 자신의 클래스를 로드하기 위해 사용해야 하는 클래스 로더를 제공한다.

- mostly uses the right class loader
ClassLoader cl = Thread.currentThread().getContextClassLoader();
if (cl == null) cl = getClass().getClassLoader(); // fallback
Class clazz = cl.loadClass(name);

# Poor use of reflection

- dangerous
Class beanClass = ...
if (beanClass.newInstance() instanceof TestBean) ...

이 개발자는 reflection API와 싸움중이다.
상속 여부에 대해서 체크하는 방법을 알아야 되는데 아직 방법을 찾지 못했다.
그래서 새로운 instance를 하나 생성하고 instanceof 연산자를 사용하곤 했다.
class의 instance를 생성하는 것이 얼마나 위험한지 모른다.
이 class가 뭐하는 class 인지도 모른다. 객체 생성 비용이 비쌀 수도 있다. 기본 생성자가 없을 수도 있다.
exception을 던지기라도 한다면.
이를 해결하는 방법은 Class.isAssignableFro(Class) method를 사용하는 것이다.
이것은 instanceof의 역방향이다.

- correct
Class beanClass = ...
if (TestBean.class.isAssignableFrom(beanClass)) ...

 

# Synchronization overkill

- synchronization overkill
Collection l = new Vector();
for (...) {
   l.add(object);
}

Vector는 동기화된 ArrayList이고, Hashtable도 동기화된 HashMap 이다.
두 class들은 객체를 동시에 접근하는 일이 있을 경우에만 사용 되어져야 한다.
동기화가 필요없는 지역 임시 변수에 이것들을 사용하는 것은 오버이며, 성능을 꽤 떨어뜨릴 수도 있다.

- no synchronization
Collection l = new ArrayList();
for (...) {
   l.add(object);
}

# Wrong list type

- 초보 개발자들은 적절한 list 유형을 결정하는데 어려움을 겪는다.
그들은 보통 Vector, ArrayList, LinkedList 중에 아무거나 골라 사용한다.
하지만, 성능 향상에 대한 문제를 고민해 봐야 한다.
하지만, 각 classe들의 구현은 adding, iterating, accessing 에 따라서 상당히 다르다.
아래 간략하게 표로 정리했는데, 이 표에서 Vector는 동기화 때문에 약간 느린거 말고는 ArrayList와 비슷하기때문에 생략했다.

                                   ArrayList                LinkedList 
add (append)        O(1) or ~O(log(n))             O(1) 
insert (middle)       O(n) or ~O(n*log(n))         O(1) 
iterate                 O(n)                                 O(n) 
get by index         O(1)                                 O(n)

linked list가 insert 작업에 대한 성능이 지속적인 반면, list entry마다 보다 많은 메모리를 사용하고 있다.
ArrayList의 insert 성능은 초기화 사이즈가 적당한지, insert 도중에 size가 늘어나는지에 따라 좌우된다.
확장은 2의 지수 형식으로 늘어나며, O(log(n))의 비용이 든다.
지수 형태의 확장은 정말 필요한 양보다 많은 양의 메모리를 사용하게 된다.
리스트 크기의 재설정은 또한 응답시간을 느리게 만들고 리스트가 크다면 major 가비지 컬렉션을 일으킬 수도 있다. 
list에서 Iterator를 사용하는 것은 그리 큰 비용이 들지 않지만, index를 사용하는 것은 아주 느리다.

# I love arrays

/**
 * @returns [1]: Location, [2]: Customer, [3]: Incident
 */
Object[] getDetails(int id) {...

문서화되었다 하더라도, 이런 종류의 메소드의 값 반환은 다루기 난처하고 에러를 쉽게 유발한다. 
반드시 이 값들을 담는 작은 클래스를 선언해서 사용해야 한다. 이는 C의 struct와 비슷하다.

Details getDetails(int id) {...}

private class Details {
    public Location location;
    public Customer customer;
    public Incident incident;
}

# Premature object decomposition

public void notify(Person p) {
    ...
    sendMail(p.getName(), p.getFirstName(), p.getEmail());
    ...
}
class PhoneBook {
    String lookup(String employeeId) {
        Employee emp = ...
        return emp.getPhone();
    }
}

첫번째 예제에서 단지 method에 상태를 전달하기 위해 객체를 분해하는 건 참 쓰잘데기 없는 일이다.
두번째 에제에서 이 method의 사용은 매우 제한적이다.
object 자체를 넘기도록 디자인되어야 한다.

public void notify(Person p) {
    ...
    sendMail(p);
    ...
}
class EmployeeDirectory {
    Employee lookup(String employeeId) {
        Employee emp = ...
        return emp;
    }
}

 

# Modifying setters

private String name;

public void setName(String name) {
    this.name = name.trim();
}

public void String getName() {
    return this.name;
}

이 개발자는 사용자가 입력한 이름 값의 앞/뒤 빈 공백에 대해서 처리를 하기 위해서
bean 의 setter method에서 공백들을 없애는 처리를 했다.
하지만, bean은 단지 값을 저장하기 위한 공간이지 Business Logic이 아니다.
bean 에서는 데이터를 변경하면 안된다. 값을 변경하려면 값이 입력되는 곳에서 차단하거나, 빈 공백을 원하지 않는 곳에서 처리해야 한다.

person.setName(textInput.getText().trim());

# Unnecessary Calendar

Calendar cal = new GregorianCalender(TimeZone.getTimeZone("Europe/Zurich"));
cal.setTime(date);
cal.add(Calendar.HOUR_OF_DAY, 8);
date = cal.getTime();

date, time, calendar, time zone에 대한 혼동이 있는 개발자들의 의한 전형적인 실수이다.
Date에 8시간을 더하기 위해 Calendar는 필요 없다. time zone도 상관이 없다.
(Think about is if you don't understand this!) However if we wanted to add days (not hours) we would need a Calendar, 
하지만, 시간이 아니라 날짜를 더하기 위해서는 Calendar가 필요하다.
because we don't know the length of a day for sure (on DST change days may have 23 or 25 hours). 
왜냐하면, 우리는 정확히 하루의 길이가 어떻게 되는지 모르기 때문이다.(DST- Daylight-Saving Time- 상에서는 23시간이 되기도 하고 25시간이 되기도 한다)

date = new Date(date.getTime() + 8L * 3600L * 1000L); // add 8 hrs

# Relying on the default TimeZone

Calendar cal = new GregorianCalendar();
cal.setTime(date);
cal.set(Calendar.HOUR_OF_DAY, 0);
cal.set(Calendar.MINUTE, 0);
cal.set(Calendar.SECOND, 0);
Date startOfDay = cal.getTime();

개발자는 하루의 시작( 0시 0분)을 계산하고 싶어한다. 이 개발자는 Calendar의 밀리초를 놓쳤다.
하지만, 진짜 큰 실수는 Calendar 객체에 TimeZone을 설정하지 않은 것이다.
TimeZone을 설정하지 않으면 Calendar는 기본 time zone을 사용한다.
이는 Desktop 프로그램에서는 괜찮을지 모르나, server-side 프로그램에서는 원하는 결과가 나오지 않을 수도 있다.
상하이에서의 0시 0분은 런던에서의 0시 0분과 전혀 다른 시간이다.
개발자는 날짜 계산을위해 타당한 time zone을 사용하는지 확인해볼 필요가 있다.

Calendar cal = new GregorianCalendar(user.getTimeZone());
cal.setTime(date);
cal.set(Calendar.HOUR_OF_DAY, 0);
cal.set(Calendar.MINUTE, 0);
cal.set(Calendar.SECOND, 0);
cal.set(Calendar.MILLISECOND, 0);
Date startOfDay = cal.getTime();

# Time zone "conversion"

- stupid
public static Date convertTz(Date date, TimeZone tz) {
  Calendar cal = Calendar.getInstance();
  cal.setTimeZone(TimeZone.getTimeZone("UTC"));
  cal.setTime(date);
  cal.setTimeZone(tz);
  return cal.getTime();
}

만약 위 코드가 뭔가 쓸모있다고 생각한다면 가서 article about time(http://www.odi.ch/prog/design/datetime.php)을 다시 읽어보라.
이 개발자는 위 기사를 읽지 않았고, 그의 날짜의 time zone을 수정하려고 노력하고 있다.
사실 그 method는 아무 일도 안한다. 반환된 Date는 parameter로 넘겼단 값과 다르지 않을 것이다.
왜냐하면, Date는 time zone에 대한 정보를 갖고 있지 않기 때문이다.
항상 UTC를 따른다. 그리고, Calendar의 getTime()/setTime() method는 항상 UTC와 실제 time zone 사이의 변환을 한다.

# Calling Date.setTime()

- not recommended
account.changePassword(oldPass, newPass);
Date lastmod = account.getLastModified();
lastmod.setTime(System.currentTimeMillis());

위 코드는 account entity의 최근 수정 날짜를 변경하고 있다.
프로그래머는 보수적이고, 새로운 Date 객체를 생성하는 것을 피하고 있다.
대신에 존재하는 Date instance를 수정하기 위해 setTime() method를 사용한다.

사실 위 코드에 잘못된 것은 없다. 하지만, 추천하고 싶지는 않다.
Date 객체는 일반적으로 조심스럽게 전달된다. 같은 Date instance가 setter에서 copy를 하지 않는 여러개의 객체들에 전달이 될 수 있다.
Date는 종종 원시형 처럼 사용된다. 그래서, Date instance를 수정하면, 이 instance를 사용하는 다른 객체에서 기대하지 않은 일이 발생할 수 있다.
물론, 프로그래머가 OO 원칙에 입각한 코드를 작성한다면, 객체가 그 자신의 본질적인 Date instance를 바깥 세상에 노출시키는 것이 깨끗한 디자인은 아니다.
대부분의 Java 코드에서 그들의 setter 에서 복제하지 않고, 단지 Date 참조를 복사하고 있다.
모든 프로그래머는 Date 를  불변(immutable) 로 취급해서, 존재하는 instance에 수정을 가하면 안된다.
이는 오직 성능 향상의 이유로만 이루어져야 한다. 한편으로는 간단한 long의 사용이 좋을 수도 있다.

- real world code
account.changePassword(oldPass, newPass);
account.setLastModified(new Date());

# Not noticing overflows

- overflow
public int getFileSize(File f) {
  long l = f.length();
  return (int) l;
}

이 개발자는 무슨 이유에서인지 파일의 크기를 반환하는 method에서 반환값을 long 대신에 int를 사용했다.
이 코드는 파일의 크기가 2GB를 넘을 경우 제대로 작동하지 않는다.
작은 값으로의 cast일 경우에는 반드시 overflow에 대해서 체크하고 exception을 던지도록 해야 한다.

- no overflow
public int getFileSize(File f) {
  long l = f.length();
  if (l > Integer.MAX_VALUE) throw new IllegalStateException("int overflow");
  return (int) l;
}

다른 버전의 overflow 버그는 아래와 같다.

long a = System.currentTimeMillis();
long b = a + 100;
System.out.println((int) b-a);
System.out.println((int) (b-a));

# Storing money in floating point variables

- broken sum
float total = 0.0f;
for (OrderLine line : lines) {
  total += line.price * line.count;
}

많은 개발자들이 위와 같이 loop을 코딩한다.
하나에 0.3$ 짜리 물건을 100개 계산한다고 하면, total 에 계산되어지는 값은 정확히 29.999971 일 것이다.
개발자들은 이 이상한 동작에 대해서 알아채고, 보다 정확한 double로 변경하여 30.000001192092896 값을 얻는다.
이런 결과는 사람과 컴퓨터가 숫자에 대해서 인식하는 방법의 차이 때문이다.
이는 소숫점자리의 돈 계산시에 가장 귀찮은 형태로 항상 발생한다.
따라서, 돈 같은 경우는 계산이 필요 없는 경우라면 text 로 저장되어야 한다.
계산시에 text 표현을 integer로 안전하게 변환해야 한다.
Double.parseDouble(amount) * 100 사용에 주의하라. 
차라리 String의 소숫점 자리에 0을 붙이거나, 소숫점을 없애고, Integer.parseInt를 이용해서 integer로 변환하라.
덧붙일 길이는 정밀도 요구에 따라 달라진다.

- correct sum
int totalcents = 0;
MoneyUtils money = new MoneyUtils(2);
for (OrderLine line : lines) {
  totalcents += money.toCents(line.priceString) * line.count;
}
String total = money.fromCents(totalcents);

public class MoneyUtils {
  private int precision;
  private String zero;
  
  public MoneyUtils(int precision) {
      this.precision = precision;
      char[] z = new char[precision];
      for (int i=0; i<z.length; i++) z[i] = '0';
      zero = new String(z);
  }
  
  public int toCents(String amount) {
      String[] parts = amount.split("\\.");
      String padded;
      switch (parts.length) {
          case 1:
              padded = parts[0] + zero.substring(0, precision);
              break;
         
          case 2:
              int end = precision - parts[1].length();
              if (end < 0) throw new IllegalArgumentException("precision not enough");
              padded = parts[0] + parts[1] + zero.substring(0, end);
              break;
              
          default: throw new IllegalArgumentException("too many dots");
      }
      return Integer.parseInt(padded);
  }
  
  public String fromCents(int cents) {
      String amount = String.valueOf(cents);
      return amount.substring(0, amount.length() - precision) 
             +"."+ amount.substring(amount.length() - precision, amount.length());
  }
}

# Abusing finalize()

- abusive code
public class FileBackedCache {
   private File backingStore;
   
   ...
   
   protected void finalize() throws IOException {
      if (backingStore != null) {
        backingStore.close();
        backingStore = null;
      }
   }
}

이 class에서는 file handle을 해제하기 위해서 finalize() method를 사용했다.
문제는 이 method가 언제 호출이 되는지 모른다는 것이다.
이 method는 garbage collector에 의해서 호출이 된다.
만약에 file handler을 다 쓰고 난 후에 이 method가 호출되기를 원할것이다
하지만, 이 method는 GC가 프로그램이 heap을 다 사용했을 경우에 호출하게 되어 있다. 
garbage collector는 단지 메모리를 관리한다. 하지만, 메모리 관리 이외의, 다른 resource를 관리하도록 남용되어서는 안된다.

- proper code
public class FileBackedCache {
   private File backingStore;
   
   ...
   
   public void close() throws IOException {
      if (backingStore != null) {
        backingStore.close();
        backingStore = null;
      }
   }
}