Skip to content

Conversation

@Gobukgol
Copy link

@Gobukgol Gobukgol commented Mar 3, 2020

No description provided.

Gobukgol added 2 commits March 3, 2020 16:08
1. 커스텀 구분자를 찾고 숫자 List로 분리하는 Splitter 클래스 구현
2. 분리된 숫자를 래핑하는 Number 클래스 구현
2. 기본 구분자 상수 [,:] 로 변경
public int calculate(String expression) {
List<Number> numbers = Splitter.getSplitNumbers(expression);

return numbers.stream()
Copy link
Contributor

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.

reduce로 변경하였습니다. reduce로 해서 새로운 Number객체로 반환해주는걸 Stream내부에서 할 수 있게되었슴다


private void validateNumber(String value) {
if (value == null || value.isEmpty()) {
throw new RuntimeException("빈 문자열 입니다.");
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.

이거 RunTimeException 을 구현한 애로 쓰는게 더 좋지않낭?
RuntimeException 을 catch 하면 모든 그 하위 Exception 이랑 같이 다 잡혀서 디버그하기 힘들것도같은뎅 !
흠 에러메시지 있어서 괜찮으려낭 ..

Copy link
Author

Choose a reason for hiding this comment

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

조금 헷갈리는게 요구사항엔 단순하게 RuntimeException을 던지라고 나와있어서 이렇게 했는데 조금 더 고민해보고 수정해볼께!

Copy link

Choose a reason for hiding this comment

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

오호 요구사항이 그랬구나 난 바보야


class SplitterTest {

@Ignore
Copy link
Member

Choose a reason for hiding this comment

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

@Ignore 걸려있는거죠?

Copy link
Author

Choose a reason for hiding this comment

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

바보같이 지우는걸 깜박했습니다... ㅎㅎ
그런데 test 패키지 우클릭하고 Run 'All Test' with Coverage 하면 테스트 커버리지 보여주는 것 같은데 요건 자코코랑 관련있는건가용

Copy link
Member

Choose a reason for hiding this comment

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

그건 인텔리제이가 테스트 커버리지 측정해줍니다 자코코랑은 다릅니다!

2. Splitter에 구분자 그룹과 실제 수식 그룹 상수 추가
3. SplitterTest에 Ignore 제거
private static final int DELIMITER_GROUP = 1;
private static final int FORMULA_GROUP = 2;

public static List<Number> getSplitNumbers(String expression) {
Copy link

Choose a reason for hiding this comment

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

아하 여기서 Number 로 꺼내주는방법이있었구나 ..

.reduce(Number::sum)
.orElseThrow(() -> new ArithmeticException());

return result.getValue();
Copy link
Member

Choose a reason for hiding this comment

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

stream 구문 이후 그 결과값을 받은뒤 return 하는거보다
return 에서 stream 구문을 끝내도록 하는게 좀 더 가독성이 좋을거같습니다. 아직 익숙하지 않아서 적절한 예시를 알려주진 못하지만 ..ㅠㅠ
찬인이형이 잘 했놧다고 생각합니당

Choose a reason for hiding this comment

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

그냥 Number객체를 리턴하는게 좋아보이네요

.reduce(Number::sum)
.orElseThrow(() -> new ArithmeticException());

return result.getValue();

Choose a reason for hiding this comment

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

그냥 Number객체를 리턴하는게 좋아보이네요

return value;
}

public static Number sum(Number a, Number b) {

Choose a reason for hiding this comment

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

클래스 메서드보단 number.sum(number)가 더 좋은 객체관계일거같아요

private static final int FORMULA_GROUP = 2;

public static List<Number> getSplitNumbers(String expression) {
if (expression == null || expression.isEmpty()) {

Choose a reason for hiding this comment

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

매번 추가 DELIMITER가 추가될때마다 if문이 하나씩 추가될꺼같은데 이런 구조보단 Splitter를 인터페이스로 빼고 여러 Splitter를 구현해서 다른 방식으로 해결하는게 좋을듯

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.

6 participants