5
\$\begingroup\$

I am a beginner learning Java and this is a calculator program I've made. I'm looking for suggestions to improve it and things I could've done better. I don't know OOP principles much, so any and all suggestions are welcome.

import java.util.Scanner;

    public class BasicCalci {
        Scanner sc = new Scanner(System.in);
        int x = 0;
        int choice = 0;
        Maths m = new Maths();

        public static void main(String[] args) {

            BasicCalci ob = new BasicCalci();
            ob.menu();

        }

    public void menu() {

        System.out.println("    Basic Calculater      ");
        System.out.println();
        System.out.println("1. Add.");
        System.out.println("2. Subtract.");
        System.out.println("3. Multiply.");
        System.out.println("4. Divide.");
        System.out.println("5. Square Root.");
        System.out.println("6. Exponent.");
        System.out.println("7. Prime Check.");
        System.out.println("8. HCF.");
        System.out.println("9. LCM.");
        System.out.println("10. Exit.");

        choice = sc.nextInt();
        accepter(choice);

    }

    public void accepter(int c) {

        double a, b;

        switch (c) {

        case 1: {
            System.out.println("Enter the numbers you want to Add.");
            a = sc.nextDouble();
            b = sc.nextDouble();
            add(a, b);
            break;
        }

        case 2: {
            System.out.println("Enter the numbers you want to Subtract.");
            a = sc.nextDouble();
            b = sc.nextDouble();
            subtract(a, b);
            break;

        }
        case 3: {
            System.out.println("Enter the numbers you want to Multiply.");
            a = sc.nextDouble();
            b = sc.nextDouble();
            multiply(a, b);
            break;

        }
        case 4: {
            System.out.println("Enter the numbers you want to Divide.");
            a = sc.nextDouble();
            b = sc.nextDouble();
            divide(a, b);
            break;

        }
        case 5: {
            System.out.println("Enter the number for Square Root.");
            a = sc.nextDouble();
            sqrt(a);
            break;

        }
        case 6: {
            System.out.println("Enter the base number and then its power.");
            a = sc.nextDouble();
            b = sc.nextDouble();
            exp(a, b);
            break;

        }
        case 7: {
            System.out.println("List: Enter the starting and ending numbers of the range.");
            System.out.println("Single: Enter the same number twice to check if it is Prime.");
            a = sc.nextInt();
            b = sc.nextInt();
            System.out.println();
            m.primeList(a, b);
            break;
        }
        case 8: {
            m.hcfLcm(choice);
            break;
        }
        case 9: {
            m.hcfLcm(choice);
            break;
        }
        case 10:{
            System.out.printf("\nThank You \n-Shekhar Verma™ ");
            break;
        }
        default:
            System.out.print("Wrong Choice.\n\n");
            menu();
        }

    }

    public void sqrt(double a) {

        System.out.printf("Result : %.2f", (Math.sqrt(a)));
    }

    public void add(double a, double b) {

        System.out.printf("Result : %.2f", (a + b));

    }

    public void subtract(double a, double b) {

        System.out.printf("Result : %.2f", (a - b));
    }

    public void divide(double a, double b) {

        System.out.printf("Result : %.2f", (a / b));

    }

    public void multiply(double a, double b) {

        System.out.printf("Result : %.2f", +(a * b));

    }

    public void exp(double a, double b) {

        System.out.printf("Result : %.2f", +(Math.pow(a, b)));

    }

}

import java.util.Scanner;

public class Maths {

    public void hcfLcm(int c) {

        Scanner sc = new Scanner(System.in);

        System.out.println("Enter the two numbers.");
        int a = sc.nextInt();
        int b = sc.nextInt();
        System.out.println();
        int hcf = 0;
        int min = Math.min(a, b);
        for (int i = min; i >= 1; i--) {

            if (a % i == 0 && b % i == 0) {
                hcf = i;
                break;
            }
        }
        if (c == 8)
            System.out.println("HCF : " + hcf);
        else
            System.out.println("LCM : " + (a / hcf) * (b / hcf) * hcf);

        sc.close();
    }

    public void primeList(double a, double b) {

        int pcount = 0;
        int count = 0;
        for (double x = a; x <= b; x++) {

            for (int d = 1; d < x; d++) {

                if (x % d == 0) {
                    count++;
                }
                if (count > 1)
                    break;
            }
            if (count == 1) {

                if (a == b) {
                    System.out.print((int) a + " is a Prime nummber.");
                    pcount++;
                } else {
                    pcount++;
                    System.out.print((int) x + "\t");

                    if (pcount % 8 == 0) {
                        System.out.println();
                    }
                }
            }
            count = 0;

            if (a == b && pcount == 0) {
                System.out.println((int) a + " is not a Prime number.");
            }
        }
        if (pcount % 8 != 0)
            System.out.println("\n");
        else
            System.out.println();

    }

}
\$\endgroup\$
5
\$\begingroup\$

