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

[4기 - 김범우] 1~2주차 과제 : 계산기 구현 미션 제출합니다. #154

Open
wants to merge 11 commits into
base: beomukim
Choose a base branch
from

Conversation

beomukim
Copy link

📌 과제 설명

목표

과제의 목표는 다음과 같습니다:

  • 주어진 문제를 해결하기 위한 애플리케이션 개발

  • 사용자로부터 입력을 받고, 필요한 계산을 수행하여 결과를 출력하는 기능 구현

  • 계산 기록을 유지하고, 이전 계산 내역을 조회할 수 있는 기능 구현

  • 코드의 가독성과 확장성 고려하여 적절한 객체지향적 설계

  • 객체지향적인 코드로 계산기 구현하기

    • 더하기
    • 빼기
    • 곱하기
    • 나누기
    • 우선순위(사칙연산)
  • 테스트 코드 구현하기

  • 계산 이력을 맵으로 데이터 저장기능 만들기

  • 정규식 사용

패키지 구조

  • calculator
    • ui
      • controller
        • CalculatorController.java
      • CalculatorUI.java
      • Menu.java
    • operation
      • CalculationOperation.java
      • Addition.java
      • Subtraction.java
      • Multiplication.java
      • Division.java
    • model
      • Calculator.java
  • main
    • Main.java

구현이 생각보다 복잡하게 되었는데, 조금 더 간결하고 깔끔한 방향으로 수정할 생각입니다!

@beomukim beomukim requested review from seung-hun-h and dojinyou June 10, 2023 03:32
@beomukim beomukim self-assigned this Jun 10, 2023
@beomukim beomukim changed the title Beomukim [4기 - 김범우] 1~2주차 과제 : 계산기 구현 미션 제출합니다. Jun 10, 2023
Comment on lines 14 to 15
System.out.println(Menu.조회.getValue() + ". " + Menu.조회);
System.out.println(Menu.계산.getValue() + ". " + Menu.계산);
Copy link
Member

Choose a reason for hiding this comment

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

출력되는 정보들이 Menu라는 클래스가 대부분 알고 있는 것 같아요! 여기서 개별적으로 가져와서 조합하는 것보다는 정보를 많이 알고 있는 클래스에게 맡기는 건 어떤가요?

Comment on lines +19 to +22
calculationOperations.put("*", new Multiplication());
calculationOperations.put("/", new Division());
calculationOperations.put("+", new Addition());
calculationOperations.put("-", new Subtraction());
Copy link
Member

Choose a reason for hiding this comment

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

key값으로 들어가는 StringCalculationOperation들의 getOperator에 정보가 있는 거 같은데 중복되는 것 같아요!
정보를 아는 클래스에게 가져오는 건 어떨까요?

Comment on lines +71 to +74
for (String targetOperator : targetOperators) {
for (int i = 0; i < operators.size(); i++) {
String operator = operators.get(i);
if (operator.equals(targetOperator)) {
Copy link
Member

Choose a reason for hiding this comment

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

다른 분들의 객체지향 생활 체조 관련 리뷰 참고 해주세요!

조회(1),
계산(2);

private final int value;
Copy link
Member

Choose a reason for hiding this comment

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

value는 너무 일반적인 이름인 것 같아요! 조금 더 구체적인 이름을 쓰시면 이해에 더 도움이 될 것 같아요~

Comment on lines +20 to +26
private static Stream<Arguments> testData() {
return Stream.of(
Arguments.of(1, 2, "+", 3),
Arguments.of(1, 2, "-", -1),
Arguments.of(2, 3, "*", 6),
Arguments.of(4, 2, "/", 2)
);
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link

@seung-hun-h seung-hun-h left a comment

Choose a reason for hiding this comment

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

안녕하세요 범우님 리뷰 남겼습니다. 확인해주세요~

Comment on lines 30 to 31
default:
System.out.println("잘못된 선택입니다. 다시 선택해주세요.");

Choose a reason for hiding this comment

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

👍

Comment on lines 19 to 29
while (true) {
output.printMenu();
int choice = input.getIntInput();

switch (choice) {
case 1:
output.printCalculationHistory(calculator.getCalculationHistory());
break;
case 2:
handleCalculation();
break;

Choose a reason for hiding this comment

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

true, 1, 2는 변수명으로 의미있는 이름을 부여해주세요

private void handleCalculation() {
input.getStringInput();

System.out.print("수식을 입력하세요: ");

Choose a reason for hiding this comment

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

출력의 책임은 Output이 갖는 것이 좋겠네요

calculationHistory.add(expression);
}

public double handleCalculation(String[] tokens, String expression) {

Choose a reason for hiding this comment

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

tokensexpression을 분리하여 매개변수로 사용하신 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

expression은 문자열 그대로 히스토리에 저장하기 위해 받고, tokens는 계산에 필요한 값들을 명확하게 표현하기 위해 따로 분리하였습니다! 따로 분리하는 것이 가독성이 좋아 보여 이렇게 사용했습니다

Choose a reason for hiding this comment

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

expression이 history를 저장하는 데만 사용하고 있어서, 가독성 측면에서 어떤게 좋은지 공감이 되질 않네요.
오히려 연산은 토큰으로하고 history 저장은 expression으로 하니 헷갈리는 것 같아요.

token은 내부에서 split 하여 사용하는 것이 좋아보입니다

List<Double> operands = new ArrayList<>();

for (String token : tokens) {
if (token.matches("[+\\-*/]")) {

Choose a reason for hiding this comment

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

Pattern객체를 상수로 만드는 것도 좋겠네요

Comment on lines 4 to 5
조회(1),
계산(2);

Choose a reason for hiding this comment

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

일반적으로 이름에 한글을 작성하지 않아요. Enum의 인스턴스의 네이밍 컨벤션에 맞게 수정해주세요
https://stackoverflow.com/questions/3069743/coding-conventions-naming-enums

package example.calculator.model.operation;

public interface CalculationOperation {
double calculate(double a, double b);

Choose a reason for hiding this comment

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

double 타입은 연산이 부정확 할 수 있어요

https://jerry92k.tistory.com/6


public class Calculator {
private Map<String, CalculationOperation> calculationOperations;
private List<String> calculationHistory;

Choose a reason for hiding this comment

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

Calculator가 히스토리까지 관리하는 책임을 가지고 있는 것 같네요. 분리 해주는 것이 좋아보여요

Copy link
Author

Choose a reason for hiding this comment

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

네. 단일 책임 원칙(Single Responsibility Principle)을 따르는 것이 좋을 거 같은데 생각을 못했네요 🥲

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants