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

[문자열 덧셈 계산기] 민경태 미션 제출합니다. #1914

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

GO-TE
Copy link

@GO-TE GO-TE commented Oct 22, 2024

No description provided.

-  문자를 숫자로 변환시키는 클래스는 Adder라서 Validator에 어울리지 않는다고 생각함
변경된 점
- answer 타입 변경 int -> long
- answer 타입 변경에 따른 getAnswer 타입 변경 int -> long
기존에 두자리 이상 숫자도 한자리씩 잘라 리턴하는 현상 발견
* extractSeparactor()
- start index 값 말고, 좀 더 static 하게 변경 (변경 될 가능성 큼)
- end 입력 되는 \n이 개행문자가 아님을 알리기 위해 이스케이프 문자 추가 (\n -> \n)

* removeCustom()
- 개행문자가 아님을 알림 (변경 가능성 큼)
- 이미 분리한 문자열을 넘기고 있어 삭제
* hasCustomSeparator()
- 개행 문자로 인식하지 않도록 수정 (\n -> \n)

* validateInput()
- 주석 제거
- 개행문자로 인식하지 않도록 수정 (\n -> \n)
* isEmpty() 구현
- 기존 공백인지 미리 검증 하지 않아 index 초과 에러 발생
- 공백임을 검증하는 함수 따로 구현 -> 커스텀 구분자 확인, 숫자로 시작하는지 확인 하는 함수에 추가
- validateInput에 공백인지를 검사하는 코드 대체
이전에 exists 메소드가 사용되지 않았음
add 메소드에 구분자가 이미 존재하는지 검증 과정을 거치도록 바꿈
+ test 코드도 이미 존재하는지 없는지 로직에 따라 바꿈 (boolean에서 예외처리하도록)
예외 메세지는 따로 관리하는게 더 여러울 것 같아서 제외했음
- 커스텀 구분자 접두사, 접미사 Constants에 따로 선언
- 기본 구분자 리스트를 불변한 리스트로 따로 선언
- 올바른 구분자 길이 선언
- 추후 확장을 위한 소수점 선언
@RedSunSmile
Copy link

전체적으로 물흐르듯이 controller에서 호출해서,separatorManager, StringHandler,Validator 전체적인 흐름다좋아요 이런식으로 발전시키는구나 한수 배웠습니다 그런데 StringHandler에서 이미 separatorManager기능 이미 다해서 굳이 separatorManager필요없어도 될거같습니다 세부적으로 나눈점은 보기좋습니다. 그리고the AngularJS commit conventions이건 어떻게 쓰는지 솔직히 몰랐습니다 이번에서야 알게되었습니다 알려줘서 고맙구요 test는 잘쓰시네요 머이건 제가 배우는과정이다보니 많이 보고 참조하겠습니다~~!!!

Copy link

@phk1128 phk1128 left a comment

Choose a reason for hiding this comment

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

리뷰가 조금 늦었습니다 ~ ㅎㅎ

SRP를 지킬려고 고민하신 흔적들이 많이 보이네요~

코딩에는 정답이 없듯이 제가 말씀 드린 부분은 고민 포인트로만 생각해주시면 좋겠습니다~

그럼 2주차도 화이팅 입니다!

Comment on lines +10 to +15
Input input = new Input();
Output output = new Output();
SeparatorManager separatorManager = new SeparatorManager();
StringHandler stringHandler = new StringHandler();
Validator validator = new Validator();
Adder adder = new Adder();
Copy link

Choose a reason for hiding this comment

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

클래스 내부에서 객체를 생성하면 어떤 단점이 있을까요 ??

관련해서 의존성 주입을 참고해보시면 좋을것 같아요 ㅎㅎ

Copy link

Choose a reason for hiding this comment

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

InputView가 입력 에 대한 책임만 갖고 있네요~

저도 이러한 방식으로 구현했는데요~ 근데 피드백으로 사용자에게 입력을 받을때 출력하는 문구는 InputView의 책임이 아닌가 하는 의견을 들었습니다~

