Utility Method to find the Square root of a number

Question

Utility to calculate the square root of a number. The method also accept an epsilon value, which controls the precision. The epsilon value could range to any number including zero. I am expecting a general code review or if there is a better way to write the code.

/**
 * The class MathUtil contains methods for performing basic numeric operations 
 such as the squareRoot functions. Square root of 2.0: 1.4142135623746899
 */
public class MathUtil {
 /**
 * Returns the correctly rounded positive square root of a double value.
 * Special cases: If the argument is NaN or less than zero, then the result
 * is NaN. If the argument is positive zero or negative zero, then the
 * result is the same as the argument. Otherwise, the result is the double
 * value closest to the epsilon.
 * 
 * Function takes a non-negative real number * 
 * @param a double
 * @param epsilon double
 * @return double
 */
 public static double squareRoot(final double a, final double epsilon) {
 if (a == 0)
 return a;
 else
 return internalSqrRoot(a, a / 2, epsilon);
 }
 /**
 * Method internalSqrRoot.
 * 
 * @param a double
 * @param x double
 * @param epsilon double
 * @return double
 */
 public static double internalSqrRoot(final double a, final double x,
 final double epsilon) {
 if (closeEnough(a, x, epsilon)) {
 return x;
 } else {
 return internalSqrRoot(a, betterGuess(a, x), epsilon);
 }
 }
 /*
 * return true if the current guess is close enought o accepted value
 * Method closeEnough.
 * 
 * @param a double
 * @param x double
 * @param epsilon double
 * @return boolean
 */
 public static boolean closeEnough(double a, double x, double epsilon) {
 return (Math.abs(x - ((a / x) + x) / 2)) <= epsilon;
 }
 /*
 * perform a simple manipulation to get a better guess (one closer to the
 * actual square root) by averaging y with x/y
 * Method betterGuess.
 * @param a double
 * @param x double
 * @return double
 */
 public static double betterGuess(double a, double x) {
 return ((a / x) + x) / 2;
 }
}

Answer 1

Duplicated logic

Avoid duplicated logic like this:

public static boolean closeEnough(double a, double x, double epsilon) {
 return (Math.abs(x - ((a / x) + x) / 2)) <= epsilon;
}
public static double betterGuess(double a, double x) {
 return ((a / x) + x) / 2;
}

The closeEnough method includes the exact same logic as betterGuess. Make it a habit to look at duplicated code fragments with suspicion.

If you eliminate the duplication, the code becomes:

public static boolean closeEnough(double a, double x, double epsilon) {
 return (Math.abs(x - betterGuess(a, x))) <= epsilon;
}
public static double betterGuess(double a, double x) {
 return ((a / x) + x) / 2;
}

... but then, does this actually make sense? Calling betterGuess from closeEnough? Not really. How can you check if a candidate square root is close enough? You cannot compare with the real square root, because that's the target unknown. What you can compare with, is the original number, which you should be able to get by squaring:

public static boolean closeEnough(double a, double x, double epsilon) {
 return Math.abs(a - x * x) <= epsilon;
}

This implementation is more logical, and has another positive side effect: it makes it possible to clean up the squareRoot method with the suspicious 0-check:

public static double squareRoot(final double a, final double epsilon) {
 if (a == 0)
 return a;
 else
 return internalSqrRoot(a, a / 2, epsilon);
}

The method can become simply:

public static double squareRoot(final double a, final double epsilon) {
 return internalSqrRoot(a, a / 2, epsilon);
}

It would seem the 0-check was there to prevent a division by zero problem in Math.abs(x - ((a / x) + x) / 2))

Method visibility

The MathUtil class name suggests it's a utility class, but the public MathUtil.betterGuess method (to name just one) doesn't seem very useful. It's an implementation detail that should be hidden, so make it private. Question the other methods too, and change their visibility as appropriate.

The variable names are quite poor. For example in closeEnough it's impossible to tell if the target number is a or x. target and candidate might have been better names.

Input validation

More than just commenting "Function takes a non-negative real number ", it would be better to enforce that by throwing an IllegalArgumentException.

Poor JavaDoc

This kind of JavaDoc is worse than no JavaDoc at all:

/**
 * Method internalSqrRoot.
 *
 * @param a double
 * @param x double
 * @param epsilon double
 * @return double
 */

It's worse, because tells nothing new about the method, but since it's there, I've read it, in vain.

Minor things

• It's recommended to use braces even with single-line if statements
• Redundant parentheses in the expression (Math.abs(x - ((a / x) + x) / 2))
• Perhaps instead of internalSqrRoot, squareRootHelper might be a better name. Or, since the method has a different signature than squareRoot, it can just as well be squareRoot (overloading)