-
Notifications
You must be signed in to change notification settings - Fork 156
[4기 - 김영주] 1~2주차 과제: 계산기 구현 미션 제출합니다. (5차) #134
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
[4기 - 김영주] 1~2주차 과제: 계산기 구현 미션 제출합니다. (5차) #134
Conversation
- 숫자 이외의 문자를 입력하는 경우 예외 처리 - 현재 서비스 되고 있는 메뉴 이외의 숫자를 입력하는 경우 예외 처리
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 영주님 첫 PR 올리시느라 고생하셨습니다.
먼저 코드 작성 하시기 전, 설계부터 고민하신 부분이 너무 좋았습니다 ! 👍🏻
고민사항에 대한 저의 개인적인 의견을 남겼습니다 정답은 아니니 영주님도 편하게 말씀해주시면 좋을 것 같아요 :)
고민사항
- 구조에 대한 고민
- 저는 개인적으로 작은 프로그램이어도 MVC 패턴을 적용하는 것은 좋다고 생각합니다. 규모가 작더라도 코드를 더 명확하고 구조화 시킬 수 있기 때문에 유지보수성, 확장성, 가독성 등등 측면에서 이점이 있다고 생각합니다.
- MVC 패턴을 적용하는 부분이 아직 감이 잘 안잡히시는 거라면 영주님만의 구조를 만들어 보시고 같이 개선시켜 나가는 부분 또한 저는 좋다고 생각합니다 ~
- 예외처리에 대한 고민
- 예외처리 로직이 복잡하다면 분리하는 것도 좋은 방법이겠지만 별도의 클래스를 만드는 것에 대한 부분이 비효율적으로 느끼신다면 if문을 사용하셔도 됩니다. 예외를 또한 잡을 것인지 프로세스를 중단할지 여부도 고려해보시면 좋을 것 같습니다.
- 매우 큰 수에 대한 고민
- 꼼꼼하게 생각하시는 부분이 좋은 것 같습니다 ! 범위에 대한 고민도 좋지만 “일반적인” 경우를 생각 하셨을 때 BigDecimal을 사용하는 경우도 한번 생각해보시면 어떨까요? 금융 계산이나 정확한 연산을 수행하는 경우라면 BigDecimal이 더 적합할 수 있지만 그만큼 원시타입 보다는 더 많은 메모리를 사용하고 연산속도에도 차이가 있을 수 있지 않을까요? 영주님의 의견을 더 들어보고 싶습니다. 🧐
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 영주 님 반가워요. 🙇
첫 PR 축하드려요. 🥳
프로그램 구조도를 PR 에 공유 해주셔서 너무 좋았어요. 👍
커밋 메시지도 잘 나누시고 PR 리뷰 시점도 작게 잘 하셨군요. 👍 👍
🧾 리뷰
형욱님이 코드리뷰를 잘 해주셨기에 저의 코드 리뷰는 몇개 없을것 같아요.
이번주차 강의에 내용을 바탕으로 해당 미션에 조금더 반영하시면 좋을것 같아요.
리뷰내용 반영하신 후에 다음 기능 구현을 해보시는 방향으로 추천 드리고 싶어요.
👼 답변
고민 사항에 대해 간단히 답변 드리자면, 현재 패키지 구조(패턴)를 너무 신경쓰지지 않으셔도 된다고 생각해요.
역할이 잘 나누어져 있는지, 혹은 지금 분리를 해야 하는 시점일에 대해 더 집중하면 좋을것 같은 생각을 하고 있어요.
매우 큰수에 대한 고민도 하신것으로 알고 있어요.
좋은 고민 포인트라고 생각해요.
하지만 이것 또한 개선, 고도화의 작업이기에 미션을 완수하고 추후에 고려 해보아도 좋은것 같아요.
고생하셨어요. 🙇
app/src/main/java/calculator/validation/MenuInputValidator.java
Outdated
Show resolved
Hide resolved
app/src/main/java/calculator/validation/MenuInputValidator.java
Outdated
Show resolved
Hide resolved
- 객체 책임 분리를 위해 입출력 기능은 View로 모두 이관 - 별도의 Validator를 삭제하고, 각 객체에서 예외처리
- null 값 초기화로 인한 NPE를 방지 - Stream 사용을 통한 가독성 향상
- 단순 예외 문구 프린트 용이므로 최상단 App에서 예외처리 - 정상과 예외 상황 모두 종료 메시지 출력 - MainController의 인덴트 깊이가 2에서 1로 감소
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다. 🙇
리뷰 코멘트에 대해 커밋 단위로 피드백 반영 알려주셔서 좋은것 같아요. 👍
간단한 리뷰 내용이라 피드백 반영해주시고 바로 다음 피처(기능)를 진행해도 될것 같아요.
(다음 리뷰는 작은 단위의 기능을 추가해주시면 될것 같아요.)
PS: 리뷰 준비 완료 되셨다면 PR 에서 리뷰 부탁드린다는 코멘트를 남기면 좋을것 같아요.
- Menu.isQuit()의 매개변수를 기존 Optional에서 Menu 타입으로 변경 - Menu.getSelectedMenu()의 예외처리를 Optional의 orElseThrow()를 통해 진행
@Jo-GyuHyeon 말씀해주셨던 피드백에 대해 9de3ddf 에 반영하여 재 업로드 하였습니다. 감사합니다! |
- 자연수와 사칙연산 기호 이외의 입력에 대한 예외처리 - 공백을 기준으로 구분되지 않는 입력에 대한 예외처리
- 나눗셈 연산 시, 제수에 0이 들어오는 경우 예외처리 - 연산자 우선순위는 고려하지 않음
- 여러 클래스에서 공통으로 사용할 수 있는 기능은 utils로 분리
- validation을 각 클래스에서 진행하므로 validation 패키지를 제거
- 매개변수를 받지 않고 this 키워드를 이용해 인스턴스 자신과 메뉴를 비교
…)로 변경 - NPE 방지를 위해 동일 연산자가 없으면 예외 처리하도록 변경
- savePartialEvaluationResult()의 책임을 줄이기 위함
- 가독성 측면에서 변수로 할당하여 run() 메서드 실행
[3차 PR 리뷰에 대한 수정사항 반영] @Jo-GyuHyeon @HyoungUkJJang
|
- 계산 내역이 없는 경우 내역 대신 별도의 문구 출력
[4차 PR 업로드] @Jo-GyuHyeon @HyoungUkJJang
service 패키지에 있는
항상 바쁜 시간 쪼개서 리뷰 해주셔서 감사합니다. 정말 많은 도움이 되고 있습니다! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 영주님 😄 고생하셨습니다.
#134 (comment) 의 질문에 대한 �저의 의견은 영주님과 동일한것 같아요.
지난번 리뷰에 관련하여 다시 피드백 드려보았어요.
이번에는 해당 리뷰 피드백 까지만 반영하시고, 전체적으로 코드를 확인 후 리뷰 요청을 진행 하면 어떨까 해요. 😸
고생많으셨어요. 🙇
- 기존 HistoryReader는 비즈니스 로직 없이 단순 View 기능을 담당 - HistoryReader의 로직을 OutputView로 이관하고, MainController에서 단순 호출
- 연산자 1개, 피연산자 2개로 이루어진 Expression 클래스를 정의 - Calculator는 Expression을 만드는 역할만 하고, 계산은 Expression에서 실행
- 계산기의 행위인 계산하다에 초점을 맞추어 메서드명 변경 - Calculator 내의 calculate 메서드는 evaluate로 메서드명 변경
[4차 PR 리뷰에 대한 수정사항 반영]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
영주님 간단하게 피드백 드렸어요. 👍
- Calculator는 식에 대한 결과를 계산하는 책임만 할 수 있도록 View 부분은 상위로 분리
- InfixNotation, PostfixNotation 구현체를 통해 여러 표기식을 지원 예정
- PostfixNotation 클래스의 makeExpression 메서드 구현
[5차 PR 업로드] @Jo-GyuHyeon @HyoungUkJJang
우려되는(?) 혹은 고민되는 부분은 아래와 같습니다.
지난 2주 간 꼼꼼하게 리뷰 해주셔서 정말 감사드립니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
간단하지만, 추상적으로 리뷰를 남겨 두었습니다.
거의 막바지에 오셨군요?
고생 많으셨습니다. 🙇
그림 표현도 잘 하셨군요!!! 👍 👍 👍
String[] elements = notation.makeElements(expression); | ||
int result = evaluate(elements); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makeElements는 notation 객체가 담당하는 것이 좋을지? 의문이 들어요. (꼭 필요할까?)
notation 객체로 분리 한 만큼 makeExpression 를 목록 형태로 제공 해준다면, 계산기는 순차적으로 연산만 수행 하는 형태는 어떨까요?
Stack<Integer> operands = new Stack<>(); | ||
Stack<Operator> operators = new Stack<>(); | ||
|
||
for (String element : elements) { | ||
notation.makeExpression(operands, operators, element) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
notation 의 makeExpression()와 계산기의 로직의 책임이 섞여 있는 것 같아요.
응집력이 약한것 같아요.
위의 피드백과 연관이 있을것 같아요.
for (String element : elements) { | ||
saveElement(postfixElements, operators, element); | ||
} | ||
|
||
while (!operators.isEmpty()) { | ||
addTopOfOperatorStackToPostfixElements(postfixElements, operators); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
최대한 메서드 라인수를 줄일려고 많이 빼셨던것 같아요.
한편으로는 해당 메서드로만 보았을때 어떤 행위들을 하는지 유추하기 어려운것 같아요.
오히려 로직 흐름을 볼 수 있게끔 적당히 함께 풀어서 로직을 작성하는 것도 좋을것 같아요.
각 내부(private) 메서드도 객체의 내부 행위라고 생각 후 고민 하시면 더 좋을것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
영주님 코드가 정말 많이 좋아졌네요 :) 멘토님이 리뷰를 정말 잘해주셔서 제가 남길 부분은 이제 많이 없는 것 같아요 ~
사소한 의견 몇개 남겼으니 한번 참고해보시면 좋을 것 같습니다. 영주님 정말 고생하셨어요 👍🏻
int menuNumber = inputMenuNumber(); | ||
Menu selectedMenu = getSelectedMenu(menuNumber); | ||
|
||
isRunning = !selectedMenu.isQuit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
! 부정 연산자는 한번 더 생각하게 만드는 느낌이 있습니다. 차라리 isRunning() 메서드로 만드시는 것은 어떻게 생각하시나요?
String history = makeHistory(expression, result); | ||
historyStorage.save(history); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MainController에서도 HistoryStorage를 의존하고있고 Calculator에서도 HistoryStorage를 의존하고 있는 것 같습니다.
사실 계산기에서 계산만 하는 것 처럼 보이지만 계산, 저장 두가지 책임을 함께 하고 있는 것 같아요.
결과를 리턴하니 저장하는 부분은 MainController에서 History 책임으로 몰아주고 Calculator에서는 의존성을 제거해주는 방향은 어떻게 생각하시나요?
@@ -0,0 +1,25 @@ | |||
package calculator.utils; | |||
|
|||
public class StringUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
유틸리티 클래스는 전역에서 공유해서 사용하기 때문에 private 생성자를 통해서 생성을 막아주면 더욱 좋을 것 같습니다 ~
기능 구현 사항
테스트 코드
이외에는 테스트 코드 작성 아직 못했습니다.
프로그램 구조도
패키지 구조
실행 화면
감사합니다. 🙏