11
\$\begingroup\$

I'm the newbie in Java (and programming) and I've tried to make a simple calculator. Code is barbaric, I know, so I would like to make it more beatiful.

Main.java:

import java.util.Scanner;
import java.util.Vector;

public class Main {
    static Main mainobject = new Main();
    ActionsToDo act = new ActionsToDo();
    Solution solve = new Solution();
    public static Vector<Double> v1 = new Vector<Double>();

    public void setValues(double fn, double sn) {
        solve.solveExpression(chooseOperator(), fn, sn);
        act.chooseAction(fn, sn);
    }

    public static double getFirstNumber() {
        System.out.println("Input the 1st number");
        Scanner sc = new Scanner(System.in);
        return sc.nextDouble();
    }

    public static double getSecondNumber() {
        System.out.println("Input the 2nd number");
        Scanner sc = new Scanner(System.in);
        return sc.nextDouble();
    }

    public char chooseOperator() {
        Scanner sc = new Scanner(System.in);
        System.out.println("What to do?" +
                "\n + for add" +
                "\n - for minus" +
                "\n * for multiply" +
                "\n / for divide" +
                "\n % for mod" +
                "\n ^ for first number into the power of second number");
        String operator = sc.next();
        return operator.charAt(0);
    }

    public static void main(String[] args) {
        mainobject.setValues(getFirstNumber(), getSecondNumber());
    }
}

Solution.java

public class Solution {
    static Main objectMain2 = new Main();
    public void solveExpression (char checker, double fn, double sn) {
        switch (checker) {
            case '+':
                System.out.println((fn + sn));
                objectMain2.v1.addElement((fn + sn));
                break;
            case '-':
                System.out.println((fn - sn));
                objectMain2.v1.addElement((fn - sn));
                break;
            case '*':
                System.out.println((fn * sn));
                objectMain2.v1.addElement((fn * sn));
                break;
            case '/':
                System.out.println((fn / sn));
                objectMain2.v1.addElement((fn / sn));
                break;
            case '%':
                System.out.println((fn % sn));
                objectMain2.v1.addElement((fn % sn));
                break;
            case '^':
                System.out.println(Math.pow(fn, sn));
                objectMain2.v1.addElement(Math.pow(fn,sn));
                break;
        }
    }
}

ActionsToDo.java:

import java.util.Scanner;
public class ActionsToDo {
    static Main objectMain = new Main();
    public void chooseAction(double fn, double sn) {
        Scanner sc = new Scanner(System.in);
        System.out.println("Dormammu, I came to bargain! " +
                "Wanna do some extra calculations?" +
                "\n 1 - for 'Yes'" +
                "\n 2 - for 'No'" +
                "\n 3 - to change the numbers");
        int dormammu = sc.nextInt();
        switch (dormammu) {
            case 1:
                objectMain.setValues(fn, sn);
                break;
            case 2:
                System.out.println("Okay! Nobody misses you" +
                        "\n But here's the calculations you've done so far");
                for (int i = 0; i < objectMain.v1.size();i++) {
                    System.out.print(objectMain.v1.get(i) + " ");
                }
                break;
            case 3:
                objectMain.setValues(objectMain.getFirstNumber(), objectMain.getSecondNumber());
                break;
            default:
                System.out.println("Shit, wrong answer,  you'll have to calculate " +
                        "with these numbers again and then you can do whatever you want");
                objectMain.setValues(fn, sn);

        }
    }
}

Note that the different classes are used just for training of using them

\$\endgroup\$
  • 2
    \$\begingroup\$ Hello, I have refactored your project. I started by writing an answer to your question in the code review form but I ended up not posting it as a code review, as I can not say precisely that what I implemented is totally correct. So I ended up doing it more in the desire to also ask for an evaluation of my code. the post the project on github: github.com/alexpfx/code-reviews/tree/master/src/main/java/… I made some unit tests for the code too: github.com/alexpfx/code-reviews/tree/master/src/test/java/… \$\endgroup\$ – alexpfx Feb 15 '17 at 2:22
  • \$\begingroup\$ thank you, it really give me the understanding of how code should look like. I'll use it as a pattern! \$\endgroup\$ – EzeR Feb 15 '17 at 16:59
  • \$\begingroup\$ @alexpfx please do submit your answer, It doesn't have to be perfect, just a nudge in the right direction is good. I want to upvote your answer! \$\endgroup\$ – Gemtastic Feb 15 '17 at 18:13
7
\$\begingroup\$

Glad you're taking on Java!

You should study Object Oriented Programming (OOP), which is core in Java, a little bit more to use classes in a more suitable and efficient way.

The general rule of a class is "one class one purpose" which it looks like you're trying to apply, but since you don't have any objects, it becomes a bit arbitrary.

