TOC
-
- 메서드, 변수 등의 네이밍을 의미있는 네이밍으로 정하라
- 불필요한 객체 생성은 static을 통해 막을 수 있다.
- 제너릭을 잘 활용하자
- 변수 이름에 자료형을 사용하지 말자
- 코드를 작성할 때는 모든 상황을 고려하자
- package-private 지지자보다는 private를 사용하자
- getter, setter의 사용을 지양하자
- equals, hashCode, toString의 재정의
- 상수의 위치
- 개행에는
System.lineSeparator();를 사용하자 - 모든 변수와 메서드 명은 해당 클래스를 기준으로 이름을 정하자
- 도메인에서 재정의한 toString을 view에서 사용할 경우 view와 model의 분리가 깨지게 된다.
우아한테크코스의 첫번째 미션이 오늘로 끝이 납니다. 미션을 진행하며 저의 첫번째 리뷰어였던 미르에게 많은 피드백을 받았는데 받은 피드백 내용들을 앞으로도 잊지 않기 위해 정리를 해보고자 합니다.
앞으로도 미션이 끝날 때마다 미션 단위로 피드백 정리 및 회고록을 작성해볼 예정입니다.
그럼 지금부터 첫번째 미션인 자동차 경주의 피드백 정리를 시작하겠습니다.
피드백을 주고받은 PR
피드백 정리
메서드, 변수 등의 네이밍을 의미있는 네이밍으로 정하라
public class StringCalculator {
private static final int NUMBERS = 2;
private static final int CUSTOM_DISTRIBUTOR = 1;
public static int splitAndSum(String input) {
if (checkNullOrEmpty(input)) {
return 0;
}
Matcher matcher = Pattern.compile(CUSTOM_DISTRIBUTOR_PATTERN).matcher(input);
if (matcher.find()) {
checkInvalidInput(matcher.group(NUMBERS), matcher.group(CUSTOM_DISTRIBUTOR));
return getSum(matcher.group(NUMBERS), matcher.group(CUSTOM_DISTRIBUTOR));
}
}
}코드를 작성한 나의 경우에는 상수로 정의된 NUMBERS가 무엇을 뜻 하는지 알지만 다른 개발자들이 봤을 때 NUMBERS는
코드 내부를 확인하기 전에는 무엇을 뜻하는지 알 수가 없습니다.
보기 좋은 코드는 코드 작성자 뿐만 아니라 다른 개발자들도 쉽게 이해하는 코드라고 생각합니다. 이에 따라 위의 코드와 같은 NUMBERS와 같이
알아보기 어려운 네이밍은 좋지 않은 네이밍이라 변경되어야합니다.
그래서 저는 NUMBERS를 더 의미있게 NUMBER_GROUP으로 네이밍을 변경했습니다.
효과적인 네이밍을 짓는 방법을 잘 모르겠다면 효과적인 이름짓기 를 참고하면 좋습니다.
또한 변수명 짓기 사이트 도 존재하니 참고해보면 학습에 도움이 될 것입니다.
불필요한 객체 생성은 static을 통해 막을 수 있다.
Matcher matcher = Pattern.compile(CUSTOM_DISTRIBUTOR_PATTERN).matcher(input);위의 코드가 들어간 메서드가 프로그램에서 자주 사용되는 메서드라면 해당 메서드가 호출될 때마다 Pattern객체를
반복적으로 생성하게 될 것입니다. 예시와 같이 자주 호출되지만 호출될 때마다 같은 객체를 반복적으로 생성한다면
해당 객체는 static으로 선언하여 불필요한 반복을 줄이는 방법을 생각해봐야한다.
제너릭을 잘 활용하자
// as-is
ArrayList<Car> cars;
// to-be
List<Car> cars;다음과 같은 코드는 문제가 될 것은 없지만 유연성이 떨어집니다. Collection을 사용하는 경우 제너릭을 활용하여 코드의 유연성을 높이는 것이 좋습니다.
ArrayList
cars 가 아닌 List car 와 같이 사용한다면 범용성이 늘어난다. 개발을 하다보면 처음 생각하였던 것과 다르게 ArrayList가 아닌 LinkedList 와 같은 다른 List들을 사용하게 될 수도 있고 추후에 유지보수를 하며 변경을 하는 상황이 발생할 수 있는데 이러한 상황을 List로 사용할 경우 더 좋다.
변수 이름에 자료형을 사용하지 말자
final List<String> carNameList = input.inputValidNames();다음 코드를 보면 변수명에 List라는 자료형이 들어갑니다. 이런 변수명은 나중에 자료형을 변경해야한다면 변수 명도 변경해줘야하는 문제가 발생합니다. 개발자의 실수로 자료형 변경 후 변수명을 변경하지 않는 상황이 발생한다면 나중에 해당 코드의 의미를 오해하는 상황이 발생하게 되어 변수명에 자료형을 적기보다는 복수형을 사용하는 것이 좋습니다.
코드를 작성할 때는 모든 상황을 고려하자
private int findFarthestPosition(){
return cars.stream()
.sorted(Comparator.comparing(Car::getPosition))
.collect(Collectors.toList())
.get(cars.size()-1).getPosition();
}처음 해당 코드를 작성할 때는 작성한 프로그램 내에서는 cars.size()가 0일 상황이 없어서 위와 같이 작성했습니다.
하지만 누군가 해당 메서드를 사용할 때 0인 경우 에러가 발생할 가능성이 있지 않을까요?
라는 피드백을 받으며 곰곰히 생각해봤고 코드를 작성할 때는 최대한 많은 상황에 대해 방어적으로 대응하며 작성하는 것이 좋다고 생각하게 되었습니다.
package-private 지지자보다는 private를 사용하자
인스턴스 변수에는 package-private지지자보다는 private를 사용하여 객체의 접근을 막아주는 것이 좋습니다. 변수뿐만 아니라 내부적으로 사용되는 메서드 또한 private로 사용하는 것이 좋습니다!
getter, setter의 사용을 지양하자
public class RacingGame {
private List<Car> getWinners(final int farthestPosition){
return cars.stream()
.filter((car)->farthestPosition==car.getPosition())
...
}
}다음 코드의 경우 car.getPositoin()을 통해 자동차의 위치 정보를 가져와서 해당 메서드 내에서 확인을 합니다.
하지만 코드를 작성함에 있어 이와 같이 getter를 통해 객체의 내부 정보를 갖고 오는 것 보다는 객체에 메시지를 보내서 결과를 return받는 식으로 프로그래밍을 하는 것이 좋습니다.
위의 경우 변경을 한다면 아래의 코드와 같이 Car객체의 위치를 가져오기보다 Car객체 내부에 위치 비교 메서드를 생성하여 해당 메서드를 통해 객체에 메시지를 보내며 결과를 반환받는 것이 좋습니다.
public class Car {
...
public boolean isSamePosition(final int position) {
return this.position == position;
}
...
}
public class RacingGame {
private List<Car> getWinners(final int farthestPosition){
return cars.stream()
.filter((car)->isSamePositoiin(farthestPosition))
...
}
}Getter는 사용을 최대한 줄이는 것이 좋은 반면 Setter는 프로그래밍을 하며 사용을 아예 하지 않는 것이 좋습니다. Setter를 public으로 열어둘 경우 누군가 악의적으로 또는 실수로 setter를 통해 객체의 내부값을 변경할 수 있기에 이러한 상황을 예방하고자 setter는 사용하지 않는 것이 좋습니다. 만약 값을 초기화해야할 일이 있다면 생성자를 사용하도록 프로그래밍해야합니다.
equals, hashCode, toString의 재정의
https://github.com/woowacourse/java-racingcar/pull/326#discussion_r805075500
상수의 위치
상수의 위치는 개발자의 성향에 따라서 각각 사용되는 클래스 위에 정의를 하던가, 상수 클래스를 생성하여 모아서 관리 할 수 있습니다.
저도 해당 내용은 프리코스를 진행할 때부터 어떤 방법이 가독성이 더 좋고 실무에서 사용하는가 의문을 갖고 있었는데 리뷰어 미르는 "각각의 책임이 있다고 생각해서 별도 클래스를 두기보다는 관련 클래스에 모아서 사용할 것 같아요:)"라는 답변을 주셨습니다.
또한 실무에서도 공통으로 쓰는 팀도 있고, 각 클래스마다 정의하는 팀도 있다고하며 나만의 개발 철학을 만들어 상수 관리를 하면 된다고 하셨습니다.
저의 경우는 미르처럼 각각 사용되는 클래스 위에 정의하는 개발 스타일을 만들어보고자 합니다.
상수의 위치 뿐만 아니라 상수로 정의하는 값의 범위 또한 자신만의 개발 철학을 만들어 상수를 만드는 것이 좋다고 합니다.
개행에는 System.lineSeparator();를 사용하자
지금까지 프로그래밍을 하며 줄바꿈을 할 때는 일반적으로 \n을 사용했습니다.
하지만 이번 리뷰를 받으며 \n는 OS마다 개행이 달라질 수 있어 System.lineSeparator();를 사용하는 것을 추천한다는 리뷰를 받았습니다.
모든 변수와 메서드 명은 해당 클래스를 기준으로 이름을 정하자
public class Car {
...
public void goForward(int randomNumber) {
if (randomNumber >= FORWARD_STANDARD) {
position++;
}
}
...
}다음 코드를 보면 위의 goForward()메서드를 보면 randomNumber라는 매개변수를 받습니다.
하지만 Car 클래스의 입장에서 생각을 해보면 과연 goForward()는 값을 받아서 해당 값이 기준 값보다 큰지를 판단하여 위치를 이동하는 역할을 하는데
굳이 받는 값이 랜덤 값이라는 사실을 알아야 할까요??
Car입장에서는 랜덤 값이 아니라 그냥 number를 받는다는 사실만 알아도 됩니다.
그래서 위의 코드처럼 매개변수 이름을 randomNumber로 정의하기보다는 아래와 같이 number로 해주는 것이 좋습니다.
public class Car {
...
public void goForward(int number) {
if (number >= FORWARD_STANDARD) {
position++;
}
}
...
}도메인에서 재정의한 toString을 view에서 사용할 경우 view와 model의 분리가 깨지게 된다.
도매인에서 toString을 재정의한 것을 이용하여 view에서 사용한다면, 출력 형태가 변경될 경우 View에 관련된 메서드를 변경하는 것이 아니라 model부분의 코드를 수정해야하는 상황이 발생합니다. 이는 MVC패턴으로 프로그래밍을 할 때 model과 view의 관계가 분리된 것이 아님으로 toString은 view에서 사용하지않고 디버깅을 위해 사용을 해야합니다.
