-
Notifications
You must be signed in to change notification settings - Fork 0
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
문자열 계산기 피드백 반영 #1
base: mission01
Are you sure you want to change the base?
Changes from all commits
c59e3c5
32219cf
9361646
4ff0b01
d725677
1d2b270
863533f
ff07771
ca8f51b
a4d2560
e59a1fc
ef5204e
d6f72da
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
import java.util.Arrays; | ||
import java.util.LinkedList; | ||
import java.util.List; | ||
import java.util.Queue; | ||
import java.util.stream.Collectors; | ||
|
||
public class Calculator { | ||
|
||
public static final int EVEN_NUMBER = 2; | ||
|
||
private final String DELIMETER = " "; | ||
|
||
private final Input input; | ||
|
||
private final Output output; | ||
|
||
private final Operators operators; | ||
|
||
private final InputNumbers inputNumbers; | ||
|
||
public Calculator(Input input, Output output, Operators operators, InputNumbers inputNumbers) { | ||
this.input = input; | ||
this.output = output; | ||
this.operators = operators; | ||
this.inputNumbers = inputNumbers; | ||
} | ||
|
||
public void calculate() { | ||
String formula = input.inputFormula(); | ||
|
||
List<String> splittedStrings = Arrays.stream(formula.split(DELIMETER)) | ||
.collect(Collectors.toList()); | ||
|
||
validateInputFormula(splittedStrings); | ||
|
||
Queue<String> queue = new LinkedList<>(splittedStrings); | ||
for (int i = 0; i < splittedStrings.size(); i++) { | ||
separateQueueData(queue, i); | ||
} | ||
|
||
int result = operators.operateAll(inputNumbers.convertToList()); | ||
|
||
output.showResult(result); | ||
Comment on lines
+29
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 사실 이 부분을 어떻게 나눠야 할지 고민이 많았었는데.. 크흠 |
||
} | ||
|
||
private void validateInputFormula(List<String> splitStrings) { | ||
if (isEven(splitStrings.size())) { | ||
throw new IllegalArgumentException("계산식이 올바르지 않습니다."); | ||
} | ||
} | ||
|
||
private void separateQueueData(Queue<String> queue, int position) { | ||
if (isEven(position)) { | ||
inputNumbers.addInputNumber(queue.poll()); | ||
} | ||
operators.addOperator(queue.poll()); | ||
} | ||
|
||
private boolean isEven(int number) { | ||
return (number % EVEN_NUMBER) == 0; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
import java.util.Scanner; | ||
|
||
public class ConsoleInput implements Input{ | ||
|
||
private final String REQUEST_INPUT_MESSAGE = "계산식을 입력해주세요"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OUTPUT에서는 메서드 안에서 출력값을 표기하셨는데, INPUT에서는 빼신 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Input에서 메시지를 밖으로 뺸 이유는 관리하기 편하려고 뻈었습니다. ㅎㅎ |
||
|
||
private Scanner scanner = new Scanner(System.in); | ||
|
||
@Override | ||
public String inputFormula() { | ||
System.out.println(REQUEST_INPUT_MESSAGE); | ||
return scanner.nextLine(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
public class ConsoleOutput implements Output{ | ||
|
||
public static final String RESULT_MESSAGE = "결과 = "; | ||
|
||
@Override | ||
public void showResult(int result) { | ||
System.out.println(RESULT_MESSAGE + result); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
public interface Input { | ||
|
||
String inputFormula(); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
public class InputNumber { | ||
|
||
private int number; | ||
|
||
public InputNumber(String numberStr) { | ||
validateNumberStr(numberStr); | ||
number = Integer.parseInt(numberStr); | ||
} | ||
|
||
public int getNumber() { | ||
return number; | ||
} | ||
|
||
private void validateNumberStr(String numberStr) { | ||
if (numberStr.isBlank() || isNotDigit(numberStr)) { | ||
throw new IllegalArgumentException("잘못된 형식의 숫자가 입력되었습니다."); | ||
} | ||
} | ||
|
||
private boolean isNotDigit(String numberStr) { | ||
return !Character.isDigit(numberStr.charAt(0)); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import java.util.List; | ||
|
||
public interface InputNumbers { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 굳굳굳굳굳!!!! |
||
|
||
void addInputNumber(String inputNumberStr); | ||
|
||
List<Integer> convertToList(); | ||
|
||
int getLength(); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import java.util.ArrayList; | ||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
|
||
public class InputNumbersList implements InputNumbers { | ||
|
||
private List<InputNumber> inputNumbers; | ||
|
||
public InputNumbersList() { | ||
inputNumbers = new ArrayList<>(); | ||
} | ||
|
||
@Override | ||
public void addInputNumber(String inputNumberStr) { | ||
validateInputNumberStr(inputNumberStr); | ||
inputNumbers.add(new InputNumber(inputNumberStr)); | ||
} | ||
|
||
@Override | ||
public List<Integer> convertToList() { | ||
return inputNumbers.stream() | ||
.map(InputNumber::getNumber) | ||
.collect(Collectors.toList()); | ||
} | ||
|
||
@Override | ||
public int getLength() { | ||
return inputNumbers.size(); | ||
} | ||
|
||
private void validateInputNumberStr(String inputNumberStr) { | ||
if (inputNumberStr.isEmpty()) { | ||
throw new IllegalArgumentException("입력된 숫자가 없습니다."); | ||
} | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import java.util.Arrays; | ||
|
||
public enum Operator { | ||
PLUS("+") { | ||
@Override | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 람다식을 활용 해보시면 더 간결해질것 같아요 😃 |
||
public int operate(int preNumber, int postNumber) { | ||
return preNumber + postNumber; | ||
} | ||
}, | ||
MINUS("-") { | ||
@Override | ||
public int operate(int preNumber, int postNumber) { | ||
return preNumber - postNumber; | ||
} | ||
}, | ||
MULTIPLE("*") { | ||
@Override | ||
public int operate(int preNumber, int postNumber) { | ||
return preNumber * postNumber; | ||
} | ||
}, | ||
DIVISION("/") { | ||
@Override | ||
public int operate(int preNumber, int postNumber) { | ||
if (postNumber <= 0) { | ||
throw new ArithmeticException("0 이하로는 나눌 수 없습니다."); | ||
} | ||
return preNumber / postNumber; | ||
} | ||
}; | ||
|
||
private String value; | ||
|
||
Operator(String value) { | ||
this.value = value; | ||
} | ||
|
||
public String getValue() { | ||
return value; | ||
} | ||
|
||
public static Operator findOperator(String operatorStr) { | ||
return Arrays.stream(Operator.values()) | ||
.filter(operator -> operator.getValue().equals(operatorStr)) | ||
.findFirst() | ||
.orElseThrow(() -> new IllegalArgumentException("입력된 연산자가 부정확합니다.")); | ||
} | ||
|
||
public abstract int operate(int preNumber, int postNumber); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 사실 enum을 이렇게 사용하는 것에 대해서 고민이 많았습니다. 깔끔한 코드의 유혹은 벗어나기 힘들더라구요 ㅎㅎ (Enum을 쓰는게 확실히 더 깔끔해져서 ㅎㅎ) |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import java.util.List; | ||
|
||
public interface Operators { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 현재 코드에서 일급 컬렉션을 추상화 하는건 과하다는 생각이 들어요🤔 https://m.blog.naver.com/PostView.naver?isHttpsRedirect=true&blogId=complusblog&logNo=221163007357 |
||
void addOperator(String operatorStr); | ||
|
||
Operator getNextOperator(); | ||
|
||
int operateAll(List<Integer> numbers); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. List 대신 InputNumbers를 받아야 할것 같아요 😃 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분은 고민하다가 생각해보니 Operators 자체에 검증된 값(InputNumbers)이 들어오도록 강제해야 하니까 피드백 감사합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 자료구조가 아닌 객체를 받아 이를 활용 해보시면 좋을것 같아요. 👍 |
||
|
||
int getLength(); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
import java.util.Arrays; | ||
import java.util.LinkedList; | ||
import java.util.List; | ||
import java.util.Queue; | ||
|
||
public class OperatorsQueue implements Operators { | ||
|
||
private Queue<Operator> operators; | ||
|
||
public OperatorsQueue() { | ||
operators = new LinkedList<>(); | ||
} | ||
|
||
public Queue<Operator> getOperators() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. operators를 생성자에서 한번에 초기화 하면 어떨까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 원칙상 생성자에서 값을 초기화 하는게 더 안정적이겠네요 ㅎㅎ |
||
return operators; | ||
} | ||
|
||
@Override | ||
public void addOperator(String operatorStr) { | ||
if (operatorStr.isEmpty()) { | ||
throw new IllegalArgumentException("연산자가 없습니다."); | ||
} | ||
operators.add(Operator.findOperator(operatorStr)); | ||
} | ||
|
||
@Override | ||
public Operator getNextOperator() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. public으로 선언된 이유가 테스트를 위해서라면 개선할 필요가 있을것 같아요! |
||
Operator operator = operators.poll(); | ||
if (operator == null) { | ||
throw new RuntimeException("더이상 연산자가 없습니다."); | ||
} | ||
return operator; | ||
} | ||
|
||
@Override | ||
public int operateAll(List<Integer> numbers) { | ||
if (numbers.isEmpty()) { | ||
throw new IllegalArgumentException("계산할 값이 없습니다."); | ||
} | ||
return numbers.stream() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getNextOperator()로 operator를 가져오지 않고, operator의 stream을 열어 연산을 진행해도 될것 같아요! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 여기서 또 stream을 열어버리면 코드가 좀 복잡해보이지 않을까요?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. stream을 두번 열지 않고도 다른 방법이 있을것 같아요! |
||
.reduce((preNum, postNum) -> getNextOperator().operate(preNum, postNum)) | ||
.orElseThrow(() -> new RuntimeException("계산에 실패하였습니다.")); | ||
} | ||
|
||
@Override | ||
public int getLength() { | ||
return operators.size(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
public interface Output { | ||
|
||
void showResult(int result); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
public class StringCalculatorApplication { | ||
|
||
public static void main(String[] args) { | ||
Input input = new ConsoleInput(); | ||
Output output = new ConsoleOutput(); | ||
Operators operators = new OperatorsQueue(); | ||
InputNumbers inputNumbers = new InputNumbersList(); | ||
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. OCP 원칙을 지키기 위해 Interface를 사용했는데, 변경이나 확장이 필요할 경우 다른 자료구조를 사용한 객체로 갈아끼워 실제 비즈니스 로직에서의 소스변경을 없애는걸 의도했죠. 그래서 OperatorsQueue Class는 내부 로직의 변경이 있더라도 Queue를 사용하는 것은 변함없을 것이라 생각해서 그런데 생각해보니 구현에 사용할 자료구조를 바꾸려 할 경우 새로운 Class를 만드는 것 보다 깔아끼우는 것만 생각하다보니 저런식의 코딩이 된 것 같아요. 피드백 감사합니다~! |
||
|
||
Calculator calculator = new Calculator(input, output, operators, inputNumbers); | ||
calculator.calculate(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
import org.junit.jupiter.api.Test; | ||
|
||
class InputNumberTest { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 테스트에 ParameterizedTest를 사용해 테스트 케이스를 늘려보세요! |
||
private final String VALID_INPUT = "2"; | ||
private final String INVALID_INPUT = "~"; | ||
|
||
@Test | ||
void test_constructor_success() { | ||
InputNumber inputNumber = new InputNumber(VALID_INPUT); | ||
|
||
assertThat(inputNumber.getNumber()).isNotNull(); | ||
assertThat(inputNumber.getNumber()).isEqualTo(Integer.parseInt(VALID_INPUT)); | ||
} | ||
|
||
@Test | ||
void test_constructor_invalid_input() { | ||
assertThatThrownBy(() -> new InputNumber(INVALID_INPUT)) | ||
.isInstanceOf(IllegalArgumentException.class); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
import static org.junit.jupiter.api.Assertions.*; | ||
|
||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class InputNumbersListTest { | ||
|
||
private InputNumbers inputNumbers; | ||
|
||
@BeforeEach | ||
void setUp() { | ||
inputNumbers = new InputNumbersList(); | ||
} | ||
|
||
@Test | ||
void test_addInputNumber_success() { | ||
String validInputNumberStr = "1"; | ||
inputNumbers.addInputNumber(validInputNumberStr); | ||
|
||
assertThat(inputNumbers.getLength()).isEqualTo(1); | ||
} | ||
|
||
@Test | ||
void test_addInputNumber_invalid_input_number_str() { | ||
String invalidInputNumberStr = "+"; | ||
|
||
assertThatThrownBy(() -> inputNumbers.addInputNumber(invalidInputNumberStr)) | ||
.isInstanceOf(IllegalArgumentException.class); | ||
} | ||
|
||
} |
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.
이 메서드는 두가지 이상의 일을 하고 있어요.
메서드들이 하나의 일만 하도록 리팩토링 해보시면 좋을것 같아요.