current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

I am trying to come up with a simple basic calculator but am supposed to work with the following constructors and methods:

simpleCalc();
simpleCalc(double op1, double op2);
double add();
double add(double op1, double op2);
double mult();
double mult(double op1, double op2);
double exp();
double exp(double op1, double op2);
void show();
void show(int dp);

I have written down some working code and I just want to know if am doing the right thing.

MyClass.java

package basicjavacalculator;

public class MyClass {
public static void main(String[] args) {
// TODO code application logic here
simpleCalc sc = new simpleCalc();
sc.show();

 }

}

simpleCalc.java

package basicjavacalculator;

import java.util.Scanner;

public class simpleCalc {

double add(){
return 0;
}

double mult(){
return 0;
}

double exp(){
return 0;
}

simpleCalc(){

}

void show(){

Scanner userInput = new Scanner(System.in);

System.out.println("=================Welcome to SimpleCalcDemo=================");
System.out.println("\n");

System.out.println("Enter first number: ");
double op1 = userInput.nextInt();

System.out.println("Enter second number: ");
double op2 = userInput.nextInt();

System.out.print("Enter operation to perform (+,x,^):");
String operation= userInput.next();

if(operation.equals("+")){
    System.out.println(add(op1, op2));
}

else if (operation.equals("x")){
    System.out.println(mult(op1, op2));
} 

else if (operation.equals("^")){
    System.out.println(exp(op1, op2));
}
else{
System.out.println("The operation is not valid.");

}

}

double add(double op1, double op2){
return op1 + op2;
}

double mult(double op1, double op2){
return op1 * op2;        
}

double exp(double op1, double op2){
double result = 1;
while (op2 > 0){
    result = result * op1;
    op2--;
}

return result;
}

}
share|improve this question

closed as off-topic by Morwenn, Hosch250, Mat's Mug♦, Legato, alexwlchan Jul 5 at 7:28

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Morwenn, Hosch250, Mat's Mug, Legato, alexwlchan
If this question can be reworded to fit the rules in the help center, please edit the question.
    
I have rollbacked the last edit, not sure why OP chose to remove the original code... –  h.j.k. Jun 29 at 2:14
2  
I've voted to put this on hold because the implementation is incomplete. –  RubberDuck Jun 29 at 11:01

2 Answers 2

up vote 3 down vote

This can be simplified:

double exp(double op1, double op2){
double result = 1;
while (op2 > 0){
    result = result * op1;
    op2--;
}

return result;
}

You should be using the Math.pow() method:

double exp(double op1, double op2){
    return Math.pow(op1, op2);
}

Also, you should use proper indentation to help you read and understand your code better, which will help prevent bugs.

This doesn't do anything:

simpleCalc(){

}

It is best to remove code like this to make your project easier to understand and maintain.

share|improve this answer
    
i understand simpleCalc() does not do anything but i have to use it in my code –  Richmahnn Mar 17 at 17:56
up vote 2 down vote

If you only scan for int operands, then you should probably store those operands in int variables.

Your exp(…, …) function fails with negative or non-integer exponents. The simple solution would be to use Math.pow(…, …), but at the least you should validate your inputs to avoid returning incorrect results.

The no-argument forms of add(), mult(), and exp() are useless, and should be eliminated.

show() isn't quite the right name, as there is no corresponding hide() operation.

The SimpleCalc object maintains no state, so there is no need to instantiate an object, and all of the functions should be declared static. There is also no need to make another class just to contain the main() function.

import java.util.Scanner;

public class SimpleCalc {
    /**
     * Suppresses the default constructor.
     */
    private SimpleCalc() {}

    public static void run() {
        Scanner userInput = new Scanner(System.in);

        System.out.println("=================Welcome to SimpleCalcDemo=================\n\n");

        System.out.println("Enter first number: ");
        int op1 = userInput.nextInt();

        System.out.println("Enter second number: ");
        int op2 = userInput.nextInt();

        System.out.print("Enter operation to perform (+,x,^):");
        String operation = userInput.next();

        if (operation.equals("+")) {
            System.out.println(add(op1, op2));
        } else if (operation.equals("x")){
            System.out.println(mult(op1, op2));
        } else if (operation.equals("^")){
            System.out.println(exp(op1, op2));
        } else {
            System.out.println("The operation is not valid.");
        }
    }

    private static long add(int op1, int op2) {
        return (long)op1 + op2;
    }

    private static long mult(int op1, int op2) {
        return (long)op1 * op2;        
    }

    private static long exp(int op1, int op2) {
        if (op2 < 0) {
            throw new IllegalArgumentException("Negative exponents are not supported");
        }
        long result = 1;
        while (op2 > 0){
            result = result * op1;
            op2--;
        }
        return result;
    }

    public static void main(String[] args) {
        SimpleCalc.run();
    }
}
share|improve this answer

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