-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Review #1917
base: main
Are you sure you want to change the base?
Review #1917
Changes from all commits
ad7d30a
a85199c
205c642
f25429a
3f36a4a
d3aab36
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 |
---|---|---|
@@ -1,7 +1,22 @@ | ||
package calculator; | ||
|
||
import calculator.domain.Calculate; | ||
import calculator.domain.Strings; | ||
import calculator.io.CalculatorInput; | ||
import calculator.io.CalculatorOutput; | ||
|
||
public class Application { | ||
public static void main(String[] args) { | ||
// TODO: 프로그램 구현 | ||
CalculatorInput input = new CalculatorInput(); | ||
CalculatorOutput output = new CalculatorOutput(); | ||
|
||
output.printAskingString(); | ||
Strings string = input.getString(); | ||
Calculate calculate = new Calculate(string); | ||
int result = calculate.addNumbers(); | ||
output.printResult(); | ||
System.out.println(result); | ||
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에 없는 이유가 있나요? 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. printResult 함수가 매개변수로 result를 받아서 처리하도록 했으면 더 좋았을 것 같습니다! |
||
|
||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
package calculator.domain; | ||
|
||
public class Calculate { | ||
|
||
private final String[] 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. final 붙이는 것 좋습니다! |
||
private int sum = 0; | ||
|
||
public Calculate(Strings strings) { | ||
this.numbers = strings.distinguisher(); | ||
} | ||
|
||
public int addNumbers() { | ||
|
||
for (String number : 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. 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이 편리한 점이 많네요. stream에 대해 더 자세히 공부 해보겠습니다. |
||
int i = Integer.parseInt(number); | ||
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. try catch문으로 예외처리 해주어야 할 것 같습니다 ㅠㅠ |
||
|
||
if (i <= 0) { | ||
throw new IllegalArgumentException("올바른 값을 입력해주세요"); | ||
} | ||
|
||
sum += i; | ||
} | ||
return sum; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
package calculator.domain; | ||
|
||
public class Strings { | ||
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. 제가 봐도 Strings는 너무 네이밍을 헷갈리게 한 것 같아요... |
||
|
||
public String[] input; | ||
private final String original; | ||
public String distinguish = ",|:"; | ||
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. final이 있으면 좋을 것 같네요 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. 불변 인자니까 final을 붙이는게 더 좋아보이네요 |
||
|
||
public Strings(String input) { | ||
this.input = input.split(""); | ||
this.original = input; | ||
} | ||
|
||
public boolean isCustomExist() { | ||
if (input[0].equals("/") && input[1].equals("/")) { | ||
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. 참고하겠습니다! |
||
int delimiterEndIndex = original.indexOf("\\n"); | ||
if (delimiterEndIndex == -1) { | ||
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. if 문 안에 중첩은 피해주세요 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. if문에 중첩이 많게 되면 코드를 읽는 사람의 입장에서 인지 비용이 올라갈 수 있어요 그래서 if문은 만족하지않은면 return 해버리는 식으로 구현하면 좋습니다! 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. 아 넵 참고하겠습니다! |
||
throw new IllegalArgumentException("잘못된 커스텀 구분자"); | ||
} | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
public String[] distinguisher() { | ||
if(isCustomExist()) { | ||
String temp = input[2]; | ||
String substring = original.substring(5); | ||
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. 커스텀 구분자를 잘못 입력하는 것에 대한 예외처리가 좀 더 보완되어야 할 것 같습니다.. |
||
return substring.split(temp); | ||
} | ||
return original.split(distinguish); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
package calculator.io; | ||
|
||
import calculator.domain.Strings; | ||
import camp.nextstep.edu.missionutils.Console; | ||
|
||
public class CalculatorInput { | ||
|
||
public CalculatorInput() { | ||
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. 여기선 의미없는 코드 같네요.. |
||
} | ||
|
||
public Strings getString() { | ||
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.
|
||
String input = Console.readLine(); | ||
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. Console의 자원회수는 어떻게 되어야할까요? 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. Console 같은 IO 클래스들은 자원회수를 해주어야합니다. close를 어디서 처리해줘야할까? |
||
|
||
return new Strings(input); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package calculator.io; | ||
|
||
public class CalculatorOutput { | ||
|
||
public void printAskingString() { | ||
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. AskingString 이렇게 자료구조가 함수명에 들어가는데 네이밍에 대해서 java 한번 공부해보면 좋을것 같아요 (정답은 없긴해요!) |
||
System.out.println("덧셈할 문자열을 입력해 주세요."); | ||
} | ||
|
||
public void printResult() { | ||
System.out.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.
string이라고 네이밍했을 때 한달 뒤에 주변 코드를 읽지 않고 무슨 변수인지 알 수 있을까요?
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.
확실히 힘들 것 같네요. 네이밍에도 신경을 써야겠습니다