An example would be to have a class called Calcualtor that has functions for what a calculator could do, then you could have another class called InputHandler that handles the reading from the user interface (in this case that's the terminal). It's very bad to call the Main class in all classes, it's very backwards, main is supposed to handle the other classes and there's only one Main.

Because of the structure you have here you notice that you find yourself using static a lot, we call this "static overuse". If you apply OOP properly you won't need static unless there's an intended use, like a variable that never changes and is the same for all objects of that type.

You should be fine by just rearranging your methods into objects and call them from main:

public class Main {
    public static void main(String[] args) {
        InputHandler inputHandler = new InputHandler();
        inputHandler.start();
    }
}

Input Handler:

public class InputHandler {
    /* Notice how I put the calculator and scanner up in the 
       object-scope, you can now re-use them throughout this object.
    */
    private Calculator calculator = new Calculator();
    private Scanner sc = new Scanner(System.in);
    private boolean keepOn = true;

    public void start() {
        System.out.println("Welcome to my calculator!");
        performAction(1);  // This is what we call a magic number. Don't do this, instead you could make variables for the options, or, you could make an enum.
        while(keepOn == true) {
            calculate();
        }
    }

    public void calculate() {
       int action = selectAction();
       performAction(action);
    }

    public double getFirstNumber() {
        System.out.println("Input the 1st number");
        return sc.nextDouble();
    }

    public double getSecondNumber() {
        System.out.println("Input the 2nd number");
        return sc.nextDouble();
    }

    public char chooseOperator() {
        System.out.println("What to do?" +
                "\n + for add" +
                "\n - for minus" +
                "\n * for multiply" +
                "\n / for divide" +
                "\n % for mod" +
                "\n ^ for first number into the power of second number");

        String operator = sc.next();
        return operator.charAt(0);
    }

    public int selectAction() {
        System.out.println("Dormammu, I came to bargain! " +
                "Wanna do some extra calculations?" +
                "\n 1 - for 'Yes'" +
                "\n 2 - for 'No'");
        int dormammu = sc.nextInt();
        return dormammu;
    }

    public double performCalculation() {
        char operand = chooseOperator();
        double firstNumber = getFirstNumber();
        double secondNumber = getSecondNumber();
        double result = calculator.calculate(firstNumber, secondNumber, operand);
        System.out.println("The result of " + firstNumber + operand + secondNumber + "is: " + result);
        return result;
    }

    public void performAction(int action) {
        switch (action) {
            case 1:
                performCalculation();
                break;
            case 2:
                System.out.println("Okay! Nobody misses you" +
                        "\n But here's the calculations you've done so far");
                for (int i = 0; i < calculator.calculations.size(); i++) {
                    System.out.println(calculator.calculations.get(i));
                }
                keepOn = false;
                break;
            default:
                System.out.println("Shit, wrong answer, try again!");
                calculate();
        }
    }
}

And the calculator:

public class Calculator {
    public Vector<Double> calculations = new Vector<>();

    public double calculate(double firstNumber, double secondNumber, char operand) {
        switch (operand) {
            case '+':
                calculations.addElement((firstNumber + secondNumber));
                return firstNumber + secondNumber;
            case '-':
                calculations.addElement((firstNumber - secondNumber));
                return firstNumber - secondNumber;
            case '*':
                calculations.addElement((firstNumber * secondNumber));
                return firstNumber * secondNumber;
            case '/':
                calculations.addElement((firstNumber / secondNumber));
                return firstNumber / secondNumber;
            case '%':
                calculations.addElement((firstNumber % secondNumber));
                return firstNumber % secondNumber;
            case '^':
                calculations.addElement(Math.pow(firstNumber, secondNumber));
                return Math.pow(firstNumber, secondNumber);
            default:
                System.out.println("Oops! Received an operand that's not supported! How'd that happen?");
                throw new UnsupportedOperationException();
        }
    }
}

This is just a small refactoring of your code. You should always treat user input with great care and use try-catch to make sure the input is what you expect. Your application will crash if you type no instead of 2.

Also notice how I used some recursion in performAction(int action) where if you do not get the expected answer it re-attempts to do the calculation. This however only works if the other answer is a digit. I'd recommend you try to read a String and then try to convert it to an int using Integer.valueOf("1") and catch errors if it's not a number.

\$\endgroup\$
  • \$\begingroup\$ That's a lot of duplicate code in your switch. I'd add private double addCalculationElement(double c) { calculations.addElement(c); return c; } and use return addCalculationElement(firstNumber + secondNumber) and so on in your switch. That way, you don't accidentally add a wrong value (e.g. a + b instead of a - b) to the calculations. \$\endgroup\$ – Zeta Feb 12 '17 at 13:52
  • \$\begingroup\$ Yes, I've merely reformatted OPs original code to show a more OOP approach to the existing code. There are many things to remark on and a lot of code should be re-organized and removed. \$\endgroup\$ – Gemtastic Feb 12 '17 at 14:02
  • 1
    \$\begingroup\$ thank you, Gemtastic! I really apreciate your advices and this guidance! \$\endgroup\$ – EzeR Feb 15 '17 at 17:01
5
\$\begingroup\$

General

If you want to learn to program you have to manage following things:

  1. Learn the theory of sequence, selection and iteration

  2. Train your algorithmic thinking by solving problems with the elements of 1. in a programming language of your choice by increasing difficulty

  3. Get familiar with the language mechanisms

  4. Apply programming paradigms like functional or object-oriented programming

  5. Formalize you code fragments by learning the currently identified 26 design patterns and learn to to apply them in the correct situations

  6. Organize your code by following the correct semantic of each code fragment and learn and apply the SOLID principles as they guide you through the jungle of design decisions

  7. Learn the restrictions of the language you use for a problem. Maybe in another language you are able to express the solution in another way that is more elegant.

You currently managed 1. and maybe 2. and you scratch at 3. But you are totally stuck at 4. Sometimes programming language mechanisms are overwhelming and it is difficult for beginners to apply them properly.

As you are using Java I guess you want to apply the OO programming paradigm. So that will be my assumption.

I have to say I had a hard time to figure out what this code is doing. And I think I did not get in in the whole until now. Without to insult but this could be considered as spaghetti code as input can happen everywhere and output can happen everywhere and the control flow is hard to grab. You mixed a lot of concerns while they should be better separated. You have global variables and less meaningful names.

Identifiy responsibilities

For each element in the usecase "Calculator with history" you should have a proper representation, a code fragment that is responsible for exactly one task AND is able to provide inner consistency (isolation and encapsulation).

The general responsibilities in every program:

  1. You read an input
  2. You process the input
  3. You produce an output

If you currently have no clue how to subdivide your program then this is a good first start.

Manage object dependencies and control flow

After you identified the responsibilities you have to glue the individual parts together.

Are there rules to do this? Not really. The tricky thing is you have to do it right and this is an identification problem again.

May way is to look at reality and see how the things are combined together. For me...

... a wheel is tied to a car and not vice versa. (structure consistency)

... the house is built first and then the kitchen gets built in. The house is not built around the kitchen. (process consistency)

Responsibilities

Even if the following code does not include all usecases I think it shows the idea to have at least really basic responsibilities:

public static void main(String[] args) {

    Input input = new Input();
    Output output = new Output();

    Calculator calculator = new Calculator();

    output.writeToConsole("Input the 1st number");

    calculator.setFirstOperand(input.getNumberFromKeyboard());

    output.writeToConsole("What to do?" +
            "\n + for add" +
            "\n - for minus" +
            "\n * for multiply" +
            "\n / for divide" +
            "\n % for mod" +
            "\n ^ for first number into the power of second number");

    calculator.setOperator(input.getCharFromKeyboard());

    output.writeToConsole("Input the 2nd number");

    calculator.setSecondOperand(input.getNumberFromKeyboard());

    calculator.execute();

    output.writeToHistory(calculator.getCurrentResult());
    output.writeToConsole(Double.toString(calculator.getCurrentResult()));

}
  1. "Input" represents the source of information. In your case it is the keyboard input
  2. "Output" represents where any information or result will be published
  3. "Calculator" represents... what should I say...
  4. the "main"-method puts it all together in a sequence

Input and Output are temporary design elements and should be replaced when you know better how to formulate the responsibilities. I only followed the IPO model to keep it simple

Other Todos

You should introduce validation of input parameters.

You should name your variables properly. "objectMain" or "v1" are not THAT meaningful.

You should try to avoid the "static" modifier.

\$\endgroup\$
  • 2
    \$\begingroup\$ I kind of disagree on your point 5... In my experience design patterns are "emerging" from well-written code. Having the design pattern as a goal can result in design failures and overdesigned spaghetti code. Example: I worked on production code that used a self-written Tree implementation and a Visitor pattern to calculate business numbers over a flat collection. Basically a simple loop was blown to the size of 4 classes and ca. 3k LoC. That's why teaching "design patterns" as a "good thing" is ... dangerous IMO . \$\endgroup\$ – Vogel612 Feb 12 '17 at 12:16
  • 1
    \$\begingroup\$ I didn't say that they are a "goal". If someone does then he will end up in the disaster you mentioned. Teaching them is a "good thing" . But teaching them as a "good thing" is saying nuclear power is overall good. I do not go with that either. I only say: Learn them to implement them correctly if their usage within a usecase is appropriate. \$\endgroup\$ – oopexpert Feb 12 '17 at 12:28
  • \$\begingroup\$ My personal opinion is that getting used to design patterns may also help to identify the appropriate usage within a usecase. I am also convinced that in software development I have to expect patterns. I totally agree that patterns "emerge". I do not know if we have a similar word in software engineering but with experience there comes a thing such like "kinesthetic awareness" that will help you to make a shortcut instead of going through the "emerging" process. \$\endgroup\$ – oopexpert Feb 12 '17 at 12:40

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.