팀원의 코드 리뷰를 하던 중 코드가 잘 이해가 가지 않아서 몇몇 부분을 method로 추출하자고 했더니 그냥 주석을 대충 달면 충분하다고 했다. 마침 며칠 전 Clean Code의 주석 섹션에서 비슷한 내용을 읽은 게 기억났지만 고집이 센 친구라 그냥 넘어갔다.
차이점이 보이는가? 각 if 컨디션마다 주석을 추가했다. 일단 각 if 컨디션이 무엇을 하고자 하는지 이해는 할 수 있다. 대신 관리해야 하는 라인이 두줄이 늘었다. 내 의문은 과연 이게 최선의 방법일까, 후에 다른 사람이 이 코드를 수정할 때 주석까지 함께 꼼꼼히 신경 쓸 것인가였다. 앞서 말한 대로 거의 대부분의 주석이 도태되는 상황이 발생할 것이다. 아마 코드는 수정을 안 하면 문제가 있지만 주석은 수정을 안 해도 기능상 문제가 없기 때문일 거다.
그저 두 개의 method만 새로 만들었을 뿐인데 각 컨디션의 목적이 명확해졌다. recursive의 정확한 목적 표시를 위해 // try again 이라는 새로운 주석을 달았는데, 무조건 주석을 달지 말자 라기보다는 코드를 보고 두세 번 생각하게 만든다면 리팩토링을 하고, 리팩토링으로 해결되지 않는 거라면 정확한 의미를 담은 주석을 달면 된다. 코드의 가독성은 정말 중요하다.
그리고 코드와 전혀 상관없는 쓸데없는 주석은 달지말자.
Clean Code에서 읽은 내용은 주석은 시간이 지남에 따라 코드만큼 관리가 되지 않기 때문에 쓰레기가 될 가능성이 높다. 그러니 특별한 경우가 아니라면 코드를 리팩토링 하여 가독성을 높이는 것에 집중하는 것이 더 효율적이라는거다.
수정 전 코드
public Person getPersonData() {
JsonObject response = queryPerson();
if (!response.has("ok") || !response.get("ok").getAsBoolean()) {
if (response.has("error") && response.get("error").getAsString().equals("bad request")) {
return getPersonData();
}
return new Person("No name");
}
return new Person(response.get("name").getAsString());
}
여러 문제가 있지만 너무 까탈스럽게 하고싶지 않아서 if 컨디션들의 가독성만이라도 높이자고 제안 했더니 그 자리에서 아래와 같이 수정했다.
주석이 추가된 코드
public Person getPersonData() {
JsonObject response = queryPerson();
// it's an error
if (!response.has("ok") || !response.get("ok").getAsBoolean()) {
// it's a bad request
if (response.has("error") && response.get("error").getAsString().equals("bad request")) {
return getPersonData();
}
return new Person("No name");
}
return new Person(response.get("name").getAsString());
}
차이점이 보이는가? 각 if 컨디션마다 주석을 추가했다. 일단 각 if 컨디션이 무엇을 하고자 하는지 이해는 할 수 있다. 대신 관리해야 하는 라인이 두줄이 늘었다. 내 의문은 과연 이게 최선의 방법일까, 후에 다른 사람이 이 코드를 수정할 때 주석까지 함께 꼼꼼히 신경 쓸 것인가였다. 앞서 말한 대로 거의 대부분의 주석이 도태되는 상황이 발생할 것이다. 아마 코드는 수정을 안 하면 문제가 있지만 주석은 수정을 안 해도 기능상 문제가 없기 때문일 거다.
리팩토링을 통한 가독성을 높인 버전
public Person getPersonData() {
JsonObject response = queryPerson();
if (hasAnyErrors(response)) {
if (isBadRequest(response)) {
return getPersonData(); // try again
}
return new Person("No name");
}
return new Person(response.get("name").getAsString());
}
private boolean hasAnyErrors(JsonObject response) {
return !response.has("ok") || !response.get("ok").getAsBoolean();
}
private boolean isBadRequest(JsonObject response) {
return response.has("error") && response.get("error").getAsString().equals("bad request");
}
그저 두 개의 method만 새로 만들었을 뿐인데 각 컨디션의 목적이 명확해졌다. recursive의 정확한 목적 표시를 위해 // try again 이라는 새로운 주석을 달았는데, 무조건 주석을 달지 말자 라기보다는 코드를 보고 두세 번 생각하게 만든다면 리팩토링을 하고, 리팩토링으로 해결되지 않는 거라면 정확한 의미를 담은 주석을 달면 된다. 코드의 가독성은 정말 중요하다.
그리고 코드와 전혀 상관없는 쓸데없는 주석은 달지말자.
예를 들면 이런거
// 누가 만들었는지 모르겠는데 진짜 못 만들었다. 집에 가고 싶다.
private void someMethod() {
....
}