Conversation
| NumberString numberString = new NumberString(inputString); | ||
|
|
||
| String delimiterRegex = delimiters.toRegexString(); | ||
| return Arrays.stream(numberString.getValue().split(delimiterRegex)) |
There was a problem hiding this comment.
numberstring안에서 value를 꺼내서 splite하는 과정대신, numberstring안에서 split한 결과를 리턴하면 좋을것 같네요.
지금은 numberString 객체안의 값을 NumbersExtractor가 제어하는건데, numberString 객체 안에서 제어할 수 있는 로직이라면 안에 담는게 좋을 것 같아서요. (캡슐화)
| Matcher matcher = customDelimiterPattern.matcher(inputString); | ||
| if (matcher.find()) { | ||
| String customDelimiter = matcher.group(1); | ||
| DELIMITER_REGEX = DELIMITER_REGEX + "|" + customDelimiter; |
There was a problem hiding this comment.
| value = numberString; | ||
| return; | ||
| } | ||
| validateString(inputString); |
There was a problem hiding this comment.
위에서 matcher.find()로 통과한상태인데 만약 NumberString이 아니면 어떻게 되나요?
There was a problem hiding this comment.
find를 거쳐서 제대로 값이 들어갔다고 생각을 하는데.. 어떤 문제인지 잘 모르겠습니다.
| assertThat(result).isEqualTo(6); | ||
| } | ||
|
|
||
| @DisplayName("공백 확인") |
| public static final String BLANK = ""; | ||
|
|
||
| public static int calculateWith(String inputString) { | ||
| if (inputString.equals(BLANK)) { |
There was a problem hiding this comment.
inputString.isEmpty() 로 가능할 것 같습니다. BLANK 상수는 없어도 될 것 같아요!
혹시 null 에 대한 검증은 어딨나여??
| public static final String BASIC_DELIMITER_REGEX = ",|:"; | ||
| public static final String CUSTOM_DELIMITER_REGEX = "//(.)\n(.*)"; | ||
|
|
||
| private String DELIMITER_REGEX = BASIC_DELIMITER_REGEX; |
| private String DELIMITER_REGEX = BASIC_DELIMITER_REGEX; | ||
|
|
||
| public Delimiters(String inputString) { | ||
| Pattern customDelimiterPattern = Pattern.compile(CUSTOM_DELIMITER_REGEX); |
There was a problem hiding this comment.
이 패턴은 늘 같은 Pattern 이라 상수로 초기화 해두면 좋을 것 같습니다 !
There was a problem hiding this comment.
| public static final String BLANK = ""; | ||
|
|
||
| public static int calculateWith(String inputString) { | ||
| if (inputString.equals(BLANK)) { |
| //import java.util.regex.Matcher; | ||
| //import java.util.regex.Pattern; | ||
| // | ||
| //public class StringSpliter { |
| private String DELIMITER_REGEX = BASIC_DELIMITER_REGEX; | ||
|
|
||
| public Delimiters(String inputString) { | ||
| Pattern customDelimiterPattern = Pattern.compile(CUSTOM_DELIMITER_REGEX); |
There was a problem hiding this comment.
| Matcher matcher = customDelimiterPattern.matcher(inputString); | ||
| if (matcher.find()) { | ||
| String customDelimiter = matcher.group(1); |
There was a problem hiding this comment.
커스텀 구분자가 더 늘어날 경우 어떠한 문제점이 발생할 수 있을까요??
There was a problem hiding this comment.
어... 음... 늘어난다는 경우가 어떤거죠..?
그리고 늘어난다 해도 잘모르겟습니다 센세..
There was a problem hiding this comment.
&&과 $$ 사이에 커스텀 문자가 들어온다던가 하는 새로운 요구사항이 추가되었을 경우요!
|
|
||
| class DelimitersTest { | ||
|
|
||
| @DisplayName("커스텀 구분자 없을 때") |
There was a problem hiding this comment.
@DisplayName 을 사용해서 테스트에 대한 설명을 하려한 점은 좋지만
현재 적혀있는 설명만으로는 정확히 어떤 테스트를 하고자 하는지 파악하기가 어렵습니다.
@DisplayName 을 보고 어떠한 동작을 하는지 조금더 자세하게 적어주는게 어떨까요?
테스트 코드는 문서로써의 역할을 수행할 수 있어야합니다!
There was a problem hiding this comment.
아, 이게 Delimiters 라는 객체의 Test라는 class명으로 이미 무엇을 테스트하는지 알려주고 있다고 생각을 했는데, 그래도 모른다 생각하고 더 자세히 알려주는게 좋은가요 ??
There was a problem hiding this comment.
이 어플리케이션을 처음 보는 사람은 Delimters가 어떤 역할을 하는지 모르는 상태라고 보는게 더 맞을것 같습니다.
그러한 상황에서 테스트 클래스의 이름만 보고 문맥을 파악하기란 쉽지 않을것 같아요!
|
|
||
| return numberString.getSplitStringNumbers(delimiters).stream() | ||
| .mapToInt(Integer::valueOf) | ||
| .boxed() |
There was a problem hiding this comment.
mapToInt 말고 map으로만 하면 boxed 할 필요없이 Integer 리스트로 collect 되지않나요??
5 participants
No description provided.