Skip to content

Conversation

@pkalsh
Copy link
Member

@pkalsh pkalsh commented Feb 25, 2021

간단한 프로그램인데 힘드네요..

Before:
  - Calculator 클래스의 calculate 메소드가 문자열을 분해하고 분해한 식을 계산까지 했음
  - 너무 많은 책임을 가지고 있다.

After:
  - Calculator의 calculate 메소드 자체는 문자열을 분해하기만 하고 계산은 Expression 클래스에게 위임
  - 정적 팩토리 메소드 of로만 Expression 객체를 얻을 수 있음
  - 할당된 식은 evaluate 메소드에 의해 계산됨
  - 연산자의 종류에 따른 연산은 Operator 클래스에 위임
  - 정적 팩토리 메소드 from에 의해 파라미터로 주어진 연산자에 따라 다른 자식 클래스 생성
Before:
  - left, right, operator 3개의 매개변수를 받는 팩토리 메소드만 존재
  - 반복문으로 표현할 수 없었음

After:
  - 왼쪽은 Expression 참조 자료형으로 받을 수 있기 때문에 첫번째 연산자의 왼쪽 피연산자만 Expression으로 표현할 수 있으면 for문으로 작성 가능
  - int 자료형 하나만 받는 팩토리 메소드를 추가
  - Expression(int)는 "매개변수 + 0"으로 초기화하여 단순한 int 자료형을 표현
- expression과 관련된 클래스들은 계산을 수행하는 역할을 가진다.
- 입력으로 들어온 문자열을 의미 단위로 나누는 역할을 하는 Parser 클래스를 calculator 패키지에 위치시키기 위해 다른 역할을 하는 expression 관련 클래스들은 분리시켰다.
- 비즈니스 로직상 우선순위 없이 앞에서부터 계산하면 되므로 큐를 기반 자료구조로 사용
- Parser는 입력받은 문자열을 공백으로 나누어 큐에 저장하여 다음 토큰을 nextToken으로 건네준다.
- Parser가 어떻게 다음 토큰을 건네주는가에 대한 것은 capsulation하여 information hiding한다.
- 하나의 공백이 아니라 하나 이상의 공백을 나타내는 정규표현식인 " +"로 split
- 공백으로 시작하는 문자열일 경우 길이가 0인 문자열이 발생하므로 filterMeaningfulToken이 필터링 해서 의미있는 토큰만 남겨줌
- 토큰 검사는 숫자인가 아닌가, 하나의 연산자로 구성되어 있는가 아닌가로 판단
- 정규표현식 사용
- invalidToken이 하나 이상일 경우 throw InvalidInputException
- exit이 입력으로 들어올 경우 프로그램 종료하도록 구현
- 계산기 결과는 Optional로 받아와 예외가 발생한 경우 비어있는 객체를 반환하도록 하여 반복문을 다시 돌게 구현
@pkalsh pkalsh changed the title Pkalsh OOP 계산기 Feb 25, 2021
@pkalsh pkalsh marked this pull request as draft February 25, 2021 17:34
@pkalsh pkalsh marked this pull request as ready for review February 25, 2021 17:36
- 입력 식이 올바른 형식인지 검사하는 역할을 담당하는 ExpressionValidator로 기능 분할
- 분할된 토큰 리스트의 순서가 "숫자 연산자 숫자 연산자 ... 숫자"의 순으로 나열되어 있는지 검사하는 기능 구현
songpang pushed a commit to songpang/java-calculator that referenced this pull request Feb 25, 2021
feat: add TDD basic format and test case for calculator class
Copy link
Member

@oereo oereo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oereo code review

