Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/main/java/calculator/Calclator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package calculator;

public class Calclator {
Spliter spliter = new Spliter();
Copy link
Contributor

Choose a reason for hiding this comment

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

여기서 선언한 Spliter가 새로 바뀌거나 한다면 위처럼 가변 참조 필드로 두어도 되겠지만, 그것이 아니기 때문에 private final 걸어주면 좋을것같아요

Copy link
Author

Choose a reason for hiding this comment

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

혹시 private final로 걸어야하는부분이 spliter 인가요?ㅠ

public int sum(String[] splitedValue) {
int result = 0;
for (int i = 0; i < splitedValue.length; i++) {
result += spliter.parseToInt(splitedValue[i]);
}
return result;
}

}
29 changes: 29 additions & 0 deletions src/main/java/calculator/CalculatorApplication.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import calculator.Calclator;
import calculator.Spliter;

import java.util.Scanner;


public class CalculatorApplication {
public static void main(String[] args) {
int result = 0;
System.out.println("숫자들을 입력하세요 : ");
Scanner scanner = new Scanner(System.in);
String inputValue = scanner.nextLine();
if (inputValue.isEmpty()) {
System.out.println(result);
return;
}

Spliter spliter = new Spliter();
String[] splitedValue = spliter.splitValue(inputValue);

System.out.println(splitedValue);
Calclator calclator = new Calclator();
result = calclator.sum(splitedValue);

System.out.println(result);
Comment on lines +9 to +25
Copy link
Contributor

Choose a reason for hiding this comment

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

result라는 변수가 꼭 필요한가 싶습니다.

Suggested change
int result = 0;
System.out.println("숫자들을 입력하세요 : ");
Scanner scanner = new Scanner(System.in);
String inputValue = scanner.nextLine();
if (inputValue.isEmpty()) {
System.out.println(result);
return;
}
Spliter spliter = new Spliter();
String[] splitedValue = spliter.splitValue(inputValue);
System.out.println(splitedValue);
Calclator calclator = new Calclator();
result = calclator.sum(splitedValue);
System.out.println(result);
System.out.println("숫자들을 입력하세요 : ");
Scanner scanner = new Scanner(System.in);
String inputValue = scanner.nextLine();
if (inputValue.isEmpty()) {
System.out.println(0);
return;
}
Spliter spliter = new Spliter();
String[] splitedValue = spliter.splitValue(inputValue);
System.out.println(splitedValue);
Calclator calclator = new Calclator();
System.out.println(calclator.sum(splitedValue));

Comment on lines +8 to +25
Copy link
Member

Choose a reason for hiding this comment

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

main 메서드길이를 최대 10라인이 되도록 수정해주세요!
저희 이번 미션 프로그래밍 요구사항에 포함되어 있었습니다 ㅎㅎ

}


}
53 changes: 53 additions & 0 deletions src/main/java/calculator/Spliter.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package calculator;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Spliter {
Copy link
Contributor

Choose a reason for hiding this comment

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

https://88240.tistory.com/440
참고하세요 :)

Copy link

@csbsjy csbsjy Mar 4, 2020

Choose a reason for hiding this comment

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

가변필드를 가지고 있지 않고 Util 성을 띄고있는 것 같아 static 클래스로 만들어줘도 좋을 것 같습니다 ! 그렇게 하면 Main과 Calculator 에서 불필요하게 두번씩 생성할 필요도 없을 것 같습니다!

Copy link
Author

Choose a reason for hiding this comment

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

네 static으로 바꿨습니다. 감사합니다~


public static final String DELIMITER1 = "[,|:]";
public static final String CUSTOMER_DELIMITER = "//(.)\n(.*)";
Comment on lines +7 to +8
Copy link
Member

Choose a reason for hiding this comment

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

해당 상수들이 public하게 공개되어있을 필요가 없을것 같아여!