이 부분에 대해서 저는 아직도 해답을 찾지 못했는데, 경태님의 생각은 어떠신지 궁금해요!

Copy link

Choose a reason for hiding this comment

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

요고는 필드에 상태를 안져도 괜찮을것 같은데, 경태님 생각은 어떠신가요 ??

Copy link

Choose a reason for hiding this comment

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

깔끔 합니다 👍

Copy link

Choose a reason for hiding this comment

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

해당 클래스에 있는 메서드들을 static으로 선언하면 재사용하기 더 좋을것 같아요~

Copy link

Choose a reason for hiding this comment

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

테스트가 잘게 쪼개져 있고, 메서드 이름도 명확해서 가독성이 좋네요 ! 👍

Copy link

@ashsty ashsty left a comment

Choose a reason for hiding this comment

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

안녕하세요 리뷰이님! 코레아 서비스 리뷰어 애쉬입니다. :)

1레벨 미션 구현하시느라 고생하셨습니다. 2레벨 미션도 열심히 하고 계신가요? 👀
간단한 코멘트 남겨두었으니 관련해서 질문이 있으시다면 언제든 답코멘트 남겨주세요.

사이트에 피드백도 남겨둘 테니 확인해보시면 좋겠네요! 👍

@@ -3,7 +3,7 @@
입력한 문자열에서 숫자를 추출하여 더하는 계산기를 구현한다.

### 입력
- [ ] 유저의 입력을 전달한다.
- [X] 유저의 입력을 전달한다.
Copy link

Choose a reason for hiding this comment

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

문서도 꼼꼼하게 적어주셨고,
기능마다 체크 표시도 해주셨네요 👍

살아있는 문서화 좋습니다!

Copy link

Choose a reason for hiding this comment

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

EOF이 종종 보이네요!

EOF이 뭔지 찾아보시고,
이 글 참고해서 설정해보셔도 좋을 것 같아요.

@@ -18,4 +19,8 @@ public boolean exists(String separator) {
public void add(String separator) {
separators.add(separator);
}

public List<String> getSeparators() {
return Collections.unmodifiableList(separators);
Copy link

Choose a reason for hiding this comment

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

unmodifiableList()를 사용하셨네요 👍

Exception in thread "main" java.lang.NumberFormatException: For input string: "112kio12u328948"
```
- [ ] 기능 쪼개기
Copy link

Choose a reason for hiding this comment

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

제출을 완료했는데도 체크가 안된 체크 박스가 있는 건 왜일까요? 👀

Comment on lines +10 to +15
Input input = new Input();
Output output = new Output();
SeparatorManager separatorManager = new SeparatorManager();
StringHandler stringHandler = new StringHandler();
Validator validator = new Validator();
Adder adder = new Adder();
Copy link

Choose a reason for hiding this comment

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

컨트롤러에서 많은 객체를 생성해주고 있네요.

클래스 내부에서 객체를 직접 생성하면 생기는 단점은 차치하고서라도, 다른 단점은 있을 수 없을까요?
현재 컨트롤러 클래스에 지나치게 많은 역할이 부여되고 있지는 않은가 고민해보아도 좋을 것 같습니다. 🤔

import java.util.List;

public class Adder {
long answer;
Copy link

Choose a reason for hiding this comment

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

Adder라는 이름만 보면 단순히 덧셈 기능만을 수행할 것 같은데,
내부에 answer라는 필드를 관리하고 있네요. 또 이 값을 getter로 가져오고 있고요.

이때 발생할 수 있는 문제는 없을까요? 🤔

}

public String removeCustom(String input) {
int numberIndex = input.indexOf(Constants.CUSTOM_SUFFIX) + 2;
Copy link

Choose a reason for hiding this comment

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

여기의 "2"는 상수로 관리하지 않고 있는 이유가 있나요?

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.

4 participants