Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Review #1917

wants to merge 6 commits into from

Conversation

HarryBae1011
Copy link

No description provided.

Copy link

@sejoon00 sejoon00 left a comment

Choose a reason for hiding this comment

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

전체적으로 책임을 분리하려고 노력한 모습이 보여서 좋았어요.
다음에는 풀리퀘 conversation에 왜 이렇게 설계했는지도 설명해주면 좋을 것 같아요.
또 예외케이스에 대한 테스트 코드가 있으면 좋을 것 같습니다.
수고했어요!


public class CalculatorInput {

public CalculatorInput() {

Choose a reason for hiding this comment

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

기본 생성자를 명시적으로 써준 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

여기선 의미없는 코드 같네요..

public CalculatorInput() {
}

public Strings getString() {

Choose a reason for hiding this comment

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

  1. 클래스에 인스턴스 변수가 없는데 static 으로 하면 좋지 않을까요?
  2. getString이라는 네이밍이 직관적이지 않은 것 같아요

}

public Strings getString() {
String input = Console.readLine();

Choose a reason for hiding this comment

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

Console의 자원회수는 어떻게 되어야할까요?

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 잘 모르겠어요 ㅠㅠ
밑줄에 바로 Console.close(); 를 넣어주면 해결될까요..?

Choose a reason for hiding this comment

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

Console 같은 IO 클래스들은 자원회수를 해주어야합니다.
프로그램이 종료되지 않는 서버 어플리케이션 같은경우 자원이 계속 회수 되지 않으면 문제가 발생할 수 있어요.

close를 어디서 처리해줘야할까?
(여러가지 방법이 잇음)
고민해보면 좋을 것 같아요


public class CalculatorOutput {

public void printAskingString() {

Choose a reason for hiding this comment

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

AskingString 이렇게 자료구조가 함수명에 들어가는데 네이밍에 대해서 java 한번 공부해보면 좋을것 같아요 (정답은 없긴해요!)

CalculatorOutput output = new CalculatorOutput();

output.printAskingString();
Strings string = input.getString();

Choose a reason for hiding this comment

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

string이라고 네이밍했을 때 한달 뒤에 주변 코드를 읽지 않고 무슨 변수인지 알 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

확실히 힘들 것 같네요. 네이밍에도 신경을 써야겠습니다

public boolean isCustomExist() {
if (input[0].equals("/") && input[1].equals("/")) {
int delimiterEndIndex = original.indexOf("\\n");
if (delimiterEndIndex == -1) {

Choose a reason for hiding this comment

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

if 문 안에 중첩은 피해주세요

Copy link
Author

Choose a reason for hiding this comment

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

이건 어떤 부분이 문제일까요?

Choose a reason for hiding this comment

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

if문에 중첩이 많게 되면 코드를 읽는 사람의 입장에서 인지 비용이 올라갈 수 있어요
계속 현재 줄이 어떤 조건이 충족된 상황인지 플러스 중첩안에서 또 하나 플러스가 되는거죠

그래서 if문은 만족하지않은면 return 해버리는 식으로 구현하면 좋습니다!

Copy link
Author

Choose a reason for hiding this comment

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

아 넵 참고하겠습니다!

public String[] distinguisher() {
if(isCustomExist()) {
String temp = input[2];
String substring = original.substring(5);

Choose a reason for hiding this comment

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

커스텀 구분자가 여러개 들어오는 케이스는 처리되었을까요?

Copy link
Author

Choose a reason for hiding this comment

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

커스텀 구분자를 잘못 입력하는 것에 대한 예외처리가 좀 더 보완되어야 할 것 같습니다..

public int addNumbers() {

for (String number : numbers) {
int i = Integer.parseInt(number);

Choose a reason for hiding this comment

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

숫자가 아닌 문자열이 들어오면 어떻게 처리해야할까요

Copy link
Author

Choose a reason for hiding this comment

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

try catch문으로 예외처리 해주어야 할 것 같습니다 ㅠㅠ


public int addNumbers() {

for (String number : numbers) {

Choose a reason for hiding this comment

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

stream을 공부해보는 것도 좋을 것 같아요

Copy link
Author

Choose a reason for hiding this comment

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

stream이 편리한 점이 많네요. stream에 대해 더 자세히 공부 해보겠습니다.

Calculate calculate = new Calculate(string);
int result = calculate.addNumbers();
output.printResult();
System.out.println(result);

Choose a reason for hiding this comment

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

이 출력은 output에 없는 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

printResult 함수가 매개변수로 result를 받아서 처리하도록 했으면 더 좋았을 것 같습니다!

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.

2 participants