Quadratic function solver + testing

Question

I wrote a simple quadratic function solver with unit tests. Can anyone check if this is okay?

Quadratic function class

public class QuadraticFunction {
    private Integer a,b,c;

    public QuadraticFunction(Integer a, Integer b, Integer c) {
        this.a = a;
        this.b = b;
        this.c = c;
    }

    public Result calculate() throws NullPointerException{
        if(calculateDelta()>0){
          return new DeltaBiggerThanZero(calculateX1(),calculateX2());
        }else if(calculateDelta() == 0){
            return new DeltaEqualsZero(calculateOneRoot());
        }else{
            return new NoSolution();
        }
    }

    private Double calculateDelta(){
        return Math.pow(b,2) - 4*a*c;
    }

    private Double rootOfDelta(){
        return Math.sqrt(calculateDelta());
    }

    private Double calculateX1(){
        return (-b-rootOfDelta())/2*a;
    }

    private Double calculateX2(){
        return (-b+rootOfDelta())/2*a;
    }

    private Double calculateOneRoot(){
        return Double.valueOf((-b)/2*a);
    }

Result class example

final public class DeltaBiggerThanZero implements Result{

    private Double x1, x2;

    public Double getX1() {
        return x1;
    }

    public Double getX2() {
        return x2;
    }

    public DeltaBiggerThanZero(Double x1, Double x2) {
        this.x1 = x1;
        this.x2 = x2;
    }
    public String printSimpleResult() {
        return "Rozwiązaniem równiania jest: x1="+x1+" x2="+x2;
    }
}

This is a test:

public class QuadraticFunctionTest {
    @Test
    public void testIfEquationHasTwoResults() throws Exception {
        QuadraticFunction quadraticFunction = new QuadraticFunction(1,-4,-5);
        Result result = new DeltaBiggerThanZero(-1.0,5.0);

        assertEquals(quadraticFunction.calculate().getX1(), result.getX1());
        assertEquals(quadraticFunction.calculate().getX2(), result.getX2());
    }

    @Test
    public void testIfEquationHasOneResult(){
        QuadraticFunction quadraticFunction = new QuadraticFunction(1,6,9);
        Result result = new DeltaEqualsZero(-3.0);

        assertEquals(result.getX1(), quadraticFunction.calculate().getX1());
    }

    @Test
    public void testIfTheresNoSolution() throws Exception {
        QuadraticFunction quadraticFunction = new QuadraticFunction(1,2,5);
        Result result = new NoSolution();
        assertEquals(quadraticFunction.calculate().getX1(), null);
        assertEquals(quadraticFunction.calculate().getX2(), null);
    }
}

Answer 1

Looks good, nice and clean. Nitpicks:

• no need to introduce a method called printSimpleResult when you can simply override toString (either way, the "print" part is a lie as the method returns the string, doesn't print it)
• missing @Override annotation in DeltaBiggerThanZero
• naming a variable "result" in the tests is a bit ambiguous, especially since it seems that you don't know what order of params assertEquals takes in (it's "expected, actual", for the record)
• why artificially limit input parameters a, b, c to integers only?
• what's the purpose of throws NullPointerException in calculate's signature? I see no way in which this exception could be triggered, and you're not required to declare unchecked exceptions (and it's a matter of discussion whether you should)

You didn't include the code for NoSolution but apparently it's an implementation that returns null for most of the fields. This begs for using Java 8's Optional. Returning null and hoping the clients will know and remember how to interpret it is a bad idea.

On the other hand, creating a class hierarchy for different kinds of solutions is rather problematic for the client (arguably this is not the scenario that calls for inheritance, which should be used judiciously). Since calculate() returns an abstract Result, the client still needs to do "sniffing" on the returned object to figure out how many (if any) results there were. They could to this either by checking via instanceof or trying to see what x1 and x2 are (if x2 is null it must be the "delta-is-zero" case, if both x1 and x2 are null it must be the "delta-is-negative" case with no solutions). Neither of these options is elegant or clean. A better solution could be to return a List<Double> that has either zero, one or two elements in it. A simple implementation that would allow you to get rid of the Result type and its subclasses could be:

public List<Double> calculate() {
    Double delta = calculateDelta();
    if (delta > 0) {
        return new ArrayList<Double>(Arrays.asList(new Double[] { calculateX1(), calculateX2() }));
    } else if (delta == 0) {
        return new ArrayList<Double>(Arrays.asList(new Double[] { calculateOneRoot() }));
    } else {
        return Collections.<Double> emptyList();
    }
}

A List has a nice default implementation of toString() so the client calling the code above could just do:

QuadraticFunction qf = new QuadraticFunction(1, -4, -5);
List<Double> results = qf.calculate();
System.out.println("Rozwiązania to: " + results);

To get the output of:

Rozwiązania to: [-1.0, 5.0]