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
-
\$\begingroup\$ Would you want to format the output? \$\endgroup\$Legato– Legato2015年03月03日 06:24:41 +00:00Commented Mar 3, 2015 at 6:24
-
\$\begingroup\$ Perhaps, any suggestions? Haven't given it that much thought \$\endgroup\$Phrancis– Phrancis2015年03月03日 06:26:40 +00:00Commented Mar 3, 2015 at 6:26
4 Answers 4
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.
-
\$\begingroup\$ Exception handling would be good to know, don't feel obligated though. Nevertheless, good information! \$\endgroup\$Phrancis– Phrancis2015年03月03日 06:49:49 +00:00Commented Mar 3, 2015 at 6:49
-
2\$\begingroup\$ Except that
num1
andnum2
aren't really constants, justfinal
references when instantiating an instance ofCalculate
. I'll probably go withfirst
andsecond
. \$\endgroup\$h.j.k.– h.j.k.2015年03月03日 07:44:47 +00:00Commented Mar 3, 2015 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– Legato2015年03月03日 13:00:16 +00:00Commented Mar 3, 2015 at 13:00
-
\$\begingroup\$
UNDERSCORE_CAPTIALS
are reserved for compile time constants. \$\endgroup\$Boris the Spider– Boris the Spider2015年03月03日 14:05:23 +00:00Commented Mar 3, 2015 at 14:05
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 add
and 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;
}
-
1\$\begingroup\$
Math.pow
doesn't count I guess :) I'm still making very naive mistakes, as you can see! \$\endgroup\$Phrancis– Phrancis2015年03月03日 06:16:57 +00:00Commented Mar 3, 2015 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– Legato2015年03月03日 06:21:45 +00:00Commented Mar 3, 2015 at 6:21
-
1\$\begingroup\$ Ah, good to know! \$\endgroup\$Phrancis– Phrancis2015年03月03日 06:24:10 +00:00Commented Mar 3, 2015 at 6:24
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
-
\$\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– Legato2015年03月08日 12:24:48 +00:00Commented Mar 8, 2015 at 12:24
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.
-
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
is0.0d
so this wouldn't have to be specified either. \$\endgroup\$Jeroen Vannevel– Jeroen Vannevel2015年03月03日 07:52:43 +00:00Commented Mar 3, 2015 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– 200_success2015年03月03日 08:09:49 +00:00Commented Mar 3, 2015 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\$janos– janos2015年03月03日 08:20:42 +00:00Commented Mar 3, 2015 at 8:20
-
\$\begingroup\$ Adding getters and setters for no reason goes against the stated purpose of practicing encapsulation \$\endgroup\$Ben Aaronson– Ben Aaronson2015年03月03日 14:20:36 +00:00Commented Mar 3, 2015 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– Rhuarc132015年03月03日 19:56:59 +00:00Commented Mar 3, 2015 at 19:56