10
\$\begingroup\$

I realize this is very beginner code, but I just today grasped the basic concepts of private variables, constructors, and such. I wrote this very simple class, and a class to test it. Any and all input welcome.

Calculate.java

import java.lang.Math.*;

public class Calculate {
    private final double num1, num2;

    Calculate(double num1, double num2) {
        this.num1 = num1;
        this.num2 = num2;
    }

    public double Add() {
        return num1 + num2;
    }
    public double Subtract() {
        return num1 - num2;
    }
    public double Multiply() {
        return num1 * num2;
    }
    public double Divide() {
        return num1 / num2;
    }
    public double Exponent() {
        return Math.pow(num1, num2);
    }
}

TestCalculate.java

class TestCalculate {
    public static void main(String[] args) {
        double num1 = 5, num2 =7;
        Calculate calc = new Calculate(num1, num2);
        // show outputs from calculations
        System.out.println("Number 1: " + num1);
        System.out.println("Number 2: " + num2);
        System.out.println("Addition: " + calc.Add());
        System.out.println("Subtraction: " + calc.Subtract());
        System.out.println("Multiplication: " + calc.Multiply());
        System.out.println("Division: " + calc.Divide());
        System.out.println("Exponent: " + calc.Exponent());
    }
}

Sample output using given variables:

Success time: 0.1 memory: 320256 signal:0
Number 1: 5.0
Number 2: 7.0
Addition: 12.0
Subtraction: -2.0
Multiplication: 35.0
Division: 0.7142857142857143
Exponent: 78125.0

\$\endgroup\$
  • \$\begingroup\$ Would you want to format the output? \$\endgroup\$ – Legato Mar 3 '15 at 6:24
  • \$\begingroup\$ Perhaps, any suggestions? Haven't given it that much thought \$\endgroup\$ – Phrancis Mar 3 '15 at 6:26
9
\$\begingroup\$

On Formatting

You can format your output using System.out.printf , or String.format()

For example, if you wanted your division test result to only include 2 digits after the decimal you could instead call System.out.printf("Division: %.2f%n", calc.divide());

The % is a special character; when the printf method reaches it it replaces that location with the specified formatted version of the proceeding arguments (You can have several in one line as long as they are in order). The '.2' that proceeds is what enforces only 2 digits after the decimal. The %n that immediately follows is so you can still have a new line after calling printf. You can read more about formatting and find some additional examples here.

On Exception Handling

I'll also echo the previous answer in saying that handling edge cases would do you some good, this could be done with a simple conditional within the method to ensure the divisor is not 0, which would otherwise cause an error or report NaN/infinity when it is undefined.

You could throw an exception if the divisor is 0 within the divide method itself, like so:

public double divide() {
        if (num2 == 0) {
            throw new ArithmeticException("Dividing by 0 is undefined");
        }

        return num1 / num2;
    }

The message could be a simple "Cannot divide by 0." Which, if you catch it, would allow a 0 divisor to exist, and you don't have to constrict the possible num2 values and calculations possible.

On Java Convention & Readability

  • variables and methods are camelCase
  • Class and interface name are PascalCase
  • CONSTANTS are UNDERSCORE_CAPITALIZED

Also, try to have a line break between your methods, it'll aid in readability once method body increases. Though I personally omit this when the methods are closely linked, like getters and setters for the same value, as @Rhuarc13 suggest you have.

Although I disagree with adding anything not explicitly necessary(any user trying to pass a no arg constructor would encounter a compilation error) since this is an exercise on encapsulation I think a getter has some merit. Setters don't make sense in this case given your variables' final state, but if you instantiated several of these Calculate objects with different values it would be helpful to have a method to keep track of them without providing users with access to the variable itself, which would break encapsulation*.

  • Access to the variable directly is arguably inconsequential since it's final, but the point of encapsulation is that the external users should not concern themselves with the fields or implementation.

If you go in your calculate and added methods like so:

public double getNum1() {
    return this.num1;
}

public double getNum2() {
    return this.num2; // this keyword refers to that particular object
}

Then when you're interested in what value is actually contained within the object for example in your test case you could call

System.out.println("Number 1: " + calc.getNum1());
System.out.println("Number 2: " + calc.getNum2());

Alternative Implementation

If you were interested in setters as well, if you had private instead of final values you could use something like this:

