코드는 여기 있습니다.
코드 리뷰?
글을 읽기 전에 말씀드리면 제대로된 코드 리뷰를 받지 못 했습니다. 제가 다니는 회사는 클린 코드, TDD, 객체지향 생활체조 등 유명한 개발 방법론에 대해 고민하지 않고 개발을 해왔습니다. “기능만 되면 된다”고 생각하시는 분들의 생각을 바꾸기 위해 스터디를 시작하게 되었고 서로 코드 리뷰를 어떻게 해줘야 할지 협의하고 있는 상황이라 많은 시간이 지나야 건전한 개발 문화가 정착될 것 같습니다.
그래도 조금이나마 리뷰 받은 내용과 제가 생각했을 때 잘못 작성되었다고 판단된 코드에 대해 기록해보려고 합니다.
tmi. 개인적으로 “기능만 되면 된다.”는 도메인을 빠르게 구축해야 하는 상황에서는 필요할 수 있다고 생각합니다. 하지만 반드시 보완이 되어야 될 것입니다. 제 경험적으로는 나중에 보완할 때는 오지 않는다고 생각되어 한꺼번 안정적인 코드를 작성하는 것이 더 좋을꺼 같습니다.
코드 컨벤션
상수는 private static final?
이전 포스트에 작성되어 있는 FormulaParser의 상수 선언부입니다.
public class FormulaParser {
private final List<Character> brackets = Arrays.asList('(', ')');
private final String numberReplaceRegex = "[^0-9.]"; // 숫자 치환을 위해 검사 정규식
private final String syntaxRegex = "[0-9|\\.|\\+|\\-|\\*|\\/|\\(|\\)]*$"; // 계산식 유효성 검사 정규식
private final String numberOfEndToEndRegex = "^\\d{1}[0-9|\\.|\\+|\\-|\\*|\\/|\\(|\\)]*\\d{1}$"; // 계산식의 앞뒤 문자 타입이 숫자 정규식
...
}구현할 때 제가 생각했던 것은
public으로 개방하지 않은 상수는 외부 클래스에서 사용할 수 없고
FormulaParser클래스 내부에서만 사용하는 데 굳이 static 키워드를 사용해야 할까?
라고 생각하였고 이 고민 때문에 상수처럼 사용하지만 private final 키워드만 사용하였습니다.
이 부분에 대해 스터디원분들과 얘기를 나눈 결과
FormulaParser의 객체를 생성하고 static 변수를 사용할 때 모든FormulaParser가 공유할 것이다.
라는 의견이 있어 확인해본 결과 static 선언 객체는 해당 클래스 내부에서 공유하여 사용하는 것으로 확인하였습니다.(static에 대해 겉핥기만 알고 있었다고 깨달았습니다…)
위와 같은 의견에 힘이 실리며 일단 static으로 변경하는 것으로 끝나는 줄 알았지만 또 다른 의견이 나왔습니다.
static 객체는 GC에서 관리하지 않고 프로그램이 종료되어야 메모리 해제되는데 과연 static이 필요한가?
의견의 내용은 타당하다고 생각합니다. static을 남발하게 되면 메모리 문제로 시스템에 악영향을 주게 될 것입니다. 결론이 나오지 않아 코드 리뷰 중 이 내용은 보류하게 되었습니다.
나중에 이에 대해 검색해본 결과 왜 자바에서 static을 지양해야 되는가? 에서는 static은 객체지향적이지 않고 대안 방안까지 자세하게 얘기하고 있고 개인적으로 고민해본 결과 다음과 같이 결론을 내렸습니다.
- 객체 사용빈도가 빈번하다면 (실시간으로 호출되는 로직에서 사용하는 경우) static으로 사용한다.
- 객체 사용빈도가 빈번하지 않다면 위에 글에서 말하는 것과 같이 transient나 volatile를 사용하는 것도 괜찮고 GC에서 수거 할 수 있도록 private final 객체로 사용한다.
일급 컬렉션
/**
* 치환된 숫자 정보를 담는 클래스
*/
public class Numbers extends HashMap<String, Double> {
private int index = 0;
/**
* 숫자 정보를 담고 해당 숫자의 key를 반환한다.
* @param stringNumber
* @return
*/
public String set(String stringNumber) {
if (stringNumber.length() == 0) {
return null;
}
String key = String.valueOf(index++);
this.put(key, Double.valueOf(stringNumber));
return key;
}
}Numbers는 계산식의 숫자를 치환한 정보를 가지는 클래스입니다. 기능만 생각했을 때 만약 계산식이 2+4/5라면 2, 4, 5는 Numbers에 저장하고 계산을 할 때 치환된 숫자를 이용하려 했습니다.
코드 리뷰 중 얘기는 하지 않았지만 저는 HashMap을 상속받는 방식의 구현으로 사용하였고 객체지향 생활체조에 언급된 일급 컬렉션으로 구글 검색 상위에 나와있는 일급 컬렉션의 소개와 써야할 이유에 대해 읽어보고 나서 다음과 같이 생각이 들어서 일급 컬렉션으로 변경하는 것이 좋다고 판단했습니다.
- Numbers가 외부에 제공하는 메서드는 put(), get() 메서드이니 다른 메서드를 일급 컬렉션 내부에서만 사용하고 외부에는 공개하지 않을 수 있다.
- Numbers의 값을 꺼내 사용하는 곳인
PostfixProcessor는 매개변수로 Map이 아닌 Numbers를 가지니 상속을 통한 방식이 아닌 일급 컬렉션이 적절하다.
책임 분리
이전 포스트에 작성된 FormulaParser 클래스 주석을 보면 다음과 같은 기능을 수행합니다.
/**
* 계산식을 변형하는 클래스
* - 숫자는 단일 문자열로 치환
* - 중위 표현식 -> 후위 표현식
*/
public class FormulaParser {
...이 클래스는 숫자 치환 기능과 중위 표현식을 후위 표현식으로 변경하는 두 가지 기능을 수행하고 있었습니다.
이 곳에 문제가 있다고 판단한 것은 중위 표현식을 후위 표현식으로 변경하는 기능을 테스트 하는 것은 문제가 없었는데 숫자 치환 기능을 테스트할 때 숫자 치환 후 계산식을 테스트 할 수 없는 문제가 있었습니다.
이 문제는 코드 리뷰 중 질문을 통해 두 가지 기능을 분리하여 사용하는 것으로 구현하는 것이 좋을 것 같다는 피드백을 받았습니다.
-
숫자 치환 기능 →
FormulaReplacerexpression과numbers두 가지를 테스트를 할 수 있는 구조가 되었습니다.
/** * 계산식을 변형하는 클래스 * - 숫자는 단일 문자열로 치환 */ public class FormulaReplacer { private static final String NUMBER_REPLACE_REGEX = "[^0-9.]"; // 숫자 치환을 위해 검사 정규식 private static final String SYNTAX_REGEX = "[0-9|\\.|\\+|\\-|\\*|\\/|\\(|\\)]*$"; // 계산식 유효성 검사 정규식 private String expression; // 입력 받은 중위 표현 계산식 private Numbers numbers; // 치환된 숫자를 가지는 context /** * 계산식 수행 클래스로 반환 * @return */ public void replace(String syntax) { validate(syntax); this.numbers = new Numbers(); this.expression = replaceNumbers(syntax); } /** * 유효성 검사 * @param syntax */ private void validate(String syntax) { if (!syntax.matches(SYNTAX_REGEX)) { throw new IllegalArgumentException("[ ERROR ] 계산식을 다시 입력해주세요."); } } /** * 계산식에서 숫자를 단일 문자로 치환 * @param syntax */ private String replaceNumbers(String syntax) { String expression = syntax; for (String stringNumber : syntax.split(NUMBER_REPLACE_REGEX)) { expression = replaceNumber(stringNumber, expression); } return expression; } /** * 계산식 치환 * @param stringNumber * @param expression */ private String replaceNumber(String stringNumber, String expression) { if (stringNumber.length() == 0) { return expression; } String key = this.numbers.set(stringNumber); return expression.replaceFirst(stringNumber, key); } public String getExpression() { return expression; } public Numbers getNumbers() { return numbers; } } -
중위 표현식을 후위 표현식으로 변경하는 기능 →
PostfixParser/** * 후위 표현식 변환 parser */ public class PostfixParser { private static final char START_PARENTHESES = '('; private static final char END_PARENTHESES = ')'; private static final List<Character> PARENTHESES = Arrays.asList(START_PARENTHESES, END_PARENTHESES); private StringBuilder stringBuilder; private Stack<Character> stack; public PostfixParser() { this.stringBuilder = new StringBuilder(); this.stack = new Stack<>(); } /** * 후위 표현식으로 변환 */ public String parse(String infix) { for (char character : infix.toCharArray()) { setCharacter(character); } while (!stack.isEmpty()) { stringBuilder.append(stack.pop()); } return stringBuilder.toString(); } /** * 단일 문자 설정 * @param character */ private void setCharacter(char character) { if (isOperators(character)) { setOperators(character); return; } stringBuilder.append(character); } /** * 후위 표현식 변환 중 계산식 우선순위에 따라 수식을 입력한다. * @param character */ private void setOperators(char character) { if (END_PARENTHESES == character) { setOperatorsWithParentheses(); return; } prioryFor(character); } /** * 수식 우선순위에 따라 문자를 배치한다. * @param character */ private void prioryFor(char character) { while (isHighPriory(character)) { stringBuilder.append(stack.pop()); } stack.push(character); } /** * 입력된 사칙연산 기호의 우선순위가 기존 사칙연산 기호보다 높은지 확인 * @param character * @return */ private boolean isHighPriory(char character) { if (stack.isEmpty()) return false; Operator arithmetic = Operator.symbolOf(character); if (arithmetic == null) { return false; } return arithmetic.prioryThen(stack.peek()); } /** * 사용자가 입력한 계산식 우선 순위에 따라 입력한다. */ private void setOperatorsWithParentheses() { while (isNotStartParentheses()) { stringBuilder.append(stack.pop()); } // 스택의 시작 괄호('(')를 제거한다. stack.pop(); } /** * 스택의 값이 시작 소괄호가 아닌 경우 true * @return */ private boolean isNotStartParentheses() { return !stack.isEmpty() && stack.peek() != START_PARENTHESES; } /** * 단일 문자가 수식 또는 소괄호인지 확인한다. * @param character * @return */ private boolean isOperators(char character) { return Operator.symbols().contains(character) || PARENTHESES.contains(character); } }
마무리
코드 리뷰 후에 피드백 받은 내용을 정리 해보았습니다. 물론 자잘한 리뷰도 많았지만 제가 중요하게 생각한 내용을 주로 다루어 보았습니다. 회사에서도 많은 분들이 함께하면 좋겠지만 하다 보면 많아 질 것이라 믿고 꾸준하게 진행되었으면 좋겠습니다.
제가 이번에 하면서 객체지향적으로 코드를 짜는 것이 무엇인지 많은 고민을 했지만 공략집 같은 것이 있었으면 좋겠다고 생각이 어마어마하게 들었습니다. 그래서 눈팅만 하던 넥스트 스텝의 자바 플레이그라운드 with TDD, 클린코드를 수강하였습니다. 슬랙 채널에서도 확인했지만 정말 많은 분들이 수강하고 있었습니다.(저만 늦게 도전한 것 같다는 생각이 들었어요….ㅠㅠ)
강의는 정말 좋은 내용이라고 생각하도 꾸준하게 미션을 수행하여 스펙업을 할 수 있는 좋은 기회라고 생각됩니다. (모든 미션을 완료하면 이것도 후기를 작성해보도록 하겠습니다.)