public String[] splitValue(String value) {
Matcher m = Pattern.compile(CUSTOMER_DELIMITER).matcher(value);
Copy link

Choose a reason for hiding this comment

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

여기서 Pattern.complie 은 항상 같은 Pattern 을 가리키잖아요?
Pattern 은 생성 비용이 높기 때문에 상수로 초기화시키고 사용하는 것이 좋다고 하더라구요!

Suggested change
Matcher m = Pattern.compile(CUSTOMER_DELIMITER).matcher(value);
private static final Pattern CUSTOM_PATTERN = Pattern.compile(CUSTOMER_DELIMITER);

아, 그리고 이 클래스의 멤버변수는 private 이어도 될 것 같습니다! Splitter 클래스만 알고 있어도 되는 변수로 보여요!

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Matcher 객체의 변수를 m으로 사용하고 있네요!
축약은 코드를 읽는 사람들에게 혼동을 줄 수 있어요!

System.out.println(m.find());
if(m.find()){
String customDelimiter = m.group(1);
String tokens =m.group(2);
String[] splitedValue = tokens.split(CUSTOMER_DELIMITER);
System.out.println(splitedValue[0]+"custom");
Copy link

Choose a reason for hiding this comment

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

System.out 은 테스트용인건가요!?

Copy link
Author

Choose a reason for hiding this comment

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

넵 지웠습니다. 감사해요

System.out.println(splitedValue[1]+"custom");
System.out.println(splitedValue[2]+"custom");
}

String[] splitedValue = value.split(DELIMITER1);
System.out.println(splitedValue[0]);
System.out.println(splitedValue[1]);
System.out.println(splitedValue[2]);
return splitedValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

이렇게 인덱싱을 했을때 outofbound 에러가 나지는 않을지 예외케이스를 생각해보세요

}

public int parseToInt(String splitedValue) {
if (isNumberic(splitedValue)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isNumberic과 아래 코드에서 parseInt를 두번 체크하는데 다른 방식을 고려해보세요

int number = Integer.parseInt(splitedValue);
checkPositve(number);
return number;
}
throw new IllegalArgumentException("양수로 된 숫자만 입력하세요.");
Copy link

@csbsjy csbsjy Mar 4, 2020

Choose a reason for hiding this comment

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

isNumeric 의 catch 문에서 throw 하는 것은 어떨까요?? return false를 굳이 하지 않아도 될 것 같아요 ! isNumeric ParseInt 중복 부분을 해결하면 같이 해결될 것 같습니다 ~

}

public void checkPositve(int parseInt) {
if (parseInt > 0) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

여기있는 return 부분이 불필요해 보입니다. if 안의 조건문을 변경해보세요

Copy link
Author

Choose a reason for hiding this comment

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

네 변경하였습니다.

}
throw new IllegalArgumentException("음수가 아닌 양수만 입력하세요.");
}

public boolean isNumberic(String splitedValue) {
try {
Integer.parseInt(splitedValue);
return true;
} catch (IllegalArgumentException e) {
return false;
}
Comment on lines +46 to +51
Copy link
Member

Choose a reason for hiding this comment

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

이펙티브자 자바3판 아이템 69 예외는 진짜 예외 상황에만 사용하라

한번 검색해서 참고해 보시면 좋을것 같아요!
저는 지금 이 코드가 제어 흐름용 exception 이라고 생각해요!

}
}
Empty file removed src/main/java/calculator/empty.txt
Empty file.
20 changes: 20 additions & 0 deletions src/test/java/calculator/CalclatorTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package calculator;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.*;
import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;



class CalculatorTest {
Spliter spliter = new Spliter();
Calclator calclator = new Calclator();
@Test
@DisplayName("올바른 계산")
void sum() {
String[] splitedValue = {"1","2","5"};
assertThat(calclator.sum(splitedValue)).isEqualTo(8);
}
}
34 changes: 34 additions & 0 deletions src/test/java/calculator/SpliterTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package calculator;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;
import static org.junit.jupiter.api.Assertions.*;
import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;

class SpliterTest {
Spliter spliter = new Spliter();
@Test
void splitValue() {
// String inputValue = new String("1,2:3");
// assertThat(spliter.splitValue(inputValue)).isEqualTo(["1", "2", "3"]);
}
Comment on lines +11 to +15
Copy link
Member

Choose a reason for hiding this comment

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

불필요한 테스트는 제거해주세요 ㅎㅎ

Copy link
Member

Choose a reason for hiding this comment

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

추가적으로 기본식이 입력 되었을 때와 커스텀 구분자가 들어간 식에 대한 계산 테스트가 존재하지 않네요!
만약 해당 테스트를 작성하기 힘들다면 작성하게 힘들어지는 부분을 찾고 리팩토링을 해보는게 좋을것 같아요!
참고로 커스텀 구분자는 \n때문에 테스트 작성하기가 힘들어 질수도 있어요 이때는 @ParameterizedTest@MethodSource를 이용해 보면 좋을것 같아요!


@Test
@DisplayName("숫자가 아닐 때 테스트")
void parseToInt() {
String splitedValue = " ";
assertThatThrownBy(() -> spliter.parseToInt(splitedValue))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("양수로 된 숫자만 입력하세요.");
}
@Test
@DisplayName("음수를 입력했을 때 테스트")
void checkPositve() {
int negative = -1;
assertThatThrownBy(() -> spliter.checkPositve(negative))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("음수가 아닌 양수만 입력하세요.");
}

}
Empty file removed src/test/java/calculator/empty.txt
Empty file.