public class Calculator {
    private double num1;
    private double num2;

    public Calculator setNum1(double num1) {
        this.num1 = num1;
        return this;
    }

    public Calculator setNum2(double num2) {
        this.num2 = num2;
        return this; // return the current state of Calculator to allow for chaining
    }

    public double divide() {
        return num1 / num2;
    }

    public double add() {
        return num1 + num2;
    }

    public static void main(String[] args) {
        // You can chain call them
        Calculator example = new Calculator().setNum1(3).setNum2(5);

        System.out.println(example.add());
        // Or use it separately.
        example.setNum1(45);
        System.out.println(example.divide());
    }
}

On User Input + Further input validation

Your revision seems concerned with user input, if you actually want to get and use input, a simple way to do that is to make use of Scanner.

Make sure to retrieve the resource at the top: import java.util.Scanner and then declare an instance: Scanner input = new Scanner(System.in)

The nice thing about that is you could ensure a non-0 divisor with a while loop.

for instance if in your main method you did:

System.out.print("Enter second value: ");
double val2 = input.nextDouble();

You could proceed with this:

while (val2 == 0) {
    System.out.print(
        "Second value cannot be 0" +
        "\nEnter a non-0 value: ");

    val2 = input.nextDouble();
}

and that would make sure that your user provided a non 0 divisor.

\$\endgroup\$
  • \$\begingroup\$ Exception handling would be good to know, don't feel obligated though. Nevertheless, good information! \$\endgroup\$ – Phrancis Mar 3 '15 at 6:49
  • 2
    \$\begingroup\$ Except that num1 and num2 aren't really constants, just final references when instantiating an instance of Calculate. I'll probably go with first and second. \$\endgroup\$ – h.j.k. Mar 3 '15 at 7:44
  • \$\begingroup\$ Their naming could be improved, I agree. Also, saying "constant" could imply a shared state with all objects which is not the case. But these are primitives, not references, which makes them constant variables. \$\endgroup\$ – Legato Mar 3 '15 at 13:00
  • \$\begingroup\$ UNDERSCORE_CAPTIALS are reserved for compile time constants. \$\endgroup\$ – Boris the Spider Mar 3 '15 at 14:05
6
\$\begingroup\$

You don't need import java.lang.Math.*; at all, because you don't use any classes in the Math namespace.


private final double num1, num2;  

Declaration of multiple variables on the same line reduces the readability of the code.


Methods should be named starting with a lower case letter.
Class names should be made out of nouns or noun phrases but Calculate is a verb. So better name it Calculator.


By calling divide() with a passed in num2 == 0 the code will throw an ArithmeticException, so you better check this edge case.


Basically passing in the constructor 2 numbers and then executing addition etc doesn't make that much sense. Passing in the constructor one number which isn't final also and then add input parameters to the methods would be more natural.

If you had a calculator, you wouldn't start with entering 2 numbers and then clicking e.g. add on them. You would enter one number and then click addand then the other number.

Like:

private double result = 0.0d;

Calculate(double num1) {
    this.result = num1;
}  
Calculate() { } 

public double multiply(double value) {
    result *= value;
    return result;
}  
\$\endgroup\$
  • 1
    \$\begingroup\$ Math.pow doesn't count I guess :) I'm still making very naive mistakes, as you can see! \$\endgroup\$ – Phrancis Mar 3 '15 at 6:16
  • 1
    \$\begingroup\$ Math.pow technically counts, it's just that java lang is built in. Unless you are using a subclass of Math, you don't need to import anything. \$\endgroup\$ – Legato Mar 3 '15 at 6:21
  • 1
    \$\begingroup\$ Ah, good to know! \$\endgroup\$ – Phrancis Mar 3 '15 at 6:24
2
\$\begingroup\$

Revised code

For reference, I am posting the revised code showing the improvements I made, all thanks to the reviews!

Here is what I applied (thank you @Legato, @Heslachler, @Rhuarc13!)

  • Used better naming and formatting of class, method and variable names

  • Used better output formatting using System.out.printf()

  • Added non-zero check

  • Added getters and called those from main instead of local variables

Calculator.java

public class Calculator {
    private final double NUM_1;
    private final double NUM_2;

    Calculator(double num1, double num2) {
        this.NUM_1 = num1;
        this.NUM_2 = num2;
    }

    // Getters
    public double getNum1() {
        return this.NUM_1;
    }
    public double getNum2() {
        return this.NUM_2;
    }

    // Calculation methods
    public double add() {
        return NUM_1 + NUM_2;
    }
    public double subtract() {
        return NUM_1 - NUM_2;
    }
    public double multiply() {
        return NUM_1 * NUM_2;
    }
    public double divide() {
        return NUM_1 / NUM_2;
    }
    public double exponent() {
        return Math.pow(NUM_1, NUM_2);
    }
}

TestCalculator.java

class TestCalculator {
    public static void main(String[] args) {
        double num1 = 3.74;
        double num2 = 6.51482;

        if (num1 == 0 || num2 == 0) {
            System.out.println("Error: Please enter only non-zero values");
        } else {
            Calculator calc = new Calculator(num1, num2);
            // show outputs from calculations
            System.out.printf("Number 1: %.4f\n", calc.getNum1());
            System.out.printf("Number 2: %.4f\n", calc.getNum2());
            System.out.printf("Addition: %.4f\n", calc.add());
            System.out.printf("Subtraction: %.4f\n", calc.subtract());
            System.out.printf("Multiplication: %.4f\n", calc.multiply());
            System.out.printf("Division: %.4f\n", calc.divide());
            System.out.printf("Exponent: %.4f\n", calc.exponent());
        }
    }
}

New output formatting:

Number 1: 3.7400
Number 2: 6.5148
Addition: 10.2548
Subtraction: -2.7748
Multiplication: 24.3654
Division: 0.5741
Exponent: 5397.0367

\$\endgroup\$
  • \$\begingroup\$ Apologies for the confusion, here NUM_1 should be num1. This was due to my own misunderstanding, but I can now say with certainty that the capitalized and underscored naming is reserved for constants which would be a value that is the same across all instances of Calculator. (Like PI = 3.14). Essentially, reserve it for something that's static final and instantiated right away. I also realized I forgot to add the Exception handling, which I'll add to my answer shortly. \$\endgroup\$ – Legato Mar 8 '15 at 12:24
-2
\$\begingroup\$

You should add a default constructor.

Calculate() { num1 = 0.0; num2 = 0.0; }

Default constructors help just in case some one creates a calculator object, but doesn't pass in any variables. Another thing to add is getters and setters. For example:

public void setNum1(double newNum1) { num1 = newNum1; }
public double getNum1() { return num1; }

Getters and setters allow you to change or view variables later on. They are extremely useful.

\$\endgroup\$
  • 2
    \$\begingroup\$ Why "should" this? Now you allow the creation of a calculator with default values, that's useless. It makes no sense to create a calculator without any values. It also hides the DivideByZeroException because you as user never passed in the 0. The default value of a double is 0.0d so this wouldn't have to be specified either. \$\endgroup\$ – Jeroen Vannevel Mar 3 '15 at 7:52
  • 1
    \$\begingroup\$ @JeroenVannevel I agree that defaulting the operands to 0 is of questionable benefit. Note, however, that division by 0.0 is not an error. Rather, it produces infinity. \$\endgroup\$ – 200_success Mar 3 '15 at 8:09
  • 1
    \$\begingroup\$ You shouldn't add something you don't need. Don't add a constructor with default arguments if you don't need it. Don't add getters and setters if you don't need them. Don't recommend to add unnecessary elements, as it unnecessarily increases the complexity of the program. Your argument of "just in case some one creates a calculator object, but doesn't pass in any variables" doesn't hold, as that would be a compilation error. \$\endgroup\$ – Stop ongoing harm to Monica Mar 3 '15 at 8:20
  • \$\begingroup\$ Adding getters and setters for no reason goes against the stated purpose of practicing encapsulation \$\endgroup\$ – Ben Aaronson Mar 3 '15 at 14:20
  • \$\begingroup\$ Part of encapsulation is making your code robust to user error, along with making data manipulatable. I agree that its better to have non default constructors in this example. However, including at least setters would be beneficial for reusability of the class. Instead of creating a whole new class and initializing it with different values, using setters would allow you to use the same object again. As for dividing by zero, making the divide() method check for this user error would make the code more robust. \$\endgroup\$ – Rhuarc13 Mar 3 '15 at 19:56

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.