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주차 과제: 계산기 구현 미션 제출합니다. #186

Open
wants to merge 77 commits into
base: hongxeob
Choose a base branch
from

Conversation

hongxeob
Copy link
Member

@hongxeob hongxeob commented Jun 14, 2023

📌 과제 설명

  • 기본적인 사칙연산(+,-,*,/)이 가능합니다.
  • 중위표기식을 쓰는 사람의 언어에서 컴퓨터가 인식하는 후위 표기식으로 변환하여 연산을 합니다.
  • 연산 이력을 별도로 저장하여 내역을 확인할 수 있습니다.
  • 사용자가 더 이상 프로그램을 사용하고 싶지 않다면 종료를 시킬 수 있습니다.
  • 계산에 필요한 연산식이 올바르지 않으면 에러메세지로 안내를 해줍니다.
    • 필수 연산자(+,-,*,/)가 들어가는 자리에 연산자가 아닌 것이 들어가면 메세지를 띄웁니다.
    • 피연산자가 들어가야 하는 자리에 숫자가 아닌 다른 문자가 들어가면 메세지를 띄웁니다.
    • 아무 연산식을 입력하지 않고 제출한다면 메세지를 띄웁니다.
    • 계산 결과가 없는 상태에서 저장 이력을 조회하면 메세지를 띄웁니다.
  • 후위표기식을 사용하는 계산기가 아닌 다른 경우가 추가될 것을 대비해, 확장성을 고려하여 설계하였습니다.

👩‍💻 요구 사항과 구현 내용

  • 객체지향적인 코드로 계산기 구현하기
    • 더하기
    • 빼기
    • 곱하기
    • 나누기
    • 우선순위(사칙연산)
  • 테스트 코드 구현하기
  • 계산 이력을 맵으로 데이터 저장기능 만들기
  • 정규식 사용

✅ 피드백 반영사항

  • 먼저 개념적으로 이해를 하고 생각하고 코드를 짜려고 노력했습니다.
  • 최대한 추상적인 것에 의존하려고 노력했습니다.
  • 사용자, 계산기, UI간의 상호작용을 생각하여 분리했습니다.
  • 입출력을 담당하는 Console을 추상체에 의존했습니다.
  • 예외시 프로그램을 종료시키는 것이 아닌 사용자에게 최대한 친절하게 에러 메세지를 알려주고, 잘못된 부분부터 시작할 수 있도록 했습니다.
  • 메서드를 최대한 역할별로 분류하고, 모아 응집도 있는 클래스를 만들고자 노력했습니다.
  • 확장성을 고려하여 최대한 구현하였습니다.

✅ PR 포인트 & 궁금한 점

  • 자기주도적 코드리뷰를 통해 많은 배움을 얻을 수 있었습니다. 다만 아직까지 패키지 분류를 어떻게 하면 좋을지 고민이 많이 됩니다.

  • 코드 중 유효성을 체크하는 Validator 클래스가 있습니다. 여러 종류의 계산기가 생겼을 때 유틸성으로 사용하기 위해 static을 사용하였습니다. 다만 non-static 메서드로 만들고 인스턴스성으로 사용하여 필요한 곳에만 주입 시켜주는게 나은지 고민입니다.

    • 저의 의도는 나중에 다양한 형식의 계산기(컨버터)가 생길 수도 있을 것을 고려해 다양한 곳에서 쓰려고 static으로 만들었습니다.
  • 커밋시 커밋 단위를 잘 쪼개었는지, 또 깃 커밋 메세지 컨벤션에 의거해서 커밋 메세지를 잘 쓰고 있는지 잘 모르겠습니다.

  • 처음으로 이렇게 객체지향에 관하여 생각해 보고 설계 및 구현을 해보았는데, 만약 같은 기능이지만 구현 방법에 차이가 있다면 그 중 어떠한 기준을 가지고 (개발자의 취향?, 팀의 규칙?, 사용자의 관점?) 방법을 선택하는 것이 가장 best practice에 가까워질 수 있을지 모르겠습니다.

Copy link

@devYSK devYSK left a comment

Choose a reason for hiding this comment

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

안녕하세요 홍섭님. 라이브 코드리뷰 피드백 반영하시느냐 고생 많으셨습니다.
함께 이야기 했던 내용중, 적용 안된 부분과 아쉬운 부분들이 있어 리뷰로 몇자 남겼습니다!

자기주도적 코드리뷰를 통해 많은 배움을 얻을 수 있었습니다. 다만 아직까지 패키지 분류를 어떻게 하면 좋을지 고민이 많이 됩니다.

-> 여러 방법이 존재합니다. 비슷한 역할을 하는애들끼리 모아두면 관리하기에 더 좋겠죠?
도메인 주도로 응집, 계층 구조 고려, 기능 또는 모둘별로, 유틸리티랑 설정 분리 등등
앞으로 개발하시면서 다른사람들 패키징, 또는 라이브러리나, 프레임워크들의 패키지 구조를 보신다면 잘 모아두신걸 보실 수 있습니다.
경험하시면서 늘으실거라 생각합니다.


코드 중 유효성을 체크하는 Validator 클래스가 있습니다. 여러 종류의 계산기가 생겼을 때 유틸성으로 사용하기 위해 static을 사용하였습니다. 다만 non-static 메서드로 만들고 인스턴스성으로 사용하여 필요한 곳에만 주입 시켜주는게 나은지 고민입니다.

  • 저의 의도는 나중에 다양한 형식의 계산기(컨버터)가 생길 수도 있을 것을 고려해 다양한 곳에서 쓰려고 static으로 만들었습니다.

