-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[4기 - 이예빈] 1~2주차 과제 : 계산기 구현 미션 중간 PR입니다. #153
base: yebinlee
Are you sure you want to change the base?
Conversation
- Infix 중위 표현 수식에서 Postfix 후위 표현 수식(리스트)로 변환
- 메뉴 선택 입력 - 입력 validate 확인 메서드 구현
- Integer로 모두 변환
} | ||
} | ||
|
||
public Integer calculateFromPostfix(ArrayList<String> postfixExpression){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 메서드는 진짜 계산기의 역할인거 같아요 . 그런데 위에서 runable한 Application의 역할을 하고 있는 것 같아서 분리할 수 있을 것 같아요!
return Integer.valueOf(calcStack.pop()); | ||
} | ||
|
||
public static boolean validateChoiceInput(String input){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate라는 동사의 메서드는 어떤 명령을 하는 것 같은 데 boolean 타입의 반환이 오는 게 약간 어색하게 느껴질 수 있을 것 같아요. isValidXXX 라는 메서드 네이밍이 더 좋지 않을까 라는 생각이 들었어요. 보통은 is나 has 같은 동사로 시작하는 메서드들이 boolean타입을 반환하는 느낌 잘 드러내는 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가로 해당 메서드를 Validator나 입력 형태를 보장하는 DTO 클래스로 역할을 위임할 수도 있을 것 같아요
String expression = input.getExpression(); | ||
if (!validateExpression(expression)) output.inputError(INVALID_INPUT_EXPRESSION); | ||
else { | ||
ArrayList<String> postfixList = expressionConverter.convert(expression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List라는 추상화된 인터페이스가 아닌 ArrayList라는 구체적인 구현체를 사용하신 이유가 있을까요?
|
||
@ToString | ||
public class InfixToPostfixConverter implements ExpressionConverter { | ||
private ArrayList<String> infix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컨버터는 입력과 출력이 항상 보장될 수 있는 stateless한 형태로 유지될 수 있을 것 같아요! 어떻게 생각하시나요?
|
||
@Override | ||
public String toString() { | ||
return command + ". " + explanation; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
private final static LinkedHashMap<Integer, String> results = new LinkedHashMap<>(); | ||
|
||
public void save(String calculation) { | ||
results.put(results.size() + 1, calculation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
해당 과제에서 다루기엔 조금 과할 수도 있지만 멀리 쓰레드 환경에서 동시성 문제가 발생할 수 있을 것 같아요! 동시성 문제를 해결할 수 있는 방법 또는 유니크 아이디를 생성할 수 있는 방법에 대해서 고민해보면 좋을 것 같아요
import java.util.List; | ||
|
||
public class CalculationRepository { | ||
private final static LinkedHashMap<Integer, String> results = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List가 아닌 Map으로 구현한 이유가 무엇일까요?
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Test; | ||
|
||
public class ExpressionValidationTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validator로 분리되어 있다면 적절한 테스트가 되었을 것 같아요 :)
import java.util.Arrays; | ||
|
||
public class InfixToArrayListTest { | ||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParameterizedTest 같은 것으로 다양한 입력에 대해서 검증해볼 수 있을 것 같아요!
- Menu의 command를 Character로 변경 - 종료 메뉴 추가 및 IO 정의
- 다양한 방법의 수식(전위,중위,후위 표현)을 convert하여 계산 가능하도록 strategy pattern 도입하여 리팩토링
- 다양한 방법의 수식(전위,중위,후위 표현)을 convert하여 계산 가능하도록 strategy pattern 도입하여 리팩토링
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 예빈님 리뷰남겼으니 확인해주세요.
global
이란 패키지를 만드셨는데, 내부 클래스들의 적절한 패키지 위치가 어딘지 고민해보시고 재배치 해주세요
- 하드 코딩을 지양하고자 다양한 상수들을 사용하였는데 이를 어떻게 관리하는 것이 좋을지
- 리뷰 남겼습니다. 전역에서 사용할 수 있는 상수 클래스를 만드는 것은 개인적으로 선호하지 않습니다. 도메인 정보가 흩어진다고 생각해서요
- 객체 지향적인 설계를 잘 해내지 못한 것 같은데, 보완해야 할 부분들이 어딘지 궁금합니다!
- 리뷰 확인해주세요
- global 패키지에 위치할 클래스가 정말 전역적으로 사용되는 것인지, 유틸 로직으로 분리한 것이 정말 하나의 유틸 클래스에 위치하는 것이 맞는지 고민해보세요.
- 클래스 내의 메서드들 (validateChoiceInput(), validateExpression() 등) 의 결과값을 테스트하고자 기존 private 지정자에서 public 지정자로 바꾸었는데, 캡슐화 원칙을 지키면서도 각 기능 단위로 단위 테스트 코드를 작성하는 방법이 있을까요?
- private을 public으로 단순히 변경했다면 그 메서드가 다른 클래스로 분리될 수 있다는 힌트가 될 수 있습니다.
void putMenu(); | ||
void showCalculationResult(List<CalculationResult> calcResults); | ||
void inputError(String errorResponse); | ||
void showResult(String calculationResult); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
result 객체를 매개변수로 전달 받도록 해도 좋을 것 같아요
continue; | ||
} | ||
List<String> convertedExpression = expressionConverter.convert(expression); | ||
Object result = calculator.calculate(convertedExpression); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object
타입은 보통 잘 사용하지 않아요. String, Integer 등등 모든 타입을 할당할 수 있기 때문이죠. Object
타입을 사용하신 이유가 있을까요?
output.showResult(result.toString()); | ||
calculationRepository.save(new CalculationResult(expression, result.toString())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString
을 직접 호출하기 보다는 객체를 파라미터로 넘겨주는 것이 좋겠네요.
toString
의 사용 목적도 한 번 생각해보셔도 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 문자열을 따로 관리하는 것이 좋은지 조금 의문이 드네요.
결국 각각의 클래스가 가져야할 정보를 한 곳에 모아둔 거라는 생각이 들어서요.
실제로 공통적으로 사용되는 문자열도 없어서요
@@ -0,0 +1,16 @@ | |||
package calculator.global; | |||
|
|||
public class InputConstants { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 클래스도 ErrorResponse
와 동일한 의견입니다.
public Operation() { | ||
setOperatorMap(); | ||
} | ||
|
||
private void setOperatorMap(){ | ||
Arrays.stream(Operator.values()) | ||
.forEach(op -> operatorMap.put(op.operator, op)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 정도 로직은 분리하지 않아도 될 것 같아요
private final static LinkedHashMap<Integer, CalculationResult> results = new LinkedHashMap<>(); | ||
|
||
public void save(CalculationResult result) { | ||
results.put(results.size() + 1, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재는 데이터 삭제 로직이 없지만, 사이즈가 변경되고 추가되면서 대체되는 데이터가 있을 것 같아요
num.append(s); | ||
} | ||
} | ||
if(!num.toString().equals("")){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(!num.toString().equals("")){ | |
if(!num.isEmpty()){ |
public static boolean isValidChoice(String input){ | ||
if (input.length() != MENU_INPUT_LENGTH) return false; | ||
Character firstChar = input.charAt(FIRST_INDEX); | ||
return Arrays.stream(Menu.values()).filter(m -> m.getCommand() | ||
.equals(firstChar)).count() == 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 로직은 Menu
에 위치하는 것이 좋겠네요
void confirm(){ | ||
Operation operation = new Operation(); | ||
|
||
int a = 10; | ||
int b = -5; | ||
|
||
operation.getOperatorMap().entrySet() | ||
.forEach(e -> { | ||
System.out.print(a + e.getKey() + b + " = "); | ||
System.out.println(operation.calculate(a, e.getKey(), b)); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 테스트는 실패하고, 검증 로직이 존재하지 않네요. 성공하도록 수정하고 검증 로직도 추가해주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 잘하신 것 같구 테스트는 꼼꼼히 짜셔서 좋았던 것 같아요!
다만 네이밍하실 때 조금 더 신경 쓰시면 좋을 것 같아요! 타입을 정하실 때도 그렇구요!
package calculator.io; | ||
|
||
public interface Input { | ||
String getChoice(String s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s
라는 변수명은 직관적이지 않은 것 같아요! interface는 설계도에 해당하는 데 설계를 잘 파악할 수 있도록 네이밍을 잘 해주는 것이 중요한 것 같아요~ 여유가 된다면 주석까지 작성해도 좋아요
|
||
|
||
private final String operator; | ||
private final BiFunction<Integer, Integer, Integer> expression; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
나눗셈 결과가 정확하게 나오나요? 3 / 2 하면 1이 나올 것 같아요!
DIVIDE("/", (num1, num2) -> num1 / num2, HIGH); | ||
|
||
|
||
private final String operator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operator.operator
는 의미를 파악하기 너무 어려운 네이밍인 것 같아요! 이렇게 클래스네이밍과 동일한 field는 안 쓰시는 게 좋아요!
|
||
public class PostfixCalculator implements BasicCalculator { | ||
|
||
private static final String OPERATOR_REGEX = "[-*/+]"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 이런 형태는 정규표현식보다는 Set으로 가능한 것들을 정의하고 contain한 지 체크할 수도 있을 것 같아요!
해당 정규표현식은 가능한 것들의 나열과 같아서~
} | ||
else{ | ||
calcStack.push(s); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | |
else{ | |
calcStack.push(s); | |
} | |
} else{ | |
calcStack.push(s); | |
} |
큰 차이는 없지만 일반적으로는 이렇게 많이 쓰는 것 같아요!
if(s.matches(OPERATOR_REGEX)){ | ||
if(!num.isEmpty()){ | ||
postfix.add(num.toString()); | ||
num = new StringBuilder(); | ||
} | ||
if (opStack.isEmpty()) opStack.push(s); | ||
else { | ||
if (Operation.getOperator(opStack.peek()).isPrioritySameOrGreater(Operation.getOperator(s))) { | ||
postfix.add(opStack.pop()); | ||
} | ||
opStack.push(s); | ||
} | ||
} else if (s.matches(OPERAND_REGEX)) { | ||
num.append(s); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if, else가 복잡하게 구성되어 있는 데 함수로 나누고 early return 방식을 이용해서 else 문을 최소화 해보세요!
import java.util.concurrent.ConcurrentHashMap; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
|
||
public class CalculationRepository { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repository도 추상화를 통해 현재의 저장소(In memory) 말고 다양한 방식의 저장소로 유연하게 저장할 수 있을 것 같아요!
private static ByteArrayOutputStream outputMessage; | ||
private static final CalculatorConsole console = new CalculatorConsole(); | ||
private static CalculationRepository repository; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static 필드로 정의하신 이유가 있을까요?
void validateWrongCalculation(Integer a, String operator, Integer b, Integer result) { | ||
Integer calculated = operation.calculate(a, operator, b); | ||
Assertions.assertNotEquals(result, calculated); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
불필요한 줄바꿈은 삭제! 🔪
try { | ||
Integer calculated = operation.calculate(a, operator, b); | ||
} catch (RuntimeException e){ | ||
Assertions.assertEquals("올바른 피연산자가 아닙니다.", e.getMessage()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try-catch
말고도 Assertions.catchThrowable
을 통해서 던져지는 객체들을 잡아서 검증할 수도 있어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전체적으로 잘하신 것 같구 테스트는 꼼꼼히 짜셔서 좋았던 것 같아요!
다만 네이밍하실 때 조금 더 신경 쓰시면 좋을 것 같아요! 타입을 정하실 때도 그렇구요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
예빈님 리뷰 남겼습니다.
toString을 직접 호출하는 경우가 많은데요, toString은 보통 직접 호출하는 용도로 사용하지 않아요.
호출하지 않도록 코드를 변경해보는게 좋을 것 같아요
public Integer mapCalculate(String operator, Integer num1, Integer num2) { | ||
if(Operation.getOperator(operator) == DIVIDE && num2 == 0){ | ||
throw new ArithmeticException(INVALID_OPERAND); | ||
} | ||
return getOperator(operator).expression.apply(num1, num2); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클라이언트 코드에서 operator
를 통해 Operator
인스턴스를 찾고 계산을 하는 방식이 더 좋을 것 같네요
그렇다면 Operation 클래스에 있는 getOperator
관련 로직은 Operator
에 위치해야 할 것 같아요
private static boolean isEvenNumber(AtomicInteger index) { | ||
return index.getAndIncrement() % 2 == 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
index가 짝수인지 확인하는 것과 index를 증가하는 로직이 함께 있어서 분리할 필요가 있을 것 같아요.
void clearStore(){ | ||
results.clear(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default 접근 제어자를 사용하신 이유가 있을까요?
private static Map<Integer, CalculationResult> results; | ||
private static AtomicInteger idCounter; | ||
|
||
|
||
public CalculationRepository() { | ||
results = new ConcurrentHashMap<>(); | ||
idCounter = new AtomicInteger(0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자를 호출할 때 마다 클래스 멤버를 초기화 하면, 외부에서 생성자를 호출할 때마다 데이터가 초기화될 수 있겠군요
public Map<Integer, CalculationResult> findAll() { | ||
return results; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📌 계산기 TDD, OOP로 만들기
다양한 표현 수식에 대한 사칙 연산을 수행하는 콘솔 기반의 계산기를 TDD와 OOP를 기반으로 구현해내는 프로젝트
👩💻 요구 사항과 구현 내용
전체적인 클래스 관계 구조도
기능 구현
✅ 피드백 반영사항
첫번째 PR 반영사항
1. CalculatorApp
Calculator
에 주입합니다.2. 다양한 형태의 Converter 구현
ExpressionConverter
인터페이스로 추상화를 하고, 현재 계산기 프로그램에서는 중위 표현식을 후위 표현식으로 변환하는InfixToPosfixConverter
클래스로 구현하여 동적으로 적절한 컨버터가 선택되도록 하였습니다.convert()
메서드에서 String 형태의 expression을 변환하여 피연산자와 연산자의 리스트 형태의 ArrayList 타입으로 수식을 변환합니다.3. Calculator 추상화
calculate()
추상 메소드를 포함하는BasicCalculator
인터페이스로 추상화하고, 구체적으로 후위 표현 수식을 계산하는PostfixCalculator
클래스로 구체화하였습니다.4. Validation
validateChoiceInput()
,validateExpression()
의 메서드는 각각 Menu 클래스 내부,CalculatorValidator
클래스 내부로 이동하였습니다.isValidInputMenu()
,isValidExpression()
으로 변경하였습니다.5. CalculationRepository - ConcurrentHashMap과 Atomic Variable을 통한 ID값 관리로 멀티 쓰레드 환경에서의 동시성 문제 고려
6. ParameterizedTest 기반 유닛 테스트
7. 상수 관리
8. toString의 쓰임
CalculationRepository
에 객체 저장 시에CalculationResult
자체를 매개 변수로 넣어주도록 변경하였습니다.9. Operation
Operation
객체를 싱글톤으로 생성하도록 LazyHolder의 방식으로 구현하였습니다.OperatorMap
을 초기화하는Operation
클래스의 객체가 필수적으로 생성되어야 하는데, Converter과 Calculator 모두 Operation에 의존성을 띄고 있어 설계 리팩토링을 진행해야 할 것 같습니다..✅ PR 포인트 & 궁금한 점
validateChoiceInput()
,validateExpression()
등) 의 결과값을 테스트하고자 기존 private 지정자에서 public 지정자로 바꾸었는데, 캡슐화 원칙을 지키면서도 각 기능 단위로 단위 테스트 코드를 작성하는 방법이 있을까요?