Here are few suggestions to improve your code:

  • You can get rid of variables that you are not using: line 6: int x = 0;
  • You can declare Line 8: Maths m = new Maths(); inside acceptor() as you are not using 'm' in the whole Test class.
  • Redundant code can be avoided: inside primeList() :

    if (count == 1) {
            if (a == b) {
                System.out.print((int) a + " is a Prime nummber.");
                pcount++;
            } else {
                pcount++;
                System.out.print((int) x + "\t");
    
                if (pcount % 8 == 0) {
                    System.out.println();
                }
            }
            }
    

    can be written as (since same statement is repeated in both if and else block) :

    if (count == 1) {
        pcount++;
            if (a == b) {
                System.out.print((int) a + " is a Prime nummber.");
    
            } else {
    
                System.out.print((int) x + "\t");
    
                if (pcount % 8 == 0) {
                    System.out.println();
                }
            }
            }
    
  • I would create two separate classes- one for functionality where all the operations such as add, subtract, hcf, primeList, etc are defined and other for providing menu options and results.This is in accordance with class design principles. Reference: http://howtodoinjava.com/best-practices/5-class-design-principles-solid-in-java/

Also, you can extend your functionality by accepting input from user multiple times, that is your program will show menu again after your computation for previous values is done.(Hint: use do while loop).

Hope this helps :)

\$\endgroup\$
  • \$\begingroup\$ Hey thanks! , only thing is should I print within the calculation function or return value from there and then print from where i called the calculating function in the first place. \$\endgroup\$ – CodeLearner Apr 2 '17 at 18:02
2
\$\begingroup\$

One of the easiest things to look out for when you start programming is repetition.

For example, in this select:

   case 1: {
        System.out.println("Enter the numbers you want to Add.");
        a = sc.nextDouble();
        b = sc.nextDouble();
        add(a, b);
        break;
    }

    case 2: {
        System.out.println("Enter the numbers you want to Subtract.");
        a = sc.nextDouble();
        b = sc.nextDouble();
        subtract(a, b);
        break;

    }
    case 3: {
        ....

You're performing very, very, similar things for each section. This would be a good place to focus on trying to make a more flexible "calculator" architecture.

The code you have is also going to get more difficult to maintain as it expands, and you add more functions with more inputs etc.

Here's how I'd approach it from a more "Object Orientated" approach:

The calculator's main job is to perform calculations. That's its functions. The calculator is simply a selection of functions.

Those should be your objects. I'm not super familiar with Java anymore, but this code should be mostly right.

A very simple way of doing it would be to somehow wrap these functions in a class that deals with everything related to performing a function internally.

So let's create an interface. This just enforces our classes have a certain method and makes it easier for our Calculator to treat them as similar objects. We're basically defining what these classes have in common:

public interface IFunction
{
    void Calculate();
}

And then we create the class itself:

class SubtractFunction implements IFunction{
    void Calculate() {
        System.out.println("Enter the numbers you want to Subtract.");
        a = sc.nextDouble();
        b = sc.nextDouble();
        subtract(a, b);
    }

    public void subtract(double a, double b) {
        System.out.printf("Result : %.2f", (a - b));
    }
}

So this is the object that subtracts. The calculator has no knowledge of how to subtract beyond it containing this Function. It's all encapsulated and doesn't leak into any other class's responsibility.

So you've created your SubtractFunction, AddFunction, MultiplyFunction etc etc, where are we going to put them? The calculator needs to store them, say in some kind of list:

public class BasicCalci {
    List<IFunction> functions = new List<IFunction>();

    public void PopulateFunctions() {
        functions.Add(new SubtractFunction());
        functions.Add(new AddFunction());
        functions.Add(new MultiplyFunction());
    }
   ...

We've created a list that contains these IFunctions.

Well, we could use a giant switch statement again, but that's getting back into long repeated difficult to maintain code. Well, you're using numbers, instead of a switch statement we can use the index of the array:

public void accepter(int userInput) {
    functions[userInput].Calculate();
}

That should work just like your switch statement.

But it's not very user friendly without the list of functions at the start. Well, we have a list of functions, we just don't have a user friendly name for them. We can add that to our IFunction:

public interface IFunction
{
    void Calculate();
    string GetDescription();
}


class SubtractFunction implements IFunction{
    void Calculate() {
        System.out.println("Enter the numbers you want to Subtract.");
        a = sc.nextDouble();
        b = sc.nextDouble();
        subtract(a, b);
    }

    string GetDescription() {
        return "Subtract two numbers"
    }

    public void subtract(double a, double b) {
        System.out.printf("Result : %.2f", (a - b));
    }
}

So at the beginning of the calculator you can loop through with a for loop:

for(int i = 0; i < functions.count ; i++) {
    System.out.printf(i + ". " + functions[i].GetDescription(); 
}

There's a few things we've done here. We've turned 10+ lines into 3. And probably more importantly, now it's super easy to add a new function. All you need to do is literally add a new function to your functions. It doesn't have a cascade of updates/changes you have to make to the calculator. (You could even add them dynamically from wherever you're using the Calculator if you add in a few lines) so it's easier to maintain for yourself, and it's more flexible for the users of it.

I think that's a first-step in terms of making it more OOP.

\$\endgroup\$
  • \$\begingroup\$ Please update your answer so that the method names comply with the Java Naming Conventions and start with a lowercase letter. \$\endgroup\$ – Timothy Truckle Apr 2 '17 at 17:41
  • \$\begingroup\$ Thanks for replying though i need to learn the stuff you mentioned before incorporating them. I have reducde the number of times I am taking input too. \$\endgroup\$ – CodeLearner Apr 2 '17 at 18:06

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.