```java
String value = scanner.nextLine();
String[] values = value.split(" ");
String[] values = value.split(" ");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

공백을 추가하신 이유는 무엇인가요???

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

글쎄요 .. 그냥 오타난거 같은데 원래 그런게 아니었나봐요?

package ui.printer;

public class ConsolePrinter implements Printer {
private final String GREET_MESSAGE = "계산기 프로그램입니다.\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 java 에서 Multiline에 대한 기능을 제공을 안해주는 상황인 것 같아요.
Adrian Walker의 @Multiline 라이브러리가 있는 것 같은데 필요성이 있다면 사용하셔도 괜찮을 것 같아요!

ex)

Suggested change
private final String GREET_MESSAGE = "계산기 프로그램입니다.\n" +
/**
계산기 프로그램입니다.
숫자와 사칙연산 기호를 공백으로 구분하여 입력해주세요.
프로그램을 종료하시려면 exit을 입력해주세요
*/
@Multiline String GREET_MESSAGE;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

보기 안좋았는데 그런 라이브러리가 있었군요!
한번 적용해보겠습니다 😄

}
}

private Optional<Integer> calculateExpression(String expression) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적인 궁금사항입니다
optional 을 사용하신 이유가 직접적으로 nulll check에 대한 부분때문에 사용하신 것 같습니다
optional 이 함수형 언어에서 따온 문법이라고 하는데 객체지향적으로 코드를 짠다고 했을 때 optional을 사용하는게 단순 null check를 할 때와 차이가 궁금합니다.

결론 : 동민님이 이 코드 부분에서 optional를 사용하신 이유가 궁금합니다

Copy link
Member Author

@pkalsh pkalsh Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이펙티브 자바에 실린 내용을 그냥 전적으로 따랐는데요.
아이템 55에 나와있는 내용입니다.

자바 8 전에는 메서드가 특정 조건에서 값을 반환할 수 없을 때 취할 수 있는 선택지가 두 가지 있었다.
예외를 던지거나, (반환 타입이 객체 참조라면) null을 반환하는 것이다.
두 방법 모두 허점이 있다. 예외는 진짜 예외적인 상황에서만 사용해야 하며 예외를 생성할 때 스택 추적 전체를 캡처하므로 비용도 만만치 않다.
null을 반환하면 이런 문제가 생기지 않지만, 그 나름의 문제가 있다. null을 반환할 수 있는 메서드를 호출할 때는, 별도의 null 처리 코드를 추가해야 한다. null 처리를 무시하고 반환된 null 값을 어딘가에 저장해두면 언젠가
NullPointerException이 발생할 수 있다. 그것도 근본적인 원인, 즉 null을 반환하게 한 실제 원인과는 전혀 상관없는 코드에서 말이다.

저자가 이런 이유로 null 반환을 꺼려하고 그 대안으로 Optional 반환을 소개하고 있어요. 물론 제 코드에 적용되는 부분은 없어 보이지만 책 내용을 따라 null 반환을 의도적으로 안하려는 습관을 들이려고 Optional을 택했습니다. 😃


private boolean hasNotAnyToken(List<String> tokenList) {
return (tokenList.size() == 0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

라인간격을 한줄씩 띄는 것으로 통일해도 좋을 것 같습니다
맥 기준 : option + command + L 을 하면 자동적으로 정렬 및 포맷을 맞추어 줍니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정렬했습니다!

@@ -0,0 +1,37 @@
package domain.calculator;

import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import java.util.stream.Collectors 하나의 라이브러리를 사용하고 있는 상황이라면 넣지 않아도 될 것 같습니다

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java.util.* 임포트 구문은 LinkedList와 Queue, List, Arrays를 끌어다 쓰는 과정에 인텔리제이에서 자동으로 임포트해준 것으로 보입니다

Copy link
Member

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 🙂
추상클래스와 인터페이스, Optional등 자바의 다양한 기술들을 활용하셨네요. 때문인지 많은 학습을 하고 있다는게 느껴졌습니다. 열정 👍

지금은 학습에 더 중점이 있기 때문에 자유롭게 시도해보는것에 반대하지는 않습니다. 저 또한 그런 성향이 강했고, 그로 인해 많이 성장했다고 생각하기 때문에 오히려 반갑습니다.

다만, 학습이 아닌 엔지니어로서의 역할을 하게 될땐, 오버엔지니어링과 그로 인한 생산성이 떨어지는것에 대해 경계해야 됩니다.

오버엔지니어링과 언더엔지니어링의 경계

  • Printer와 Receiver를 인터페이스로 만든 이유가 있나요?
  • 확장성을 고려하다 가독성이나 유지보수성이 낮아지진 않았나요?
  • 간단한 프로그램인데 힘든 이유는..?

"Scalable" 이란 단어는 소프트웨어 엔지니어의 마음에 신비롭고 깜짝 놀라게 하는 힘이 있음. 살짝 입밖에 내는 것만으로 그들을 타락한 광란에 빠져들게 함. 이 단어를 사용함으로써 무자비한 행동들이 정당화 됨

(번역) 업계에서 6년 있은 뒤, 마음이 바뀐 소프트웨어 개발 토픽들

@pkalsh pkalsh requested a review from kouz95 March 21, 2021 08:06
Copy link
Member

@kouz95 kouz95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enum과 추상클래스 둘 다 시도해 보셨네요 👍

추가적으로 적용해 볼 수 있는 것에 대해 간단한 리뷰 남겼으니 확인해주세요!

이외에도 ExpressionValidator의 로직이 꽤나 복잡해 보이는데, Integer.parseInt()Operation의 생성 과정 중에서 대부분의 예외처리가 이루어지고 있지 않을까? 라는 생각도 들어요.
작은 클래스들을 많이 만들고, 클래스의 생성 과정에서 유효성 검사를 진행한다면 Validator와 같은 클래스에 의존하지 않고도 스스로 안전한 객체임을 보장할 수 있을거에요 🙂

요구사항은 충분히 만족한것 같아 Approve 하겠습니다 👏
고생하셨습니다!

Comment on lines +47 to +49
int result;
result = calculator.calculate(expression);
return Optional.of(result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int result;
result = calculator.calculate(expression);
return Optional.of(result);
return Optional.of(calculator.calculate(expression));

도 충분할 것 같습니다 🙂

Comment on lines +7 to +9
private int left;
private int right;
private Operator operator;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private int left;
private int right;
private Operator operator;
private final int left;
private final int right;
private final Operator operator;

Expression을 불변 객체로 만들 수도 있겠네요 🙂

@kouz95 kouz95 merged commit e92d674 into biforest:pkalsh Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants