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

자동차 경주 1단계 미션 리뷰 수정 및 2단계 문자열 계산 소스 생성 #5173

Open
wants to merge 1 commit into
base: seojigyeong
Choose a base branch
from
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
137 changes: 137 additions & 0 deletions src/main/java/study/StringAddCalculator.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
package study;

import javafx.css.Match;

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class StringAddCalculator {

private static final StringAddCalculator instance = new StringAddCalculator();
public static int ZERO = 0;
public static String COMMA = ",";
public static String COLON = ":";

public int splitAndSum(String input) {

int result = ZERO;

// 1. 빈 문자열 또는 null 값을 입력할 경우 0을 반환해야 한다.
Copy link

Choose a reason for hiding this comment

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

주석은 보통 함수가 의미를 충분히 전달할 수 없는 이름이거나, 상황이나 조건에 따라 성능이 달라지거나 외부적인 요인등 함수 시그니처로 충분히 설명이 되지 않고 인수인계가 필요한 경우 작성하는 편이에요.
그 외에는 불필요한 주석은 함수명과 중복되어 의미의 혼동 혹은 중복을 야기하며 가독성을 떨어트리는 편입니다.

if(_checkValidNull(input)) {
Copy link

Choose a reason for hiding this comment

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

자바 코드 컨벤션을 맞춰주시면 좋을 것 같습니다! 함수명 혹은 변수명등 모두 언더바로 시작하지 않고 알파벳 소문자로 시작하도록 가이드하고 있습니다.

참고: https://naver.github.io/hackday-conventions-java/#identifier-char-scope

return result;
}

// 2. 숫자 하나를 문자열로 입력할 경우 해당 숫자를 반환한다.

if(_isNumeric(input)) {
result = _parseInt(input);
Copy link

Choose a reason for hiding this comment

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

숫자 하나일 경우, 아래 다음 로직들을 탈 필요가 없을 것 같지 않나요?

Suggested change
result = _parseInt(input);
return parseInt(input);

}


// 3. 숫자 두개를 컴마(,)구분자로 입력할 경우 두 숫자의 합을 반환한다.
// 4. 구분자를 컴마(,) 이외에 콜론(:)을 사용할 수 있다.
if(_containsCharacter(input, COMMA) || _containsCharacter(input, COLON)) {
result =_sumNumbericSplitByCommaOrColon(input);
}

// 5. "//"와 "\n" 문자 사이에 커스텀 구분자를 지정할 수 있다.

Matcher m = _containsCharacter(input);
if(m != null) {
result = _sumNumbericSplitByCharacter(input, m);
}

return result;

}

private void _checkMinusNumeric(int input) {
if(input < 0) {
throw new RuntimeException();
}

}
private boolean _checkValidNull(String input) {
if(input == null ) {
return true;
}
if(input.isEmpty()) {
return true;
}
return false;
Comment on lines +55 to +61
Copy link

Choose a reason for hiding this comment

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

Suggested change
if(input == null ) {
return true;
}
if(input.isEmpty()) {
return true;
}
return false;
return `null인가?` and `공백인가?`;

여러 조건들을 묶을 수 없을까요?

}

private int _parseInt(String input){

int inputInteger = ZERO;
if(!_isNumeric(input)) {
return inputInteger;
}

inputInteger = Integer.parseInt(input);

/**
* 6. 음수 체크
* */
_checkMinusNumeric(inputInteger);

return inputInteger;
}

private boolean _isNumeric(String input) {
return Pattern.matches("-?\\d+", input);
}

private boolean _containsCharacter(String input, String Character) {
// 문자열에 컴마가 포함되어 있는지 확인
if(COMMA.equals(Character)) {
return input.contains(COMMA);
} else if(COLON.equals(Character)) {
return input.contains(COLON);
}
return false;
}

private int _sumNumbericSplitByCommaOrColon(String input) {
int sumResult = 0;
String[] numbers = input.split(COMMA);
for(String number : numbers) {
if (_isNumeric(number)) {
sumResult += _parseInt(number);
} else {
if(_containsCharacter(number, COLON)) {
String[] numbersColon = number.split(COLON);
for(String numberColon : numbersColon) {
sumResult += _parseInt(numberColon);
}
}
}
}

return sumResult;
}
Comment on lines +95 to +112
Copy link

Choose a reason for hiding this comment

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

  • 하나의 함수가 현재 문자열 분리, 변환, 합계등의 다양한 로직들을 책임지고 있습니다. 즉 테스트하기 힘든 함수로 보이네요. private함수인만큼 화이트박스로 테스팅이 필요한데, 해당 반복,분문에 대한 모든 검증 조건을 테스트하기 너무 까다롭지 않을까요 🤔
  • 반복문 > 분기문 > 분기문 > 반복문 이처럼 너무 깊은 indent가 이뤄지고 있어요. 객체 지향 생활체조 원칙에서는 indent를 2로 줄여보라는 말을 하고 있는데 이 로직도 할 수 있지 않을까요? 보통 indent가 길어진다는건 하나의 함수에서 너무 많은 책임을 가져간다는 의미와 일맥상통합니다.


private Matcher _containsCharacter(String input) {
Matcher m = Pattern.compile("//(.)\n(.*)").matcher(input);
if (m.find()) {
return m;
} else {
return null;
}
}
Comment on lines +114 to +121
Copy link

Choose a reason for hiding this comment

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

  • if-else는 안티패턴입니다 (참고)
  • Pattern을 함수 호출시마다 새로 생성하고 있는데, Pattern과 같은 객체는 생성비용이 상당히 큰 편입니다. 그에 비해 재사용성은 높은 편이구요. 그렇다면 정적 필드로 만들어서 재사용하도록 하는건 어떨까요? (참고)

private int _sumNumbericSplitByCharacter(String input, Matcher m) {
int sumResult = 0;

String customDelimiter = m.group(1);
String[] tokens= m.group(2).split(customDelimiter);
Comment on lines +125 to +126
Copy link

Choose a reason for hiding this comment

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

1과 2가 무엇을 의미하는지 알기 쉬울까요? 매직넘버는 상수로 분리해주면 좋을 것 같습니다

for(String token : tokens) {
sumResult += _parseInt(token);
}

return sumResult;
}

public static StringAddCalculator getInstance() {
return instance;
}
Comment on lines +134 to +136
Copy link

Choose a reason for hiding this comment

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

싱글턴 패턴으로 만드셨군요. 그렇다면 생성자 제한을 통해 해당 함수가 아니면 객체 생성을 못하도록 막는게 어떨까요?
그리고 보통 생성자의 위치가 최상단인만큼 싱글턴 관련 함수들도 최상단에 있는게 가독성을 높힐 수 있을 것 같아요

}
7 changes: 7 additions & 0 deletions src/test/java/study/SetTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ public void setUp() {
numbers.add(3);
}

