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.

Sign up for GitHub

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

Jump to bottom

[로또 - TDD] 1단계 - 문자열 계산기 #2334

Merged
merged 16 commits into from
Apr 29, 2022
Merged

[로또 - TDD] 1단계 - 문자열 계산기 #2334

merged 16 commits into from
Apr 29, 2022

Conversation

Wu22e
Copy link

@Wu22e Wu22e commented Apr 25, 2022

안녕하세요. 이번 로또미션을 수행할 김형우 라고 합니다!
앞으로 잘 부탁드립니다~!

문자열 계산기 리뷰 요청 드립니다!

yiminan
yiminan requested changes Apr 26, 2022
View reviewed changes
Copy link

@yiminan yiminan left a comment

Choose a reason for hiding this comment

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

안녕하세요~! 형우님 만나 뵙게 되어서 반갑습니다.
"로또"를 함께할 안이민이라고 합니다. 😄
몇 가지 코멘트를 남겨드렸습니다. 고민하시고, 다시 요청을 부탁드릴게요.
궁금하신 점이나 같이 생각해볼 부분이 있으시면, 코멘트나 DM을 주셔서 같이 고민해보면 좋겠습니다.
좋은 밤되세요!

src/main/java/calculator/README.md Show resolved Hide resolved
src/main/java/calculator/Calculator.java Outdated Show resolved Hide resolved
src/main/java/calculator/Calculator.java Outdated Show resolved Hide resolved
src/main/java/calculator/Calculator.java Outdated Show resolved Hide resolved
src/main/java/calculator/Calculator.java Outdated Show resolved Hide resolved
src/main/java/calculator/SplitString.java Outdated Show resolved Hide resolved
src/main/java/calculator/SplitString.java Outdated Show resolved Hide resolved
src/test/java/calculator/SplitStringTest.java Outdated Show resolved Hide resolved
src/main/java/calculator/StringCalculator.java Outdated Show resolved Hide resolved
src/test/java/calculator/StringCalculatorTest.java Outdated Show resolved Hide resolved
khw0801 and others added 6 commits April 27, 2022 13:39
fix : 열거형 필드 상수를 위한 final 추가
13da65a
refactor : Calculator -> Operator 네이밍 변경
b9c9ee0 
- Operator 의 calculate 메서드명 operate 로 변경
- test 코드에서 불필요한 로컬변수 제거
@Wu22e
refactor : Operator 클래스 변수 캐싱할 때 FP 방식 이용
43c1fff
@Wu22e
docs : 클래스 구조 변경에 따른 기능 목록 변경
ff84f7b
@Wu22e
refactor : 피연산자 클래스 생성
3e1b241 
- SplitString 클래스의 과도한 책임 분리
- Operator에서 연산자 유효성 체크
- Operand에서 숫자 유효성 체크
- 문자열 계산기 가독성을 위한 인덱스 상수화
@Wu22e
refactor : 예외 메시지에 실패 정보 담기
2bb5321
@yiminan
Copy link

yiminan commented Apr 29, 2022

형우님 재요청을 안해주셔서 혹시 까먹고 놓치신건지 싶어서 코멘트 남깁니다! 😄
아직 반영이 덜 된건가요?

@Wu22e
Copy link
Author

Wu22e commented Apr 29, 2022

이번주가 좀 바쁜일이 많아서 요청을 빠르게 드리지 못했네요 ㅠㅠㅠ

피드백 주신 내용들 최대한 반영하려고 노력했습니다..!
Expression 객체를 만들어서 StringCalculator 부분을 깔끔하게 해보고 싶었는데
현재 입력받는 문자열구조에서 더나은 방법이 떠오르지 않아 우선은 매직 넘버를 상수화 해서 가독성을 좀 더 높였습니다.

이번 리뷰도 잘 부탁드립니다!

yiminan
yiminan approved these changes Apr 29, 2022
View reviewed changes
Copy link

@yiminan yiminan left a comment

Choose a reason for hiding this comment

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

안녕하세요. 형우님! 😄
코멘트 드렸던 부분을 잘 이해하시고 개선도 잘해보셨네요.
개선 과정에서 생각되었던 부분에 대해서 코멘트 추가도 남겨드렸구요.
다음 미션에서 반영해주시면 됩니다.
먼저 코멘트에서 제가 답변하지 않았던 부분도 추가로 답변 드렸습니다.
참고 부탁드릴게요.
다음 미션을 위해서 PR은 머지하도록 하겠습니다.
즐거운 주말되세요~! 다음 미션에서 뵙겠습니다. 👍

src/test/java/calculator/StringCalculatorTest.java Show resolved Hide resolved
src/main/java/calculator/Operand.java Show resolved Hide resolved
src/main/java/calculator/Operand.java Show resolved Hide resolved
src/main/java/calculator/SplitString.java Show resolved Hide resolved
src/main/java/calculator/StringCalculator.java Show resolved Hide resolved
src/main/java/calculator/StringCalculator.java Show resolved Hide resolved
src/main/java/calculator/StringCalculator.java Show resolved Hide resolved
src/test/java/calculator/SplitStringTest.java Show resolved Hide resolved
@yiminan yiminan merged commit 49f8d95 into next-step:wu22e Apr 29, 2022
@yiminan yiminan mentioned this pull request May 3, 2022
Merged
@Wu22e Wu22e changed the title Step1. 문자열 계산기 PR. [로또 - TDD] 1단계 - 문자열 계산기 May 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yiminan yiminan yiminan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Wu22e @yiminan