-
Notifications
You must be signed in to change notification settings - Fork 8
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
(강상문) StringCalculator 리뷰 요청드립니다. #27
base: sangmoon/main
Are you sure you want to change the base?
Conversation
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 사용
- 클래스의 역할
위 두 가지에 대해서 더 공부해보면 좋을 것 같습니다
src/main/java/Input.java
Outdated
public class Input { | ||
public static String[] readInput(Scanner scanner) { | ||
System.out.print("수식을 입력하세요: "); | ||
return scanner.nextLine().split(" "); | ||
} | ||
} |
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.
- Input은 어떤 역할을 하는 클래스인가요?
- readInput 메소드가 static인 이유가 뭔가요
- Scanner 객체를 매개변수로 받는 이유가 뭔가요
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.
- 사용자의 입력을 받아오는 역할을 하는 클래스입니다.
- 객체를 생성하지 않고 클래스 이름으로 직접 호출 할 수 있게 하려고 사용했었습니다.
- Scanner 객체를 사용하는 다른 클래스가 있을 경우 Scanner 객체를 사용할 수 있게 하려고 사용했었습니다.
src/main/java/Operations.java
Outdated
public class Operations { | ||
public int add(int a, int b) { | ||
return a + b; | ||
} | ||
|
||
public int subtract(int a, int b) { | ||
return a - b; | ||
} | ||
|
||
public int multiply(int a, int b) { | ||
return a * b; | ||
} | ||
|
||
public int divide(int a, int b) { | ||
if (b == 0) { | ||
throw new ArithmeticException("0으로 나눌 수 없습니다."); | ||
} | ||
return a / 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.
Enum을 사용해보세요
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.
Enum 추가하여 수정했습니다.
src/main/java/Operator.java
Outdated
@@ -0,0 +1,30 @@ | |||
public class Operator { | |||
public static int calculate(String[] values) { |
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.
매개변수의 values는 어떤 값을 받아오는 건가요
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.
사용자가 입력한 수식을 공백을 기준으로 분할 하여 입력한 수식을 받아옵니다.
src/main/java/Output.java
Outdated
@@ -0,0 +1,6 @@ | |||
public class Output { | |||
|
|||
public static void printResult(int 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.
print는 예약어입니다. 예약어를 사용하지 않는 메소드명을 사용해주세요.
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.
resultOutput으로 메소드명을 변경했습니다.
add를 addition으로 수정
public static OperatorType fromString(String symbol) { | ||
switch (symbol) { | ||
case "+": | ||
return ADDITION; | ||
case "-": | ||
return SUBTRACT; | ||
case "*": | ||
return MULTIPLY; | ||
case "/": | ||
return DIVIDE; | ||
default: | ||
throw new IllegalArgumentException("잘못된 연산자입니다."); | ||
} |
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.
지정된 연산자를 제외한 다른 기호를 입력했을 경우에 출력하도록 했지만
계산기의 경우 따로 없어도 될 것 같습니다.
switch (operatorType) { | ||
case ADDITION: | ||
result = operations.addition(result, nextNumber); | ||
break; | ||
case "-": | ||
case SUBTRACT: | ||
result = operations.subtract(result, nextNumber); | ||
break; | ||
case "*": | ||
case MULTIPLY: | ||
result = operations.multiply(result, nextNumber); | ||
break; | ||
case "/": | ||
case DIVIDE: | ||
result = operations.divide(result, nextNumber); | ||
break; |
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.
Enum 사용법을 다시 공부해서 적용해봅시다. Switch문을 사용하지 않는 방식으로요.
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.
BiFunction 필드를 활용하여 Switch문 없이 연산을 처리 할 수 있게 수정했습니다.
public class InputView { | ||
public String[] formulaInput() { | ||
Scanner scanner = new Scanner(System.in); | ||
System.out.print("수식을 입력하세요 : "); | ||
return scanner.nextLine().split(" "); | ||
} | ||
} |
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.
FormulaParser 클래스에 문자열을 나누는 역할을 분할했습니다.
} | ||
|
||
public void run() { | ||
String[] values = inputView.formulaInput(); |
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.
calculatorRun으로 수정했습니다.
|
||
public class Operator { | ||
public static int calculate(String[] values) { | ||
Operations operations = new Operations(); | ||
private final Operations operations; | ||
|
||
public Operator() { | ||
this.operations = new Operations(); | ||
} |
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.
OperatorType의 기능을 수정하면서 Operator의 기능과 중복되기에 삭제했습니다.
|
||
public static OperatorType fromString(String symbol) { | ||
switch (symbol) { | ||
case "+": |
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 메소드여야만 하는 이유가 있나요?
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메소드로 선언하면 인스턴스 생성없이 호출 할 수 있어서 사용했습니다.
InputView에 있던 문자열을 나누는 역할 분할
파싱 로직을 FormulaParser를 추가하여 책임 분리
String[] values = inputView.formulaInput(); | ||
public void calculatorRun() { | ||
String formula = inputView.formulaInput(); | ||
String[] values = formulaParser.parse(formula); |
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.
values는 어떤 용도로 사용되는 변수인가요
|
||
public Operator() { | ||
this.operations = new Operations(); | ||
} | ||
|
||
public int calculate(String[] values) { |
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.
현재 푸쉬한 코드에 존재하지 않는 코드입니다.
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.
values 말한 거에요
OperatorType operatorType = OperatorType.fromString(symbol); | ||
result = operatorType.apply(result, nextNumber); |
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 메소드를 사용해야할 이유가 있을까요?
return a / b; | ||
}); | ||
|
||
private static final Map<String, OperatorType> SYMBOL_MAP = new HashMap<>(); |
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.
이걸 필드로 선언한 이유가 있을까요?
데이터 주도 설계가 아닌 도메인 주도 설계를 공부해보세
요청드립니다.