-
Notifications
You must be signed in to change notification settings - Fork 156
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주차 과제: 계산기 구현 미션 제출합니다. #157
base: jay-so
Are you sure you want to change the base?
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.
재훈님 코드 잘 봤습니다.
- Map의 키 값으로 사용하도록 순서대로 하셨다고 했는데 나중에 구현된걸 보고 이야기해야할것같아요!
- Number타입으로 했다고 하셨는데 그러면 소숫점이 없을땐 int,long 아닐땐 double, float을 쓰시는건가요??
src/main/java/Calculator.java
Outdated
public Number calculator() { | ||
|
||
//1. 연산자 우선순위를 계산함 | ||
Stack<Object> stack = new Stack<>(); |
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.
타입과 네이밍을 의미있게 해주세요! 어떤 스택인지 알 수 있으면 좋아보입니다. 그리고 이름 지으실때 자료구조는 끝에 안넣으셔도 됩니다. (ex: objectStack과 같은식은 쓰지않기)
src/main/java/Calculator.java
Outdated
result = operand1 + operand2; | ||
return result; | ||
} | ||
|
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.
이건 Tip인데 안쓰는 공백이나 import문은 지우는 습관이 필요합니다.
src/main/java/Calculator.java
Outdated
int operand1 = scanner.nextInt(); //피연산자1 | ||
String operator = scanner.nextLine(); //연산자 | ||
int operand2 = scanner.nextInt(); //피연산자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.
위의 operand1,operator,operand2같은 경우는 전역변수로 하면 해당 인스턴스를 재사용할 때 상태가 유지돼서 예상치 못한 버그를 발생시킬 수 있습니다. 전역변수보단 지역변수가 더 어울려보입니다. 입력같은경우도 입력책임을 갖는 클래스나 메서드를 이용하면 더 좋아보입니다.
그리고 전역변수로 할땐 접근제어자, static, final키워드도 같이 고려해보시면 좋을것같습니다.
src/main/java/Console.java
Outdated
public class Console { | ||
|
||
public void start() { | ||
Scanner scanner = new Scanner(System.in); |
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.
Scanner같은 경우는 런타임에서 재사용성이 많기 때문에 전역변수로 써도 될것같구, private, static, final과 같은 키워드도 같이 보려해보시면 좋을것같습니다.
src/main/java/Console.java
Outdated
int selectMenu = scanner.nextInt(); | ||
|
||
//입력받은 값이 1이라면 | ||
if (selectMenu == 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.
매직넘버를 지양해주세요. 만약 1을 Enum으로 쓴다면 더 좋아보입니다! 코드상으로 1이라는 메뉴가 무엇인지 모르기 때문에 고려해주세요.
src/main/java/Memorizer.java
Outdated
//계산한 결과를 저장하는 ArrayList 클래스 - 계산 결과를 순번으로 저장함 | ||
ArrayList<Number> result = new ArrayList<>(); |
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.
Map의 Value값으로 들어가는 result인가요? 이름도 의미있게 바꿔주시면 좋을듯합니다.
src/main/java/Operator.java
Outdated
public int priority(String operator){ | ||
if(operator.equals('+')||operator.equals('-')) | ||
return 1; | ||
else if (operator.equals('*')||operator.equals('-')) { | ||
return 2; | ||
} | ||
return -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.
매직넘버인 '+'를 쓰는것보단 ENUM을 통해서 쓰는게 더 좋아보입니다. Enum안에 우선순위를 넣으면 더 좋구요!
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.
재훈님 고생하셨습니다! 2차 피드백남겼습니다
src/main/java/Console.java
Outdated
public class Console implements Input, Output { | ||
|
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.
Input과 Output을 하나로 한 이유가 있을까요? 추후 Input만 달라진 구현체가 필요하다고 하면 애매해 보입니다.
public class Application { | ||
public static void main(String[] args) { | ||
Console console = new Console(); | ||
Memorizer repository = new Memorizer(); | ||
InitCalculator initCalculator = new InitCalculator(); | ||
PostfixCalculator postfixCalculator = new PostfixCalculator(); | ||
new Calculator(console,console,repository,initCalculator, postfixCalculator).run(); | ||
} |
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.
Application이 두갠데 어디가 진짜 Application인가요??
private final PostfixCalculator postfixCalculator; | ||
public Calculator(Input input, Output output, Memorizer repository, InitCalculator initCalculator, PostfixCalculator postfixCalculator) { | ||
this.input = input; | ||
this.output = output; | ||
this.repository = repository; | ||
this.initCalculator = initCalculator; | ||
this.postfixCalculator = postfixCalculator; | ||
} | ||
public void run() { |
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.
공백으로 변수, 생성자 메서드 등을 구분하면 가독성이 더 좋아일것같습니다
case 1: | ||
output.MemoryCalculator(repository.getMemoroizer()); | ||
break; | ||
case 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.
여긴 아직 매직넘버네요..!
|
||
public class Console implements Input, Output { | ||
private final Scanner scanner = new Scanner(System.in); | ||
private static final Validation validation = new Validation(); |
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.
콘솔메뉴쪽에서 Validation하는 코드만 있는것같은데, Enum에서 처리해주셔도 될 것 같습니다.
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.
또 public static 메서드 같은 경우는 인스턴스 없이 사용 가능합니다.
public void printCalculator(int result) { | ||
System.out.println(result); | ||
} | ||
} |
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.
마지막 빈줄을 통일해주셨으면 좋겠습니다!
List<String> postfixExpression = new ArrayList<>(); | ||
|
||
for (String ch : infixCalculator.split(" ")) { | ||
if(Pattern.matches("[0-9]+",ch)){ |
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.
Pattern.Compile로 컴파일해두고 쓰시면 비용적으로 재사용시 더 좋습니다
} | ||
stack.pop(); | ||
} else { | ||
while (!stack.isEmpty() && getPrecedence(ch) <= getPrecedence(stack.peek())) { |
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.
Validation이 너무 길다면 편의 메서드도 고려하면 좋습니다
import java.util.regex.Pattern; | ||
|
||
public class InitCalculator { | ||
public static List<String> calculate(String infixCalculator) { |
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.
static 메서드인 이유가 있을까요?
switch (operand) { | ||
case "+": | ||
result = operand1 + operand2; | ||
break; | ||
case "-": | ||
result = operand1 - operand2; | ||
break; | ||
case "*": | ||
result = operand1 * operand2; | ||
break; |
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.
ENUM으로 case 해주시면 좋을것같아여 만드셔가지구
�과제하느라 고생하셨어요.
|
구현한 내용
콘솔을 통해 사칙 연산(더하기, 빼기, 곱하기, 나누기, 우선 순위(사칙연산), 맵으로 계산 이력,정규식을 구현했습니다.
🔎 요구사항
✅ 구현할 때, 궁금했던 점 + 주로 봐주셨으면 하는 점