코드리뷰 정리

 입사 2달만에... 첫 개발과제를 받고 오늘 팀장님께 코드리뷰를 받았다. 팀장님이 하신 말씀 중에 상처가 되는 말도 있었지만, 코드에 대해서 지적하신 내용은 내가 생각해도 팀장님의 말씀이 맞다고 생각했다. 그래서 까먹고 그냥 흘려버리기 전에 코드리뷰 받은 내용을 정리해둔다.

---

1. 객체 자체의 null 체크 잊지 말기

 - 객체를 만들어서 객체의 변수로 null 체크는 하지만 객체 자체의 null 체크를 안 하고 있던 부분이 있었다.

 

2. 메소드에서 파라미터로 받은 객체 자체의 값을 변경해서 돌려주지 말기

 - 굳이 그런 로직으로 써야한다면 새로 객체를 만들어서 그 객체를 돌려주도록 한다.

 

3. 변화가 없는 경우, 상수로 빼기

 - 요청일시포맷이 정해져있는 경우 이런 포맷을 상수로 빼내는 것이 좋다.

 

4. API에 Json 형식으로 데이터를 넘겨주고, 그 데이터를 받을 때 직렬화(Serializable)을 사용하기

 - Json형식으로 데이터를 받아서 데이터를 처리해야 했는데, 이때 나는 @RequestBody로 하여 받았다. 그런데 입력데이터의 특성상 Json의 깊이가 1단계가 아니라 2단계였다.(이 표현이 맞는지 모르겠지만.) 예를 들자면 아래와 같다.

{
  "clientKey": "클라이언트키",
  "requiredDateTime": "요청일시",
  "data": [
  	{
      "userId": "유저아이디",
      "gender": "성별"
    }
  ]
}

 - 이럴 때 그냥 아래와 같이 객체를 만들어서 받으면 안된다. 왜냐하면 Map에서 꺼내서 쓰다 보면 오타가 발생할 위험도 있고, 맵에 있는 데이터를 검증하기 번거로워지기 때문이다. 그냥 객체를 만들어서 객체에 검증 메소드를 만드는 것이 낫지.

# 잘못된 예

public class RequestUserDTO {

    private String clientKey;
    private String requiredDateTime;
    private Map<String, String> data;

}

# 이렇게 해야 한다.

public class RequestUserDTO {

    private String clientKey;
    private String requiredDateTime;
    private DataDTO data;

}

public class DataDTO {

	private String userId;
    private String gender;

}

 

5. 어떤 코드를 가져다가 쓸 때는 성능테스트를 해보고 사용하기

 - 써있는대로 가져다가 쓰지 말고, 테스트해보면서 성능면에서 부하를 주지 않는지, 메모리나 다른 문제는 없는지 어떻게 해야 효율적으로 사용할 수 있을지 생각해봐야 한다.

 

6. 두 가지 값만을 가지는 경우, boolean으로 처리하는 게 실수를 줄일 수 있다.

 - 예를 들어, 아래와 같은 코드가 있었다.

String continueYn = "Y";

 여기서 cotinueYn은 "Y" 혹은 "N" 값만 가질 수 있다. 이럴 경우, String이 아니라 boolean으로 처리하는 것이 실수를 줄일 수 있다.

 

7. 코드 중복은 최대한 줄인다.

 

8. 에러가 발생하는 경우에는 그 에러가 발생했다는 사실과 데이터가 있다면 그 데이터를 같이 출력해준다.

 - e.printStackTrace(); 는 사용하지 않고, 설정해놓은 로그 error로 출력한다.

 

9. try ~ catch 사용시 주의

- File I/O를 하는 과정에 try ~ catch를 사용했는데, 이 부분에서 지적을 받았다.

if (savePath != null) {

    File dir = new File(savePath);
    
    if(!dir.exists() || !dir.isDirectory()) {
        dir.mkdirs();
    }
    
    File saveFile = new File(savePath.concat(File.separator).concat(fileName)); // 이 부분 Path로 바꿀 것
    
    try (FileOutputStream fos = new FileOutputStream(saveFile)) {
        fos.write(pData);
        fos.flush(); // 버퍼 비우기
    } catch (IOException e) {
        // 에러내용을 로그로 남긴다.
    }
    
} else {

    // 저장 경로가 null일 때의 처리
    return false;
    
}

return true;

 - 위 코드에서 지적받아서 수정한 내용이 Try-with-resources이다. 위 코드처럼 try에 자원 객체를 전달하면 try 블록이 끝났을 때 자동으로 자원을 종료해주는 기능이다. finally에 자원 해제하는 기능을 넣는 건 알고 있었는데, 요즘은 이 방법이 권장된다고 한다. (팀장님피셜) 그리고 파일 저장경로를 만드는 과정에서 concat()을 사용하지 말고, Paths.get()를 사용하라고 하셨다.