@Test
@DisplayName("set 자료구조 중복 제거 검사")
public void contains1() {

assertThat(numbers.size() == 3).isTrue();
}

@ParameterizedTest
@ValueSource(ints = {1, 2, 3})
@DisplayName("특정 값이 Set에 존재하는지 확인하는 테스트")
Expand Down
52 changes: 52 additions & 0 deletions src/test/java/study/StringAddCalculatorTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package study;

import org.junit.jupiter.api.Test;

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

public class StringAddCalculatorTest {

StringAddCalculator stringAddCalculator = StringAddCalculator.getInstance();

@Test
public void splitAndSum_null_또는_빈문자() {

int result = stringAddCalculator.splitAndSum(null);
assertThat(result).isEqualTo(0);

result = stringAddCalculator.splitAndSum("");
assertThat(result).isEqualTo(0);
}

@Test
public void splitAndSum_숫자하나() throws Exception {
int result = stringAddCalculator.splitAndSum("1");
assertThat(result).isEqualTo(1);
}

@Test
public void splitAndSum_쉼표구분자() throws Exception {
int result = stringAddCalculator.splitAndSum("1,2");
assertThat(result).isEqualTo(3);
}

@Test
public void splitAndSum_쉼표_또는_콜론_구분자() throws Exception {
int result = stringAddCalculator.splitAndSum("1,2:3");
assertThat(result).isEqualTo(6);
}

@Test
public void splitAndSum_custom_구분자() throws Exception {
int result = stringAddCalculator.splitAndSum("//;\n1;2;3");
assertThat(result).isEqualTo(6);
}

@Test
public void splitAndSum_negative() throws Exception {
assertThatThrownBy(() -> stringAddCalculator.splitAndSum("-1,2,3"))
.isInstanceOf(RuntimeException.class);
}

}
14 changes: 6 additions & 8 deletions src/test/java/study/StringTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand All @@ -19,14 +21,11 @@ public void split1(String word) {
}


@Test
@ParameterizedTest
@DisplayName("특정 문자열 (괄호) 제거")
public String split2(String input) {

String result = removeParentheses(input);

return result;
// assertThat(result).contains("1,2");
@CsvSource(value = {"(1,2):1,2"}, delimiter = ':')
public void split2(String input, String expected) {
assertThat(removeParentheses(input)).isEqualTo(expected);
}

private String removeParentheses(String input) {
Expand All @@ -38,7 +37,6 @@ private String removeParentheses(String input) {
@DisplayName("제한 위치 외 문자열 검색 에러")
public void split3() {

// 범위를 벗어나는 위치의 문자 가져오기 - StringIndexOutOfBoundsException 예상
assertThrows(StringIndexOutOfBoundsException.class, () -> {
char result = ABC.charAt(5);
});
Expand Down