-> 홍섭님의 의도를 제가 이해한바에 따르면, 다양한 형식의 컨버터가 생긴다면.. static일수록 코드를 직접 건드리고 수정해야 하는 경우가 많아질것 같은데요 ?


커밋시 커밋 단위를 잘 쪼개었는지, 또 깃 커밋 메세지 컨벤션에 의거해서 커밋 메세지를 잘 쓰고 있는지 잘 모르겠습니다.

-> 팀 바이 팀, 케이스 바이 케이스라고 생각합니다. 커밋단위가 다 다르더라구요.

지금 커밋 내역 보니 그래도 prefix를 깔끔하게 두셔서 읽기에 불편한점은 없었습니다!


처음으로 이렇게 객체지향에 관하여 생각해 보고 설계 및 구현을 해보았는데, 만약 같은 기능이지만 구현 방법에 차이가 있다면 그 중 어떠한 기준을 가지고 (개발자의 취향?, 팀의 규칙?, 사용자의 관점?) 방법을 선택하는 것이 가장 best practice에 가까워질 수 있을지 모르겠습니다.

-> 잘 하셨다고 생각합니다만, 처음이라고 하신다면.. 앞으로 더 성장이 기대가 됩니다.
저도 이부분은 흑구님과 동일한 생각인데요,
다만 프로젝트를 할 때마다 주제가 바뀌고 경험이 쌓여 더 나은방향을 고민하게 되더라구요.
혼자 개발하더라도 매번 다르고 팀의 규칙도 팀이 바뀌면 달라집니다 .

가능하면 셋 다 챙기는것이 좋습니다.

@hongxeob
Copy link
Member Author

흑구님 영수님 소중한 코드리뷰 정말 감사드립니다!
피드백 남겨주신 대로 최대한 개선해 보았으며
크게 수정된 부분은

  • 리턴 타입, 파라미터 타입을 value - > object 변경
  • Manager가 하는 책임을 다른 클래스를 만들어 역할 분배
  • 유틸성 메서드 인스턴스화 방지
  • 유효성 검사에 필요한 더블체크 등
  • 가독성을 위한 개행 수정

1차 pr 후 답변 및 궁금한 점

PR 답변

  • 가끔 사칙연산만 쓰는 화면으로 돌아가서 사칙연산만을 제공하는 서비스가 있다고 할때, 해당 계산기는 어떤 디자인 패턴을 통해 AbstractCalculator에 주입될 수 있을까요?
    • 전략 패턴을 사용하여 알고리즘군을 정의하고, 각각의 알고리즘을 캡슐화하여 상호 교환이 가능하도록 만들면 될 것 같단 생각이 드는데.. 현재 관련해서 팀원들과도 생각을 이야기 해보고있습니다 ! 흥미로운 주제인듯합니다
  • Util 클래스가 여기저기서 객체로 생성되는것에 대해서 어떻게 생각하세요?
    • 생성자를 private만들어 인스턴스화 시도시 인스턴스화 될 수 없다고 예외를 줘야겠군요!

코멘트 중 궁금한 점

  • postfixConverter가 주입되어야할 위치가 과연 여기가 가장 베스트인가요?
    • Converter는 계산기와 밀접한 관련이 있는 구성 요소라고 생각합니다.
      따라서 Calculator 클래스와 밀접한 관련이 있는 경우 Converter를 주입하는 것이 적합하다고 생각했습니다.
      예를 들어, Calculator 클래스의 생성자에서 Converter를 주입받고, Calculator 클래스를 사용하는 다른 클래스에서는 Calculator 객체를 생성할 때 Converter를 함께 주입하는 방식으로 사용할 수 있다고 생각했습니다.
      그런데 Converter가 다른 클래스에서도 사용된다면, Converter를 주입받는 위치를 변경해야 할 수도 있을것 같습니다.
      예를 들어, 여러 곳에서 Converter를 사용하는 경우 Converter를 공통으로 사용하는 클래스의 생성자에서 주입받고..?!
      해당 클래스를 주입받는 형태로 의존성을 전파할 수 있을까 하는 생각이 드는데요. 이렇게 하면 Converter의 인스턴스를 한 번만 생성하여 공유할 수 있을 것 같습니다
      생각보다 감이 잘 안잡히네요 ㅠㅠㅠ..

✅ PR 포인트 & 궁금한 점

  • SRP를 지켜보는 연습을 하기 위해 기존의 CalculatorManager가 하는 책임을 다른 매니저(?)들의 객체를 만들어 분류해 주었는데 너무 오버 엔지니어링인가 하는 고민이 듭니다! 연습의 목적으로는 '쪼개보길 잘했다' 라고 생각이 드는데 명확한 판단이 잘 안섭니다..!

Comment on lines 21 to 33
// @Override
// public boolean equals(Object o) {
// if (this == o) return true;
// if (o == null || getClass() != o.getClass()) return false;
// CalculationResult that = (CalculationResult) o;
// return Objects.equals(formula, that.formula) && result == that.result;
// }
//
// @Override
// public int hashCode() {
// return Objects.hash(formula, result);
// }
}

Choose a reason for hiding this comment

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

이런거 커밋하지 마세요.

Copy link
Member Author

Choose a reason for hiding this comment

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

앗 커밋시에는 조금 더 신중하게 하는 습관을 길러보도록 하겠습니다..!

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