[4기 - 박상민] 1~2주차 과제: 계산기 구현 미션 제출합니다. #188
Conversation
smartandhandsome
changed the title
| private final int MAX_COUNT = 3; | ||
|
|
||
| @Override | ||
| public void run() { | ||
| int stageNumber = 0; | ||
| while (stageNumber < MAX_COUNT) { | ||
| int choice = -1; | ||
| try { | ||
| choice = view.getChoice(); | ||
| } catch (WrongValueException e) { | ||
| view.wrongInput(); | ||
| continue; | ||
| } | ||
| Option option = Option.from(choice); | ||
| Execution execution = option.convert(); | ||
| if (!execution.execute()) { | ||
| view.wrongInput(); | ||
| continue; | ||
| } | ||
| stageNumber++; | ||
| } | ||
| } |
There was a problem hiding this comment.
사실 실제 계산기라면 의미없는 제한일 수 있는데
README에 실행결과가 3번만 일어나길래 실행결과와 똑같이 만들기 위해서
변수를 써서 3번으로 제한 해뒀습니다ㅎㅎㅎ
| SUB("-", (a, b) -> a - b), | ||
| MUL("*", (a, b) -> a * b), | ||
| DIV("/", (a, b) -> a / b), | ||
| EMPTY(null, (a, b) -> null); |
| @Override | ||
| public void printAll() { | ||
| for (int i = 0; i < id; i++) { | ||
| SolvedExpression solved = repository.get(i); | ||
| Console.output.println(solved.getExp() + " = " + solved.getResult()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Repository는 print를 시키는게 아니라 리스트를 반환하는게 더 입출력에 의존적이지 않은 애플리케이션을 만들 수 있을 것 같습니다.
만약 이 애플리케이션이 웹 애플리케이션으로 바뀐다면 이 레포지토리는 사용할 수 없겠죠?
| public class Expression { | ||
|
|
||
| private final String infix; | ||
| private final List<String> infixExp; | ||
| private final List<String> postfixExp; | ||
|
|
||
| public Expression(String infix) throws WrongExpressionException, WrongValueException { | ||
| this.infix = infix; | ||
| this.infixExp = Arrays.asList(infix.split(" ")); | ||
| if (isWrongExpression()) { | ||
| throw new WrongExpressionException(); | ||
| } | ||
| this.postfixExp = new InfixToPostfix().convert(infix); | ||
| } | ||
|
|
||
| public SolvedExpression solve() { | ||
| Stack<Double> stack = new Stack<>(); | ||
| for (String s : postfixExp) { | ||
| if (NumberValidator.validate(s)) { | ||
| stack.push(Double.parseDouble(s)); | ||
| } else if (OperationValidator.validate(s)) { | ||
| Double a = stack.pop(); | ||
| Double b = stack.pop(); | ||
| ArithmeticOperation operation = ArithmeticOperation.from(s); | ||
| Double calculated = operation.calculate(b, a); | ||
| stack.push(calculated); | ||
| } | ||
| } | ||
| Double result = stack.pop(); | ||
| return new SolvedExpression(infix, result); | ||
| } |
There was a problem hiding this comment.
표현식을 이렇게 하나의 객체로 묶어서 연산이 내부에서 이뤄지도록 만든건 인상적이네요. 좋습니다. :)
|
@smart-sangmin 상민님 수고하셨습니다. 객체지향적으로 코드를 짜려고 노력하신 경험이 없는거 치곤 잘 작성하셨는데요? ㅎㅎ 궁금하다고 적어주신 부분 답변드리겠습니다.
-> 뭐 이건 나누나 안나누나 비슷했을 것 같아요. 다만 Option에서 유효한 choice인지 검증할 수 있고, 검증하는 로직이 Runner 쪽으로 노출되지 않기 때문에 의미는 있어보입니다. 다만 이 부분이 다이나믹한 차이를 만들지는 않을 것 같습니다. 더 간단하게 만들려고 했다면 그냥 Option 에서 진행하는 일을 Runner의 private 메서드로 만들었어도 가능했을 것 같네요.
-> 문제는 없어보이는데, 이건 나중에 의존성 주입(DI)에 대해 학습해보시면 차이를 느끼실 수 있을겁니다. 지금은 크게 신경쓰지 않으셔도 될 것 같아요. 아마 이렇게 만드신 이유는 공용으로 쓰이는데 매번 새로 생성하기 싫어서 그랬겠죠? 한번만 생성한다는 점에서는 긍정적이라고 봅니다.
-> 보통 커밋 단위는 기능이 실행될 수 있는 단위로 진행하는 경우가 일반적입니다. 아직 상민님이 개발하려는 기능을 어떻게 개발해야할지 머리속에 잘 그려지지 않는 상태라면 이렇게 될 수 있을 것 같아요. 나중에는 어떤 코드를 만들어야할지 머리속에 그려지실겁니다. 그럼 그때에는 기능 단위로 생각보다 쉽게 개발하실 수 있을거에요. 제가 간단하게만 이야기했는데, 관련해서는 git branch 전략을 찾아보시면 여러가지 방법들이 나옵니다.
-> 일반적으로 전자가 권장되는데, 후자의 경우 null 객체 패턴이라는 코드 형태입니다. 둘다 장단점이 있어서 뭐가 더 나은 방법이라고 이야기할 수 없어요. 후자의 경우 코드를 다형성 있게 작성하는데 도움을 줍니다. 그러나 오용하면 안됩니다. 지금 작성하신 코드에서는 개인적으로 예외가 발생되는게 더 적절해 보이긴해요. 왜냐하면 잘못된 값이 입력된 것은 일종의 유효성 검사에 실패한 것이기 때문에 예외로 받아드리는게 좋다고 생각하기 때문입니다.
-> 이건 팀미팅때 이야기 했던것같기도한데... 사실 이런 비율이라는게 의미가 있나 모르겠네요. ㅎㅎ 매번 다르기도하고요. 다만 말씀드리고 싶은건 상민님 단계에서는 최대한 많이 코드를 작성하고 구현해보라는겁니다. 설계를 위해선 구현 경험이 있어야하는데 지금은 충분한 구현 경험이 없기 때문에 설계를 위해 머리를 싸매고 시간을 많이 투자해도 막상 구현 과정에서 기존 설계가 잘못된 경우가 많을겁니다. |
There was a problem hiding this comment.
상민님 구현하시느라 고생많으셨습니다.
소소한 피드백을 남겨드렸으니 확인부탁드리고
주신 질문에 대한 답변 남겨드립니다.
int choice 를 Option 으로 바꾸고 Option 을 Execution 으로 바꿉니다. (일단 제 생각엔 Option은 유저가 선택할 수 있는 옵션, Execution은 실행에 초점을 맞췄는데 어떻게 생각하시나요? 이런 경우에 Option 굳이 필요없을까요? 뭐가 좋은건지 모르겠습니다.)
상민님도 의문이 드신만큼 현재 코드가 크게 의미가 있는 코드가 아니라서 있어도 그만 없어도 그만인것 같습니다.
Console을 공용으로 쓰기 때문에 static으로 두었는데 괜찮은가요?
상민님은 현재 객체를 컴파일 타임에 의존하도록 구현하고 계신데요 에프님이 말씀주신것처럼 DI에 대해 학습해 보시고 객체를 외부에서 주입할 수 있을때 어떤 이점이 있는지 공부하시면 가지고 계신 궁금증을 해결하시는데 도움이 되리라 생각합니다.
하나하나 커밋을 해야되지만, A부분을 커밋하고 B를 구현하고 있었는데 갑자기 A가 더 나눠질거 같아서 나눴습니다. 이후 다시 또 생각해보니 A를 나누는게 좀 억지인거 같아서 다시 고치고 이런 동작을 반복하다 보니까 git log가 엉망이 되고 머리속이 복잡해져서 다시 새로하고 다시 새로하고 하다가 결국 다 코드 구현을 완성하고 차례로 커밋하는 식으로 해버렸습니다... 멘토분들은 어떤 식으로 하시는지, 또 git commit 하시는 시점이 궁금합니다.
미리 커밋 메시지를 작성해놓고 커밋 메시지에 해당하는 기능만을 구현하는 방법을 사용해보시면 어떨까요? 요새 tdd를 학습하면서 기능 목록을 먼저 정의하고 정의한 기능 목록을 commit 메시지로 미리 써놓고 해당 기능만 구현하는 느낌으로 코드를 작성하고 있는데 괜찮더라구요 ㅎ
ArithmeticOperation에서는 해당 없는 값은 Exception을 발생시켰는데 Option에서는 None으로 처리했습니다. 혹시 둘 중 어떤게 좋은 방법인가요?
에프님이 깔끔하게 답변주셨네요
마지막으로 이번에 계산기 만들면서 초기 설계 단계에서 충분하게 고민을 안 해서 그런지 태평양에서 헤엄치는 느낌이 들었습니다. 두 분은 프로젝트를 시작할 때 설계와 구현의 비율을 어느정도로 하시는지 궁금합니다.
저는 동료들과 커뮤니케이션을 많이 해보면서 도메인을 정확히 맞춘 다음에 설계에 들어가는데요
설계를 대부분 끝내고 구현을 하는 편입니다. 결국 설계한대로 구현하게 되기 때문에 설계를 하지 않으면 구현도 못하겠더라구요
다만 지식이 부족해서 설계하기 까다로운 부분은 프로토타이핑을 통해 구현 가능성을 먼저 검증하고 실제 구현에 들어가는 편입니다.
| public Double calculate(Double operand1, Double operand2) throws DivideByZeroException { | ||
| if (this.operation.equals("/") && operand2 == 0) { | ||
| throw new DivideByZeroException(); | ||
| } | ||
| return function.apply(operand1, operand2); | ||
| } |
There was a problem hiding this comment.
기왕 Enum 만드신거 "/" 말고 DIV enum으로 비교해보시면 어떨까요?
| public class Calculate implements Execution { | ||
| ExpressionRepository repository = new ExpressionRepository(); | ||
|
|
There was a problem hiding this comment.
Repository 인터페이스를 구현한 ExpressionRepository가 추상화된 Repository 타입이 아닌 구체 타입인 ExpressionRepository으로 선언되어 있고 해당 객체를 Calculate가 직접 의존하고 있는데요
때문에 Repository 인터페이스를 선언하신 이유를 찾기 힘들어보이네요
| public SolvedExpression solve() { | ||
| Stack<Double> stack = new Stack<>(); | ||
| for (String s : postfixExp) { | ||
| if (NumberValidator.validate(s)) { | ||
| stack.push(Double.parseDouble(s)); | ||
| } else if (OperationValidator.validate(s)) { | ||
| Double a = stack.pop(); | ||
| Double b = stack.pop(); | ||
| ArithmeticOperation operation = ArithmeticOperation.from(s); | ||
| Double calculated = operation.calculate(b, a); | ||
| stack.push(calculated); | ||
| } | ||
| } | ||
| Double result = stack.pop(); | ||
| return new SolvedExpression(infix, result); | ||
| } |
| public boolean isWrongExpression() { | ||
| int cntNumber = 0; | ||
| int cntOperation = 0; | ||
| for (String s : infixExp) { | ||
| if (NumberValidator.validate(s)) { | ||
| cntNumber++; | ||
| } else if (OperationValidator.validate(s)) { | ||
| cntOperation++; | ||
| } | ||
| } | ||
| return cntOperation != cntNumber - 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
해당 클래스 내부에서만 사용하는 메서드라면 private으로 두셔서 캡슐화시키는건 어떨까요~?
| public class OperationValidator { | ||
| private static final Pattern OPERATION_PATTERN = Pattern.compile("^[+\\-*\\/]$"); | ||
|
|
||
| public static boolean validate(String s) { | ||
| return OPERATION_PATTERN.matcher(s).matches(); | ||
| } | ||
| } |
There was a problem hiding this comment.
잘 작성해주셨네요 👍
지금처럼 작은 기능 단위로 책임을 나눠서 구현하고 이를 테스트 하시면서 서비스를 점진적으로 구축해나가시면 더욱 안정성있고 변경에 유연한 구조를 가져가는데 도움이 될꺼라고 의견드려봅니다.
| public static void main(String[] args) { | ||
| Runnable runnable = new Runner(); | ||
| Thread t = new Thread(runnable); | ||
| t.start(); | ||
| } |
There was a problem hiding this comment.
메인 스레드에서 실행하시지 않고 Runnable 인터페이스를 구현해서 별도의 스레드로 실행하신 이유가 궁금해지네요
There was a problem hiding this comment.
이전에 이렇게 사용한다고만 어렴풋이 듣었던 걸 Thread와 Runnable에 대한 이해도가 없이 작성했습니다.
Thread와 Runnable에 대해 공부를 해보겠습니다!
| @Test | ||
| @DisplayName("사칙연산 도출") | ||
| void from() { | ||
| //given | ||
| String[] operations = {"+", "-", "*", "/"}; | ||
| // when | ||
| // then | ||
| assertThat(ArithmeticOperation.from(operations[0])).isEqualTo(ArithmeticOperation.ADD); | ||
| assertThat(ArithmeticOperation.from(operations[1])).isEqualTo(ArithmeticOperation.SUB); | ||
| assertThat(ArithmeticOperation.from(operations[2])).isEqualTo(ArithmeticOperation.MUL); | ||
| assertThat(ArithmeticOperation.from(operations[3])).isEqualTo(ArithmeticOperation.DIV); |
There was a problem hiding this comment.
@ParameterizeTest를 활용해보는건 어떨까요?
다른분이 작성하신걸 참고해보시면 좋을 것 같습니다
| Option o = Option.from(choice); | ||
| Execution convert = o.convert(); | ||
| // then | ||
| assertThat(convert instanceof Inquiry).isTrue(); |
There was a problem hiding this comment.
구체 타입을 검증하시는 거라면 isExactlyInstanceOf를 사용해볼 수 있는것 같습니다.
참고
📌 과제 설명
계산기
👩💻 요구 사항과 구현 내용
Class 설명
CommonException: Custom ExceptionIO.console: Console 입출력Runner: 실행DomainCalculator: 사칙연산Execution: 사용자 실행 옵션 (조회, 계산, 그 외)Expression: 계산 식, 저장소, 다 푼 계산 식utils: 중위표기식을 후위표기식으로 변경, 숫자 검증, 연산자 검증View: 사용자 인터페이스Main: app 실행✅ PR 포인트 & 궁금한 점
일단 조금 늦게 제출한 점 멘토님들께 너무 죄송합니다.
변명을 쪼끔하자면... 저번 주에 학교 시험이 있고 막학기다 보니 학교쪽에 쪼금 힘을 줬습니다...
시험 끝나고 하면 되겠지 했는데, 과제 구현 중 피드백 받을 시간이 부족하단 걸 깨달았습니다...
그래서 피드백은 불닭볶음면맛으로 부탁드립니다. 겸허히 받아드리겠습니다.
일단 저는 이전에 객체지향적인 코드를 짜려고 노력하면서 코딩을 한 경험이 없습니다.
그래서 근거를 두고 코드를 작성하려고 했는데 아는게 별로 없다보니 근거를 찾기가 매우 힘들었습니다.
int choice를Option으로 바꾸고Option을Execution으로 바꿉니다. (일단 제 생각엔 Option은 유저가 선택할 수 있는 옵션, Execution은 실행에 초점을 맞췄는데 어떻게 생각하시나요? 이런 경우에Option굳이 필요없을까요? 뭐가 좋은건지 모르겠습니다.)Console을 공용으로 쓰기 때문에 static으로 두었는데 괜찮은가요?ArithmeticOperation에서는 해당 없는 값은 Exception을 발생시켰는데Option에서는 None으로 처리했습니다. 혹시 둘 중 어떤게 좋은 방법인가요?