3
\$\begingroup\$

i have created an object orientated Calculator without GUI.

i have provided only the most important parts of the code. I want to add another review question for those parts (especially: MathResult and Operands)

Interface Calculator

public interface Calculator {
    MathResult<?> calculate(String operation);
}

Interface OperationParser

public interface OperationParser<T> {
    boolean matches(String operation);
    Operation<T> getOperation();
}

example-implementation: SimpleCalculator

public class SimpleCalculator implements Calculator {

    private final List<OperationParser<?>> operationParsers = new ArrayList<>();

    public SimpleCalculator(){
        operationParsers.add(new SimpleInfixArithmeticOperationParser(MathContext.DECIMAL64));
        operationParsers.add(new SimpleTrigonometricOperationParser());
    }

    @Override
    public MathResult<?> calculate(String input) {
        Optional<OperationParser<?>> parser = operationParsers.stream().filter(ip -> ip.matches(input)).findAny();
        if(parser.isPresent()){
            System.out.println("calculating "+parser.get().getOperation());
            return parser.get().getOperation().calculate();
        }else {
            throw new IllegalArgumentException("no valid parser for input '"+input+"' found");
        }
    }

}

Math Result

public class MathResult<T> {

    private final T t;

    public MathResult(T t) {
        this.t = t;
    }

    @Override
    public String toString() {
        String type = t == null ? "null" : t.getClass().getSimpleName();
        return "MathResult{" + type + ':' + t + '}';
    }
}

Operation

public class Operation<T> {

    public final Operands<T> operands;
    public final String operationSymbol;
    public final Function<Operands<T>, MathResult<T>> function;

    public Operation(Operands<T>  operands,String operationSymbol, Function<Operands<T>, MathResult<T>> function){
        this.operands = operands;
        this.operationSymbol = operationSymbol;
        this.function = function;
    }

    public MathResult<T> calculate() {
        return function.apply(operands);
    }

}

Operands

public class Operands<T> {

    private final T first;
    private final T second;

    public Operands(T single) {
        this(single, null);
    }

    public Operands(T first, T second) {
        if (first == null) {
            throw new IllegalArgumentException("first (single) operand must be set");
        }
        this.first = first;
        this.second = second;
    }

    public T get() {
        return getFirst();
    }

    public T getFirst() {
        return first;
    }

    public T getSecond() {
        if (second == null) {
            throw new IllegalArgumentException("second Operand does not exist");
        }
        return second;
    }
}

main Class

public class CalculatorApp {

