[보민] 문자열 덧셈 계산기 #12

bominjang · 2020-03-03T14:39:44Z

No description provided.

jyami-kim · 2020-03-03T17:05:51Z

src/main/java/calculator/Parser.java

+		return toInt;
+	}
+
+	public void checkTheValueIsPositive(int number){


public으로 열어서 parser.checkThe.. 하면 이 파셔 객체는 checking의 역할도 부여됩니다.

이 메서드를 private으로 막는 것 보다는, 클래스는 그 이름에 맞는 기능을 가져야 하니까 Checking하는 클래스를 생성해서 그 멤버로 static checkTheValueIsPositive 메서드를 만드는 게 나을까요?

모든클래스를 다 분리할 필요는 없다고 생각해요
재사용 가능성이 있고 validate하는 과정이 복잡하고 Parser클래스 내부가 지저분해질 정도로 과하다 싶으면 그때 분리해서 역할을 위임하면 됩니다.

클래스 분리보단 내부에서만 쓰이는 함수니까 private으로 내부에 유출시키지 않는게 좋아보이네요

jyami-kim · 2020-03-03T17:06:11Z

src/main/java/calculator/Calculator.java

+public class Calculator {
+	public int addValues(String[] tokens) {
+		int result=0;
+		Parser parser = new Parser();


https://88240.tistory.com/440
참고

jyami-kim · 2020-03-03T17:12:46Z

src/main/java/calculator/Splitter.java

+	public static final String DEFAULT_DELIMITER = ",|:";
+	public static final String CUSTOM_DELIMITER_REGEX = "//(.)\n(.*)";
+
+	private static Matcher m;


https://88240.tistory.com/440
지금 Mathcer가 static인데, Splitter를 여러 객체로 만들어서 여러 객체에서 동시에 checkDelimeter를 한다면 문제가 생길 수 있습니다. (쓰레드)

또한 사실상 checkDelimeter메서드가 사실상 m의 세터역할을 하고있는데, Splitter 클래스 밖에 다른 클래스에서 Splitter.checkDelimeter()를 하게되면 Splitter의 멤버필드에 접근해서 변경하는 느낌이라 캡슐화가 잘된 코드가 아닙니다.

그래서 m을 쓰는 곳을 확인해봤는데, splitByCustomDemeter 하나뿐인데, 인자로 안넣은 이유가 있나요?

text가 있어야해서 splitByCustomDelemeter 안에 넣었습니다. 인자로 넣는다는 말씀은 메인애플리케이션에서 메서드 호출할 때 인자로 넣으라는 말씀이신가여...?!

build.gradle

csbsjy · 2020-03-04T01:22:51Z

src/main/java/calculator/InputChecking.java

+
+public class InputChecking {
+
+	public static boolean isBlank(String value)


기본 컨벤션은 아래와 같아용!
isBlank(String value){ < ---- 여기 괄호 시작
}

넵!! 수정했습니다.

csbsjy · 2020-03-04T01:25:47Z

src/main/java/calculator/Parser.java

+			checkTheValueIsPositive(toInt);
+			return toInt;
+		} catch (NumberFormatException e) {
+			System.out.println("숫자를 입력하세요.");


throw !
지난면에 System.exit(0) 이 왜 안되는지 물어보셨던거 같은데, 생각해보니까

먼저 일반적인 애플리케이션은 parse Exception 이 터진다고 시스템을 종료하진 않을 것 같아요. 사용하는 측에서 Catch 해서 에러페이지를 노출시키던가 하겠죠?

그리고 System.exit(0) 에서 0 이 종료코드인 것 같은데 맞나요? 저는 잘 몰라서 ㅎㅎ 어쨋든 0 만보고 이 시스템이 뭐때문에 종료되었는지 파악할 수 없어서 문제가 될 것 같습니다 :)

네! 에러메세지 노출시키는 걸로 반영하였습니다. 감사합니당

csbsjy · 2020-03-04T01:32:57Z

src/test/java/calculator/InputCheckingTest.java

+	@DisplayName("인자의 값이 빈칸 또는 null 인지 점검한다.")
+	@Test
+	void shouldCheckIsTheInputBlankTest() {
+		String isBlank = "";


#11 (comment)

csbsjy · 2020-03-04T01:34:34Z

src/test/java/calculator/ParserTest.java

+	@Test
+	void shouldCheckPositiveValueTest() {
+		int negativeInt = -1;
+		Parser mockedParser = mock(Parser.class);


파서를 Mocking한 이유가 있을까요?

void type 메서드를 테스트하는 방법을 찾던 중, 이 방법이 나와서 해보았습니다..

음 저도 Mocking 에 대해 잘 알지는 못하지만,
Mocking 은 의존성을 끊어내고 테스트를하거나, 테스트환경/개발환경을 분리시켜 테스트하거나 등등의 경우에 쓰이는 것이지 void type 테스트를 위한 것은 아니지 않을까 싶어요! 저도 잘 모르겠네요 하하

pci2676

전체 줄 정렬 기능을 이용해주세요! ctrl + alt + l 그리고 o

pci2676 · 2020-03-04T05:06:24Z

src/test/java/calculator/StringAddingCalculatorApplicationTest.java

+class StringAddingCalculatorApplicationTest {
+
+	@Test
+	void main() {
+	}
+}


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

pci2676 · 2020-03-04T05:08:58Z

src/test/java/calculator/InputCheckingTest.java

+	void shouldCheckSinglularityTest(String values) {
+		assertThat(inputCheckingTest.checkSinglularity(values)).isEqualTo(true);
+	}


이 테스트 23, 12 일때 실패하네요 ㅎㅎ

한글자일때만 통과해야하는 미션으로 이해했는데 제가 잘못안건가여..?! 그래서 일부러 23,12일때는 실패하는 걸 보여주려고 이렇게 작성했습니다...ㅎ

아하 그렇다면 입력 values 에 따른 결과 확인이 달라져야겠네요!
@ValueSource 말고 @CsvSource 를 이용해서 단언문 결과값을 true로 고정하지말고 매번 달라질수 있도록 해보는건 어떨까요!

pci2676 · 2020-03-04T05:12:53Z

src/test/java/calculator/InputCheckingTest.java

+		assertThat(inputCheckingTest.isBlank(isBlank)).isEqualTo(true);
+		assertThat(inputCheckingTest.isBlank(isNull)).isEqualTo(true);


InputCheckingTest 의 isBlank 메서드는 static 메서드인데 인스턴스를 생성해서 인스턴스에서 호출해서 사용하고 있네요!

항상 참인 단언문은 아래와 같이 바꿀 수 있습니다!

Suggested change

assertThat(inputCheckingTest.isBlank(isBlank)).isEqualTo(true);

assertThat(inputCheckingTest.isBlank(isNull)).isEqualTo(true);

assertThat(inputCheckingTest.isBlank(isBlank)).isTrue();

assertThat(inputCheckingTest.isBlank(isNull)).isTrue();

넵 반영했습니다! 감사합니다.

pci2676 · 2020-03-04T05:14:06Z

src/main/java/calculator/StringAddingCalculatorApplication.java

+		System.out.println("input : ");
+		Scanner scanner = new Scanner(System.in);
+		String inputValue = scanner.nextLine();
+
+		if (InputChecking.isBlank(inputValue)) {
+			System.out.println("값을 입력하세요.");
+			System.exit(0);
+		}
+
+		Parser parser = new Parser();
+		if (InputChecking.checkSinglularity(inputValue)) {
+			System.out.println("result : "+parser.parseSingleValue(inputValue));
+			return;
+		}
+
+		Splitter splitter = new Splitter();
+		String[] tokens = splitter.checkDelimeter(inputValue);
+
+		Calculator calculator = new Calculator();
+		System.out.println("result : "+calculator.addValues(tokens));
+	}


이번 미션의 프로그래밍 요구사항 중 한 메서드의 최대 허용 라인수는 10 라인이랍니다!

넵!! 수정했습니다

pci2676 · 2020-03-04T05:16:27Z

src/main/java/calculator/StringAddingCalculatorApplication.java

+import java.util.Scanner;
+
+public class StringAddingCalculatorApplication {
+	private static int result;


사용되지 않는 변수네요!

반영했습니다!

pci2676 · 2020-03-04T05:22:12Z

src/main/java/calculator/Splitter.java

+
+	private static Matcher m;
+
+	public String[] checkDelimeter(String text){


check 라는 네이밍이 반환값을 가지고 있는게 전 어색해보이네요!
검사의 성격을 띄고 있는 네이밍인것 같아요!

네이밍 더 신경쓰겠습니다.

pci2676 · 2020-03-04T05:23:41Z

src/main/java/calculator/Splitter.java

+	private static Matcher m;
+
+	public String[] checkDelimeter(String text){
+		m = Pattern.compile(CUSTOM_DELIMITER_REGEX).matcher(text);


Pattern.compile()은 내부적으로 새롭게 new Pattern()을 행하고
GC의 대상이 되어 수거되기 때문에 그 비용이 비싼 편입니다.

다음 글을 참고해 보세요!
https://javabom.tistory.com/30

pci2676 · 2020-03-04T05:24:25Z

src/main/java/calculator/Splitter.java

+	}
+
+	private String[] splitByDefaultDelimiter(String text) {
+		return text.split(",|:");


만들어둔 상수를 사용하면 좋을것 같아요!

pci2676 · 2020-03-04T05:26:16Z

src/main/java/calculator/Splitter.java

+		if(m.find()){
+			return splitByCustomDelimeter();
+		}
+		return null;


null을 반환하기 보다 exception을 발생시키는게 더 좋을것 같아요!

pci2676 · 2020-03-04T05:31:00Z

src/test/java/calculator/ParserTest.java

+	void shouldCheckPositiveValueTest() {
+		int negativeInt = -1;
+		Parser mockedParser = mock(Parser.class);
+		doThrow(new IllegalArgumentException()).when(mockedParser).checkTheValueIsPositive(negativeInt);


mock을 사용하지 않고 exception 테스트는 다음과 같이 고칠 수 있을 것 같아요!

Suggested change

doThrow(new IllegalArgumentException()).when(mockedParser).checkTheValueIsPositive(negativeInt);

assertThatThrownBy(() -> new Parser().checkTheValueIsPositive(negativeInt))

.isInstanceOf(IllegalArgumentException.class);

아하 넵 감사합니다 !

그런데 왜 mock을 사용하는 것보다 assertThat사용을 더 권장하시는 지 궁금합니다!

음..!
제가 리뷰를 이상하게 달아놨네여ㅎㅎ
mock을 사용하지마세요! 라는건 아니지만
mockito 의 doThrow 구문은 stub을 하는 역할입니다.
어떤 말이냐면 mock(Parser.class)로 생성한 Parser mockedParser가 어떠한 행동을 할지 강제 할 수 있도록 하는 거에요!
그래서 지금 코드에서는 mockedPaser가 -1을 입력받았을 경우 IlleagalArgumentException을 발생하게 해라! 라고 지정을 한 것입니다.
이러한 mock 라이브러리를 이용한다면 아래와 같은 코드도 통과를 한답니다.

@DisplayName("양수인지 점검") @Test void shouldCheckPositiveValueTest() { int negativeInt = -1; Parser mockedParser = mock(Parser.class); doThrow(new IllegalArgumentException()).when(mockedParser).checkTheValueIsPositive(1); assertThatThrownBy(() -> mockedParser.checkTheValueIsPositive(1)) .isInstanceOf(IllegalArgumentException.class); }

그래서 사실상 테스트가 의미있는 테스트가 아니게 되어버렸어요!

이전에 했던 리뷰를 수정하고 정리하자면.
mockito의 doThrow 는 단언문이 아닙니다. 그래서 위 테스트 코드에는 exception을 검증 하는 코드가 누락되어있습니다!

아하 넵 감사합니다!!!

malibinYun · 2020-03-06T04:30:41Z

src/main/java/calculator/Calculator.java

+public class Calculator {
+	public static int addValues(String[] tokens, Parser parser) {
+		int result = 0;
+		for (int i = 0; i < tokens.length; i++) {


여기는 foreach나 stream을 사용해도 좋다고 생각합니다.
과하다 생각할 순 있지만 익숙해지는데에 연습하는것이 좋아보여요 !

넵! 익숙해져볼게욥...!!

malibinYun · 2020-03-06T04:36:57Z

src/test/java/calculator/InputCheckingTest.java

+	@Test
+	void shouldCheckIsTheInputBlankTest() {
+		String isBlank = "";
+		String isNull = null;


isㅇㅇㅇ 이런 류의 명명은 Boolean 타입의 변수나 이 타입을 리턴하는 메소드에서 쓰이는 것이라,
isNull은 이름이 이상하다고 생각합니다.

chldbtjd2272 · 2020-03-07T11:56:06Z

src/main/java/calculator/InputChecking.java

+
+public class InputChecking {
+
+	public static boolean checkIsTheInputBlank(String value) {


boolean형 함수에서 false가 나올수 없는 함수는 이상한 함수입니다
이런 함수는 void validate하는 함수로
if(value == null || value.isEmpty(){
Exception();
}
요게 더 좋아보이네요
그리고 nullpointerException보다는 IlleganAgumentException이 더 명시적으로 좋은 익셉션인거같습니다

chldbtjd2272 · 2020-03-07T11:57:02Z

src/main/java/calculator/InputChecking.java

+		return true;
+	}
+
+	public static void checkSinglularityAndPrintResult(String inputValue, Parser singleParser) {


인풋값 체크하는 함수에 프린트용 함수가 있는 건 이상합니다

chldbtjd2272 · 2020-03-07T12:04:44Z

src/main/java/calculator/Parser.java

+		try {
+			toInt = Integer.parseInt(stringToInt);
+			checkTheValueIsPositive(toInt);
+			return toInt;


여기선 리턴이 없어도 되겠네요

chldbtjd2272 · 2020-03-07T12:08:13Z

src/main/java/calculator/Parser.java

+		return toInt;
+	}
+
+	public void checkTheValueIsPositive(int number){


모든클래스를 다 분리할 필요는 없다고 생각해요
재사용 가능성이 있고 validate하는 과정이 복잡하고 Parser클래스 내부가 지저분해질 정도로 과하다 싶으면 그때 분리해서 역할을 위임하면 됩니다.

클래스 분리보단 내부에서만 쓰이는 함수니까 private으로 내부에 유출시키지 않는게 좋아보이네요

chldbtjd2272 · 2020-03-07T12:13:29Z

src/main/java/calculator/Splitter.java

+	public static final int DELIMETER_GROUP = 1;
+	public static final int TOKEN_GROUP = 2;
+
+	private Pattern pattern = Pattern.compile(CUSTOM_DELIMITER_REGEX);


해당 객체는 상수로 존재하는게 좋아요
싱글톤 객체로 존재할때 효율이 좋아질거에요. Splitter객체를 생성할때마다 pattern이 생성되니 gc효율은 여전히 안좋아요
static final로 어플리케이션에 단하나만 존재하는 인스턴스로 만들어서 사용하는게 좋아요

chldbtjd2272 · 2020-03-07T12:45:40Z

src/main/java/calculator/StringAddingCalculatorApplication.java

+package calculator;
+
+public class StringAddingCalculatorApplication {
+	public static void main(String[] args) {


객체간의 관계가 어색합니다!
메인 메서드에서 객체간의 관계를 설정하고 있어요.

인풋 받기 -> 체크 -> 파서생성 -> 체킹클래스에서 파서 찍기 ->....

객체간의 관계가 없이 메인안에서 함수의 인자로 넘기거나 상태값도 메인 메서드에서만 유지하는 지역변수로 활용하고 있어서
구현되어져 있는 클래스만 보고는 요구사항을 파악하기 굉장히 힘들어보여요!

지금 클래스의 관계는 전부 static 메서드로 만들어도 표현할 수 있습니다.
이런 관계보단 Calculator라는 클래스안에 결과라는 멤버변수를 두고 Splitter와 Parser를 Calculator내부에서 사용하도록 하는게 어떨까요

Gobukgol · 2020-03-09T17:06:18Z

src/test/java/calculator/InputCheckingTest.java


 import static org.assertj.core.api.Assertions.assertThat;

 class InputCheckingTest {

-
 	@DisplayName("인자의 값이 빈칸 또는 null 인지 점검한다.")
 	@Test
 	void shouldCheckIsTheInputBlankTest() {


Null 또는 빈 문자열 테스트는 junit5의 @ParameterizedTest와 @NullAndEmptySource 로 간단하게 할 수 있어요!

@ParameterizedTest @NullAndEmptySource void name(String isBlankOrNull) { }

이런식으로요! 또 checkIsTheInputBlank 함수는 NullPointerException throw 하니까 assertThat 보다는
assertThatThrownBy 를 이용하면 좋을 것 같아요!

문자열 계산기 완성

9c9dd23

bominjang changed the title ~~[보민] 문자열 계산기 완성~~ [보민] 문자열 덧셈 계산기 Mar 3, 2020

jyami-kim reviewed Mar 3, 2020

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

csbsjy reviewed Mar 4, 2020

View reviewed changes

pci2676 requested changes Mar 4, 2020

View reviewed changes

Test에서 csvSource,Exception 추가 필요함.

9e57c22

malibinYun reviewed Mar 6, 2020

View reviewed changes

chldbtjd2272 reviewed Mar 7, 2020

View reviewed changes

Test에서 막힘

ed96d0b

Gobukgol reviewed Mar 10, 2020

View reviewed changes


		public class InputChecking {

		public static boolean isBlank(String value)

		assertThat(inputCheckingTest.isBlank(isBlank)).isEqualTo(true);
		assertThat(inputCheckingTest.isBlank(isNull)).isEqualTo(true);


		private static Matcher m;

		public String[] checkDelimeter(String text){

	doThrow(new IllegalArgumentException()).when(mockedParser).checkTheValueIsPositive(negativeInt);
	assertThatThrownBy(() -> new Parser().checkTheValueIsPositive(negativeInt))
	.isInstanceOf(IllegalArgumentException.class);


		public class InputChecking {

		public static boolean checkIsTheInputBlank(String value) {

[보민] 문자열 덧셈 계산기 #12

Are you sure you want to change the base?

[보민] 문자열 덧셈 계산기 #12

Uh oh!

Conversation

bominjang commented Mar 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pci2676 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment