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

Open
wants to merge 10 commits into
base: Curry4182
Choose a base branch
from

Conversation

Curry4182
Copy link

@Curry4182 Curry4182 commented Jun 15, 2023

📌 과제 설명

  • HashMap을 이용하여 계산 이력을 저장 하였습니다.
  • 계산 이력 기능이 정상 작동하는지 테스트 케이스를 작성하였습니다.
  • 역할과 책임을 생각하며 코드를 작성하였습니다.
  • CalcManager 객체 생성을 팩토리 메서드 패턴을 활용해서 생성하도록 만들었습니다.
  • 식을 입력하면 계산 사칙연산 계산 결과가 나오도록 하였습니다.
  • 내부적으로 후위연산으로 변경하고 계산결과가 출력되도록 하였습니다.

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

  • 사용자에게 콘솔로 입력받은 계산 식을 맵으로 저장
    -> 애플리케이션이 동작하는 동안 데이터베이스 외에 데이터를 저장할 수 있도록 개발 하였습니다.
  • 입력 받은 데이터를 저장하고 불러오는 테스트 코드 작성

✅ 피드백 반영사항

  • 필요하지 않는 의존성 주입 수정 (CalcConsoleView 클래스)
  • 메서드 레퍼런스를 사용하여 가독성 있게 만들었습니다.
  • main 함수에 너무 많은 인스턴스가 생성되어 facory 클래스에서 생성 하도록 수정하였습니다.
  • Test 케이스에 given, when, then의 의미를 생각하고 다시 수정하였습니다.
  • 식을 토큰 단위로 나누고 계산 하도록 구현
  • 계산 결과 검증 테스트 구현

✅ PR 포인트 & 궁금한 점

  • main함수에서 팩토리 메서드를 활용하여 CalcManager를 생성하도록 코드를 수정하였습니다. 처음에는 repository, calculator, view를 하나의 팩토리에서 생성하도록 만들다가 서로 의존적이지 않는 클래스라고 생각해서 분리 하여 만들었습니다.
  • 코드를 작성하면 팩토리 메서드 처럼 객체 생성을 분리하는것에 대해 확신이 점점 들지 않았습니다. 기존에 main에서 만드는 것보다 복잡해졌지만 효율적인가에 대해 고민했습니다.
  • 코드의 확장성을 위해 지금 처럼 구현하는게 맞을지 아니면 기존 코드처럼 main에서 CalaManager와 관련된 repository, view, Calculator를 생성 하지만 내부적으로 생성할 수 있는 의존성들은 내부에서 생성하도록 구현하는게 좋을지 궁금합니다!!

Copy link

@ICCHOI ICCHOI left a comment

Choose a reason for hiding this comment

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

병곤님 과제 하느라 수고하셨습니다!
1주차보다 많이 발전하신 것 같아요.
New팀에서도 이렇게 적극적으로 임하시면 금방 성장 하실거에요 :)

import calcproject.view.CalcInput;
import calcproject.view.CalcOutput;

public class CalcManagerDependencyInjectionContainer {
Copy link

Choose a reason for hiding this comment

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

DependencyInjectionContainer라는 네이밍은 너무 스프링 기술에 치중되어 있는 느낌이 드는데요,
실제로 스프링의 기술을 구현해서 사용하는 것이 아니라면, 다른 이름이 좋아보여요

Comment on lines 14 to 22
CalcExpressionTokenizer calcExpressionTokenizer = new CalcExpressionTokenizer();
Calculator calculator = new Calculator(calcExpressionTokenizer);

CalcManager calcManager = new CalcManager(calcResultRecordRepository, calcConsoleView, calcConsoleView,
calculator);
CalcManagerViewFactory calcManagerViewFactory = new ConsoleViewCalcManagerViewFactory();
CalcResultRecordRepositoryFactory calcResultRecordRepositoryFactory = new InMemoryCalcResultRecordRepositoryFacotry();

CalcManagerDependencyInjectionContainer calcManagerDependencyInjectionContainer =
new CalcManagerDependencyInjectionContainer(calcResultRecordRepositoryFactory, calcManagerViewFactory,
calculator);
Copy link

Choose a reason for hiding this comment

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

모든 객체가 main을 통해서 생성되고 있어서, main과 강한 의존성을 가지겠네요.
구조를 짜고 계층형으로 분리하면 좀 더 좋을 것 같아요.

@@ -12,9 +12,9 @@ public class MemoryCalcResultRecordRepository implements CalcResultRecordReposit
private Map<Integer, CalcResultRecordModel> calcMap;
Copy link

Choose a reason for hiding this comment

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

입력과 조회(모든 레코드) 두 가지 기능만 사용하고 있는데 굳이 Map을 쓰신 이유가 있나요?

@@ -15,7 +15,7 @@ public enum Command {

public static Command valueOf(int choiceNum) {
return Arrays.stream(values())
.filter(value -> value.equals(choiceNum))
.filter(value -> value.getCmdIdx() == choiceNum)
Copy link

Choose a reason for hiding this comment

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

비교하는 메서드를 따로 만드는건 어떨까요?

@@ -15,7 +15,7 @@ public enum Command {

public static Command valueOf(int choiceNum) {
return Arrays.stream(values())
.filter(value -> value.equals(choiceNum))
.filter(value -> value.getCmdIdx() == choiceNum)
.findAny()
.orElse(CALCULATE.EXIT);
Copy link

Choose a reason for hiding this comment

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

CALCULATE.EXIT는 사용자 의도로 인해 종료한다는 느낌인데요
강제종료를 의도하신거라면 TERMINATE는 어떠신가요?

Comment on lines 35 to 37
if (stackPeekOPerator == Operator.UnSupportedOp) {
return null;
}
Copy link

Choose a reason for hiding this comment

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

List을 반환값으로 가지고 있는데 null을 반환하는 이유가 있을까요?

Comment on lines 59 to 60
for (int i = 0; i < postFixNotationTokens.size(); i++) {
String token = postFixNotationTokens.get(i);
Copy link

Choose a reason for hiding this comment

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

for each를 사용해도 되겠네요

Comment on lines +68 to +69
double operand2 = stack.pop();
double operand1 = stack.pop();
Copy link

Choose a reason for hiding this comment

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

stack이 비어있다면 어떻게 되나요?

Comment on lines 18 to 24
private static Stream<Arguments> expressionProvider() {
return Stream.of(
Arguments.of("1 + 2 * 3 / 4", Arrays.asList("1", "+", "2", "*", "3", "/", "4")),
Arguments.of("10 * 4 / 1", Arrays.asList("10", "*", "4", "/", "1")),
Arguments.of("4 * 3 / 2 - 1", Arrays.asList("4", "*", "3", "/", "2", "-", "1"))
);
}
Copy link

Choose a reason for hiding this comment

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

MethodSource를 사용해보셔군요!
그런데 테스트에 어떤 값을 넣었는지 확인하기는 좀 불편한 것 같아요

Comment on lines 45 to 51
for (int i = 0; i < expressionList.size(); i++) {
String expression = expressionList.get(i);
Double calcRsult = calcResultList.get(i);

CalcResultRecordModel calcResultRecord = new CalcResultRecordModel(expression, calcRsult);
this.calcResultRecordRepository.saveCalcResultRecord(calcResultRecord);
expectedCalcRecords.add(calcResultRecord);
Copy link

Choose a reason for hiding this comment

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

이런 코드가 테스트 코드에 들어가게 되면, CalcResultRecordModel 코드가 변경되면 전혀 상관없는 MemoryCalcResultRecordRepository의 테스트 코드가 깨지게 되겠네요

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.

2 participants