[4기 - 박은지] 1~2주차 과제: 계산기 구현 미션 제출합니다.#136
Open
1o18z wants to merge 39 commits intoprgrms-be-devcourse:Eunjifrom
Open
[4기 - 박은지] 1~2주차 과제: 계산기 구현 미션 제출합니다.#1361o18z wants to merge 39 commits intoprgrms-be-devcourse:Eunjifrom
1o18z wants to merge 39 commits intoprgrms-be-devcourse:Eunjifrom
Conversation
Hunkik
reviewed
Jun 9, 2023
There was a problem hiding this comment.
계산기 과제 고생하셨습니다! 시간이 여유로워서 천천히 확인했습니다. 최대한 아는만큼만 의견 달아보겠습니다! 틀릴수도 있습니다 ..ㅎ
아직 네이밍이나 기본적인 부분만 리뷰했습니다. 고치시면 추가적으로 더 확인하도록 할게요.
그리구
1 + 3 * 4 / 2 - 1
결과 : -0.16666666666666674
이 결과가 이상하게 나오네요 .. 계산 로직 수정 부탁드립니다!
- OOP를 잘 지키고 있는지, 한 곳에 너무 많은 역할들이 몰려있는지 궁금합니다!
- 처음 하신것 치곤 좋다고 생각합니다! 나중에 더 메서드를 잘게 나눈다거나, validation을 메서드로 뺀다거나 하는 방향으로 생각하시면 좋아보입니다
- 테스트 코드를 따로 작성해본 경험이 없어서 어떻게 작성해야 하는지 감이 잘 잡히지 않습니다🥲 테스트 코드를 작성에 관한 자료를 찾아보는 중인데, 혹시 이 부분에 대해 미리 피드백을 주실 부분이 있으시다면 남겨주시면 참고하여 작성하겠습니다!
- 저는 처음엔 김영한님 강의를 봤구, 이후엔 여러 사람의 테스트코드나 책을 통해 확인했습니다. 힘드시면 나중에 말씀해주세요!
- 실패 테스트까지 작성해주셨으면 좋겠습니다
- 코드를 작성하면서 커밋을 어느 시기에 해야할 지 잘 모르겠습니다. 예를 들어 한 클래스에서는 메소드명을 하나 수정하고, 다른 클래스에서는 코드를 개선했다면, 이런 상황에서는 코드 개선에 관한 내용으로 묶어서 하나의 커밋을 작성하는 것이 좋을지, 아니면 메소드명과 코드 개선한 것을 따로 커밋을 작성하는게 좋을지 궁금합니다!
- 커밋 같은경우는 나중에 협업할 땐 기능별로 하지만 개인 커밋변경사항 같은 경우는 자유롭게 하시면 됩니다. 나중에 git branch 전략 한 번 확인해주세요
- 저는 일반적으론 특정 기능 기준으로 완료시에 커밋하는 편입니다.
- 아직 예외 처리에 대한 코드를 작성하지 못했는데, 예외를 한 곳에서 모아 처리하는 것이 좋을지, 예외가 발생한 곳에서 각각 처리하는 것이 좋을지 궁금합니다!
- �어떤게 좋은지는 애매하지만, 모아서 처리한다면 한 곳에서 에러를 관리하여 편해지지만 구체적으로 어디서 발생했는지 확인하기 힘들수도 있다고 생각합니다. 트레이드오프라고 생각하기 때문에 자유롭게 해주시면 좋을것같아여.
src/test/java/CalculatorTest.java
Outdated
| @@ -0,0 +1,32 @@ | |||
| import org.assertj.core.api.Assertions; | |||
| import org.example.Calculator; | |||
src/test/java/CalculatorTest.java
Outdated
| import org.junit.jupiter.api.Test; | ||
|
|
||
|
|
||
| public class CalculatorTest { |
There was a problem hiding this comment.
패키지는 일반적으로 다른 패키지의 접근이 필요없어 class만 쓰는경우가 많습니다!
| Output.showMenu(); | ||
|
|
||
| switch (Input.inputMenu()) { | ||
| case 1: |
There was a problem hiding this comment.
매직넘버를 최대한 사용하지 않는 방향으로 부탁드립니다! 매직 넘버는 따로 검색하면 나오니 한 번 찾아보시면 좋을것같아여
Comment on lines
30
to
31
| case 3: | ||
| return; |
There was a problem hiding this comment.
default로 1,2,3 이외의 입력을 했을때의 default도 있었으면 좋겠습니다! 오류처리나 간단한 출력이라도 괜찮을것같아여
| private static final BufferedReader br = new BufferedReader(new InputStreamReader(System.in)); | ||
|
|
||
| public static Integer inputMenu() throws IOException { | ||
| return Integer.parseInt(br.readLine()); |
There was a problem hiding this comment.
숫자 이외를 입력 시 예외처리가 필요해보입니다! 입력 후 Integer.parseInt 하기 전이 좋아보이네요
| return stack.pop(); | ||
| } | ||
|
|
||
| public List<String> seperate(String[] formula) { |
Comment on lines
29
to
30
| List<String> list = new ArrayList<>(); | ||
| Stack<String> stack = new Stack<>(); |
| MULTI("*", (num1, num2) -> num1 * num2), | ||
| DIV("/", (num1, num2) -> num1 / num2); | ||
|
|
||
| private final String operator; |
|
|
||
| public class History { | ||
|
|
||
| private Map<Long, Formula> map = new LinkedHashMap<>(); |
There was a problem hiding this comment.
- LinkedHashMap을 사용 한 이유가 있을까요?
- 해당 자료구조 같은 경우는 런타임시에 한 번 생성되고 변경되지 않아보여 static final 붙이는걸 추천드립니다.
- 의미있는 이름으로 바꿔주세요!
Author
There was a problem hiding this comment.
키를 순서(번호), 값을 수식을 받아 순서를 보장해 저장하기 위해 LinkedHashMap을 사용했습니다 !!
src/test/java/CalculatorTest.java
Outdated
| @Test | ||
| @DisplayName("곱하기") | ||
| void multiply() { | ||
| Assertions.assertThat(Calculator.multiply(6, 2)).isEqualTo(12); |
Author
✅ 피드백 반영사항
예외 처리와 테스트 코드 작성 부분은 진행 중입니다! 👩🏻💻 |
Hunkik
reviewed
Jun 13, 2023
Comment on lines
15
to
17
| final int HISTORY = 1; | ||
| final int CALCULATE = 2; | ||
| final int EXIT = 3; |
There was a problem hiding this comment.
접근제한자를 안 붙인 이유가 있을까요?
또 만약 런타임중에 변경되지 않는다면 static도 찾아보시면 좋을것같아요
|
�과제하느라 고생하셨어요. 코드를 읽기 쉬웠고, 객체에 역할 부여를 너무 과하지 너무 한곳에 몰려있지도 않은거 같아요.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 과제 설명
✅ PR 포인트 & 궁금한 점