[4기 - 최지연] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #166
Conversation
|
지연님이 궁금한점만 일단 빠르게 답변드릴께요~!
네 문자열을 다룰때 정규식을 잘 활용하시면 유효성 검증과 토큰화 과정을 짧은 코드로 줄일 수 있습니다.
보통 단일 책임 원칙이라고 해서 객체가 하나의 책임만을 가질 수 있게 분리하라고 하는데요
적절한 클래스 개수는 정의할 수 없을 것 같은데요 이것도 리뷰 드리면서~! |
| public boolean isValidFormula(String inputRefined) { | ||
|
|
||
| boolean startWithNum = true; | ||
| boolean onlyAllowed = true; | ||
|
|
||
| char firstChar = inputRefined.charAt(0); | ||
|
|
||
| if (firstChar == '+' || firstChar == '*' || firstChar == '/') { | ||
| System.out.println("유효하지 않은 수식입니다!"); | ||
| startWithNum = false; | ||
| } | ||
|
|
||
| if (!inputRefined.matches("^([0-9/./+/*///-]*)[0-9]$")) { | ||
| onlyAllowed = false; | ||
| } | ||
|
|
||
| return startWithNum && onlyAllowed; | ||
| } |
| private String formalize(String expression) { | ||
| StringBuilder sb = new StringBuilder(); | ||
| char c; | ||
|
|
||
| for (int i = 0; i < expression.length(); i++) { | ||
| c = expression.charAt(i); | ||
|
|
||
| if (!Character.isDigit(c)) sb.append(' ').append(c).append(' '); | ||
| else sb.append(c); | ||
| } | ||
|
|
||
| if (sb.length() > 1 && sb.charAt(1) == '-') { | ||
| sb.deleteCharAt(2); | ||
| sb.deleteCharAt(0); | ||
| } | ||
|
|
||
| return sb.toString(); | ||
| } |
There was a problem hiding this comment.
공백이 제거된 연산식에 공백을 끼워넣는 작업으로 보이는데요
원래 입력된 연산식을 그대로 넣어 줄 수 는 없을까요~?
| public interface CalculatorFunction { | ||
|
|
||
| String name = null; | ||
|
|
There was a problem hiding this comment.
이 name이라는 변수가 오직 로그를 찍기 위해서 CalculatorFunction 인터페이스를 포함해서 CalculatorFunction의 구현체인 CalculateService의 인스턴스를 생성하는 과정에서부터 포함되고 있는데요, 로그를 찍는게 중요할 수도 있지만, 설령 중요하더라도 이런 변수를 인터페이스에 선언할 이유는 없고, 지금 찍고 계신 로그는 별로 중요한 내용은 아닌 것 같아서요! ("계산", "test" 등)
이걸 위해서 인터페이스에서부터 변수를 추가할 필요가 있는지 생각해봐야할 것 같습니다.
| String expression; | ||
| LinkedList<String> expForCal; | ||
| String result; |
There was a problem hiding this comment.
이런식으로 지역 변수를 선언하면서 초기화 하지 않는 코드는 가급적 지양하는게 좋습니다.
예외 처리 때문에 이렇게 되었겠죠?
이렇게 나뉘지 않도록 만들어보세요. :)
| public String calculate(LinkedList<String> expForCal) throws Exception { | ||
| return plusOrMinus(divideOrMultiple(expForCal)); | ||
| } |
There was a problem hiding this comment.
이 코드가 public인 이유는 무엇일까요?
아마 테스트 코드 때문에 이렇게 하셨을 것 같은데, 이렇게 따로 테스트하고 싶어지는 경우 CalculateService처럼 너무 클래스가 하는 일이 많을 가능성이 높습니다.
실제로 CalculateService는 현재 160여줄이나 될 정도로 길죠.
클래스를 한번 나눠보시기 바랍니다.
나눌 때의 방향성은 팀 미팅때도 이야기드렸던 것처럼, 가급적 입력과 출력에 의존적이지 않도록 바꿔보세요. 현재에는 다분히 키보드에서 입력되는 표준 입력에 의존적인 코드로 보입니다.
There was a problem hiding this comment.
그래도 연산자 우선순위를 메서드의 호출 순서를 통해 해결하신건 인상적이네요.
There was a problem hiding this comment.
예외 관련해서는 Checked Exception 말고 Unchecked Exception으로 바꿔보시기 바랍니다.
| public class Calculator { | ||
|
|
||
| private Integer optionNumber = 2; | ||
| private ResultSaveService resultSaveService; | ||
| private CalculateService calculateService; | ||
| private static Map<Integer, CalculatorFunction> menus = new HashMap<>(); | ||
| private StringBuilder menusBuilder = new StringBuilder(); | ||
| private CalculatorFunction calculatorFunction; | ||
|
|
||
| public Calculator(ResultSaveService resultSaveService, CalculateService calculateService) { | ||
| this.resultSaveService = resultSaveService; | ||
| this.calculateService = calculateService; | ||
| } | ||
|
|
||
| public static void main(String[] args) { | ||
|
|
||
| ResultSaveService saveService = new ResultSaveService("조회"); | ||
| CalculateService calService = new CalculateService("계산"); | ||
| Calculator calculator = new Calculator(saveService, calService); | ||
|
|
||
| calculator.menus.put(1, saveService); | ||
| calculator.menus.put(2, calService); | ||
|
|
||
|
|
||
| for (int i = 1; i <= calculator.menus.size(); i++) { | ||
| calculator.menusBuilder.append(i + ". " + menus.get(i).getName() + "\n"); | ||
| } | ||
|
|
||
| calculator.menusBuilder.append("\n"); | ||
| calculator.menusBuilder.append("선택 : "); | ||
|
|
||
| calculator.run(); | ||
| } |
There was a problem hiding this comment.
입력과 출력으로부터 코드를 분리하는 것의 일환으로 main 메서드가 있는 클래스에서 입출력이 이뤄지지 않는게 좋습니다.
입출력을 처리하는 클래스를 따로 빼보시고, main가 있는 이 클래스는 그 클래스를 생성하여 사용하는 형태로 바꿔보세요. :)
| if (results.size() == 0) { | ||
|
|
||
| System.out.println("[ 조회된 계산이력 없음 ]\n"); | ||
| } else { | ||
| StringBuilder savedResults = new StringBuilder(); | ||
| Iterator<Integer> iterator = results.keySet().iterator(); | ||
|
|
||
| while(iterator.hasNext()) { | ||
| Result result = results.get(iterator.next()); | ||
| savedResults.append(result.fullEquation()); | ||
| } | ||
| System.out.println(savedResults); | ||
| } |
There was a problem hiding this comment.
간단한 스킬이지만, 이렇게 if-else 문은 조금 더 가독성 좋게 바꿔볼 수 있습니다.
early return이라고 부르는 방법인데요, 아래와 같이 코드를 바꾸는겁니다.
if (results.size() == 0) {
System.out.println("[ 조회된 계산이력 없음 ]\n");
return;
}
StringBuilder savedResults = new StringBuilder();
Iterator<Integer> iterator = results.keySet().iterator();
while(iterator.hasNext()) {
Result result = results.get(iterator.next());
savedResults.append(result.fullEquation());
}
System.out.println(savedResults);
| class CalculateServiceTest { | ||
|
|
There was a problem hiding this comment.
테스트 코드 관련해서는 현재 상태로도 문제 없습니다.
다만, 위에 리뷰드린 것처럼 private 이어도 충분한 메서드를 테스트를 위해 public으로 바꾼 부분에 대해 '어떻게하면 private을 public으로 바꾸지 않고도 테스트할 수 있을지?' 고민해보시기 바랍니다.
사실 지금 단계에서 생각하기는 꽤 어려운 주제일 수도 있어요. 그래도 한번 고민해보세요!
|
최지연님 수고하셨습니다. :) 아래는 궁금하다고 적어주신 내용에 대한 답변입니다.
-> 지금도 일부 정규식을 활용하고 계신 것 같은데요, 말씀해주신대로 정규식을 활용하면 검증 과정을 더 개선해볼 수 있습니다!
-> 이건, 팀 미팅때와 리뷰에 적어뒀다고 생각합니다. 지연님 코드에서 간단히 요약하자면,
이 2가지가 쟁점일 것 같습니다. 이 외에도 클래스를 나누는 기준은 많이 있긴해요. 그러나 우선 이정도에서 시작해보시면 좋을 것 같습니다!
저는 적절한 개수라고 생각하긴 했어요. 물론 조금은 더 나뉘어도 좋겠다고 싶었던 부분은 있었습니다. 다만, 위에 2가지 포인트 이야기 드린건 좀 분리되면 더 적절할 것 같다고 생각이 들긴했습니다. 그러나 지금처럼 작성하셔도 문제가 있는건 아니에요. |
java-calculator
과제설명
요구사항
구현할 때 궁금했던 점