    public static void main(String[] args){
        final Calculator calculator = new SimpleCalculator();
        final Scanner scanner = new Scanner(System.in);
        final Pattern exitPattern = Pattern.compile("[eE][xX][iI][tT]");
        while (true) {
            String line = scanner.nextLine();
            if(exitPattern.matcher(line).matches()){
                break;
            }
            try {
                MathResult<?> result = calculator.calculate(line);
                System.out.println("result: " + result);
            }catch (Exception e){
                System.out.println("error: " + e);
            }
        }

    }
}
\$\endgroup\$
6
  • 1
    \$\begingroup\$ I'd like to comment about the classes/interfaces architecture, but you've left out too many classes/interfaces to do that, e.g. Operation. \$\endgroup\$ Jul 26 at 9:00
  • \$\begingroup\$ i found codereview.stackexchange.com/questions/224467/… tio be similar - this question didn't pop up when i was submitting my code \$\endgroup\$ Jul 26 at 9:00
  • \$\begingroup\$ @RalfKleberhoff: I will open a new Question providing those, just after lunch (it`s 11°° right now) \$\endgroup\$ Jul 26 at 9:01
  • 2
    \$\begingroup\$ Please, include all interfaces, and at least one implementation of every interface. \$\endgroup\$ Jul 26 at 9:02
  • \$\begingroup\$ i will add those asap, sorry, i^m at work \$\endgroup\$ Jul 26 at 9:06
3
\$\begingroup\$

Naming

What you call "operation", in most cases I'd rename to be an "expression".

Documentation

You create a quite complex set of classes and interfaces, and the only hint about their usage is in the class and method names. You should add Javadoc comments at least to public classes, interfaces and methods.

Interface OperationParser

public interface OperationParser<T> {
    boolean matches(String operation);
    Operation<T> getOperation();
}

getOperation() doesn't have parameters, so where does it get its input (expression text form) from? Probably, you rely on it being called only after a matches() call, and save the expression internally in some field. This goes against the expectations of callers, and will make any multi-threaded use of the parser impossible.

Generics

You often use a type variable T, but it's not clear what it's meant to be (for lack of Javadoc). My best guess is that it might be the number type you deal with, expecting e.g. Integer or Double, and then I'd recomment to specify that like <T extends Number>.

In the Operation class, you assume both operands as well as the result to be of the same type. What about dviding two integers? In math (as opposed to many programming languages), this gives a rational number, and generally not an integer.

User interface

In the SimpleCalculator.calculate() method, you print to System.out. If that's meant for debugging, use some logging framework instead (a basic one comes with the JRE: the java.util.logging package).

If it's really meant for the user, restructure your code. It's not some calulate() method's responsibility to tell something to a user. A calulate() method should calculate things and return the calculation results to the caller.

\$\endgroup\$
3
  • \$\begingroup\$ the OperationParser Interface indeed violates the contract, that felt already bad when i was writing it! thank you, this is indeed not a good design. \$\endgroup\$ Jul 27 at 4:21
  • \$\begingroup\$ Operation indeed is of Double precision for trigonometric operations, its of precision BigDecimal for arithmetic operations and of Boolean for bool operations... depending on the input - i am also thinking of a HexNumber for a Base16 calculator \$\endgroup\$ Jul 27 at 4:23
  • \$\begingroup\$ That was a very insightful Review - thank you very Much (+1 & accept) \$\endgroup\$ Jul 27 at 6:12
1
\$\begingroup\$

Naming

MathResult; I was wondering why the prefix Math in MathResult is important. Are there other types of results? If not, we can keep it simple and only name it Result.

Operation; As @Ralf Kleberhoff already points out, would Expression a better fit. The grouping of operations and operands is an expression, where an operation is a function such as addition, subtraction and so on, and an operand is a number.

OperationParser; A better fit would be ExpressionParser, like described in Operation.

Generics

In addition to @Ralf Kleberhoff, I would like to point out that operands can only have the same type. The Expression 1 + 1.5 is not possible.

(Every operand could be a Double and to display an Integer can be the work of the UI-Logic)

The Calculator

public interface Calculator {
   MathResult<?> calculate(String operation);
}

The calculator is bound to String. Instead, you can use a custom data type that abstracts String. The abstraction is already in your code: Operation (which we will call Expression):

public interface Calculator {
    Result calculate(Expression expression);
}

This method signature makes more sense: A calculator calculates a result based on an expression.

The Simple Calculator

After we changed the method signature of Calculator the SimpleCalculator will change, too.

Without the change the SimpleCalculator has four responsibilities:

MathResult<?> calculate(String input) { 
  Optional<OperationParser<?>> parser = // 1. Parse the string input
  if(parser.isPresent()){
      System.out.println(/* 2. print message */)
      return // 3. calculate the result
  }else {
      // 4. handle wrong input
  }
}

Just as a hint, we could use the methods on Optional to get a code like:

MathResult<?> calculate(String input) { 
    Result result = operationParsers.stream()
                                    .filter(ip -> ip.matches(input))
                                    .findAny()
                                    .map(parser -> parser.getOperation().calculate())
                                    .orElseThrow(() -> new IllegalArgumentException("no valid parser for input '"+input+"' found"));
    System.out.println("calculating" + result.getOperation())
}

With the change of the method signature (from The Calculator) we can reduce the number of responsibilities to two:

Result calculate(Expression expression) { 
    System.out.println(/* 1. print message */)
    return // 2. calculate the result
}

If you want to go further, we can even divide these responsibilities with the help of the Decorator Pattern:

class SimpleCalculator implements Calculator {
    Result calculate(Expression expression) { 
        return expression.calculate();
    }
}

class LogCalculationDecorator implements Calculator {
    private final Calculator calculator;

    Result calculate(Expression expression) { 
        System.out.println(/* ... */)
        return calculator.calculate(expression);
    }
}

// in main or else where
var calculator = new LogCalculationDecorator(new SimpleCalculator());
calculator.calculate(expression);

We can even add a Decorator to validate the expression; for example, to ensure that it is not a division with 0:

class ValidateCalculationDecorator implements Calculator {
    private final Calculator calculator;

    Result calculate(Expression expression) { 
        if(/* is division by zero*/) {
            throw new // exception;
        }
        
        return calculator.calculate(expression);
    }
}

// in main or else where
var calculator = new ValidateCalculationDecorator(
                 new LogCalculationDecorator(
                 new SimpleCalculator()));
calculator.calculate(expression);

It is not necessary to use the Decorator, I just want to mention that it would fit. Instead we can even simply use Dependency Injection:

class SimpleCalculator implements Calculator {
    private final Validator validator;
    private final Logger logger;

    Result calculate(Expression expression) { 
        validator.throwIfNotValid(expression)
        logger.log(expression);
        return expression.calculate();
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ i had the same idea when i first tried to implement the calculator: using a predefined Operation (well ok, Expression i might really take up that point) - But after my first implementation i moved back to String to allow any input, just as a calculator should do. If the input is valid, the calculator delivers an result, if it is not a valid input then you´ll get an error message \$\endgroup\$ Jul 31 at 19:20
  • \$\begingroup\$ The generic part is for handling different types of input... not just numbers, it might also be booleans .... but i had even further ideas, where you would provide functions (f(x) = y) which would result in another function) \$\endgroup\$ Jul 31 at 19:21
  • \$\begingroup\$ about the four responsibilities - i accidently hat that System.out. call - that should definitly be removed. and using streams is definitely the right idea, which i should follow up!! thanks for that clearing here! \$\endgroup\$ Jul 31 at 19:24
  • \$\begingroup\$ the Operation is responsible to handle mathematical incorrct input - and throw an exception, when calculated - this is intended to happen there - and it would be an overkill to manually check the input. what comes after div/zero? taking root from negative? ups, having irrational numbers? tl'dr: the operation (expression) handles that \$\endgroup\$ Jul 31 at 19:27
  • \$\begingroup\$ THANK YOU VERY MUCH for your Review - you showed a bunch of ideas that are valuabe and worth following! and a good eye on what is not so good on my approach! please keep up reviewing - your review is inspiring!! \$\endgroup\$ Jul 31 at 19:29
1
\$\begingroup\$

I want to add to the other answers and provide a different more drastic suggestion and a more generic and (I think) clean approach.

This is the perfect example where a nice class hierarchy shines.

This is also the perfect example where to use visitors.

I think in this case everything should be an expression.

First, the problem can be broken down in separate blocks:

  • Representation of the expression in memory: The Expression class hierarchy
  • Building the expressions from strings: The parser for infix notation
  • Evaluating the expressions: evaluate() method on Expression interface (or ExpressionEvaluator visitor)
  • Printing the expressions: toString() method on Expression interface (or ExpressionWriter visitor)

Get rid of MathResult and Operands. Everything can be an Expression.

interface Expression {
    Expression evaluate();
    String toString();
}

MathResult should be a Literal->Expression just like inputs, a Literal is just a number. You can also define different literals for different types if you want to be more generic (BooleanLiteral, Stringliteral, etc..)

class Int->Literal->Expression {
    int value;
    Expression evaluate() {return this};
    String toString() {
        return intToString(value);
    }
}

Operation is an expression, you will have to add classes for each operation:

abstract class BinaryExpression {
    Expression left;
    Expression right;
}

public class Addition->BinaryExpression {
     int value;
     Expression evaluate() {
          Expression r = right.evaluate();
          Expression l = left.evaluate()
          if (l and r are Int)
              return new Int( l.value() + r.value())
          else
              return new Addition(l, r);
    };

    String toString() {
        return left.toString() + " + " + right.toString();
    }
}

The Parser

The parser will turn strings into expression hierarchy, e.g:

"1 + 2" -> new Add(new Int(1), new Int(2))

A more complex parser might be able to read and create more complex expressions, e.g:

"1 * (2 + 4)" -> new Mul(Int(1),Add(Int(2),Int(4)))

There are software that helps you create parsers e.g.: javaCC

And then to use it:

Expression e = ExpressionInfixParser.parse("2 + 4");
print(e.toString()) //prints "2 + 4"
print(e.evaluate().toString()) //prints "6"

As you can see the class hierarchy is very generic and can represent many different expressions as long as you implement the operations you are interested in and the parser can read them. The interfaces are also very simple.

You can move the evaluate and toString code to separate visitors if you want to separate the representations from the logic. This can be useful, for example, if you want to be able to print your expression with different notations you can have different visitors that share a lot of code: ExpressionPrefixWriter, ExpressionInfixWriter, ExpressionPostfixWriter.

You can also easily extend the functionality with additional classes, e.g. you can have a Variable class that represent generic variables:

class Variable->Expression {
    String name;
    String toString() {
        return name;
    }
    Expression evaluate( Context context ) {
        if (context.contains(name))
            return context[name];
        else
            return this;
    }
}

And then evaluate expression in different contexts, a context can be as simple as:

class Context -> Map<String, Expression> {}

Example:

Expression e = ExpressionInfixParser.parse("a * ( 2 * 2 ) + b");

print(e.toString()) //prints "a * ( 2 * 2 ) + b"

print(e.evaluate().toString()) //prints "a * 4 + b"

Context c = {{"a", new Int(5)}}
print(e.evaluate(c).toString()) //prints "20 + b"

Context c = {{"a", Int(2)}, {"b", Int(9)}}
print(e.evaluate(c).toString()) //prints "17"

Sorry for the code syntax, it is more of a pseudocode.

\$\endgroup\$
4
  • \$\begingroup\$ using a class hierarchy instead of generics is really worth a look! might simplify some more thing. \$\endgroup\$ Aug 1 at 6:06
  • \$\begingroup\$ the parser part is a bit more bloated than the calculator - i want to make a separate review about the parsers later. i will notify you when it's online! \$\endgroup\$ Aug 1 at 6:07
  • \$\begingroup\$ Thank you for the Review you provided a nice aspekt on the Operation (ehm: Expression) ! \$\endgroup\$ Aug 1 at 6:08
  • \$\begingroup\$ If your parser becomes too complex and you want to expand the functionalities (e.g. parse functions calls, variables, etc..) you should consider using a parser generator: JavaCC or ANTLR or one of the many others. \$\endgroup\$
    – fede. dev
    Aug 2 at 13:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.