BigRational based on BigIntegers

For Project Euler I've implemented a BigRational. The functionality is rather limited, just things I needed or will probably need soon. The style departs a bit from the standard.

I found out that using the fancy names "numerator" and "denominator" made the code very hard to read with tons of long and broken lines full with "...ator", so I resolved to p and q in the hope that everyone can remember the only two fields.

I intentionally left out Javadoc for everything obvious (e.g., compareTo says only "Returns +1, 0, or -1 according to the specification." as these exact values are not prescribed). Also things which may change soon (e.g. toString) are left undocumented. I may have forgotten something.

The naming intentionally differs from BigInteger as "plus" and "times" are much better names for immutables. Names like "add" are to be used for methods changing the instance (cf. Collection, *Builder). There may be a MutableBigRational one day...

```
import static com.google.common.base.Preconditions.checkNotNull;

import java.math.BigInteger;

import lombok.AccessLevel;
import lombok.EqualsAndHashCode;
import lombok.Getter;
import lombok.NonNull;
import lombok.RequiredArgsConstructor;

import com.google.common.math.LongMath;

/**
* This class exactly represents an arbitrary big fraction {@code p/q}.
*
* It uses a normalized {@link BigInteger} representation assuring that {@code gdc(p, q) == 1} and {@code q > 0}.
*/
@RequiredArgsConstructor(access=AccessLevel.PRIVATE) @EqualsAndHashCode
public final class BigRational implements Comparable {
public static BigRational fraction(BigInteger p, BigInteger q) {
{
final int signumQ = q.signum();
checkDivisionByZero(signumQ);
final int signumP = p.signum();
if (signumP == 0) return ZERO;
if (signumQ
* an decimal representation of an integer number (example: {@code 123})
* two such strings separated by a slash (example: {@co

fraction

if (q <= 0) {
        checkDivisionByZero(q);
        p = -p;
        q = -q;
    }

This seems unnecessarily complex. First, you check if the denominator is non-positive (negative or zero). Then you check if it is zero, throwing an exception if it is. If it isn't, you return and do the things for the case that it is negative. The problem that I have with this is that it is not evident that you won't do the negative case if the number is zero. One has to determine that checkDivisionByZero throws an exception on zero and aborts the fraction(long, long) function in order to make sense of this.

if (q < 0) {
        p = -p;
        q = -q;
    } else {
        checkDivisionByZero(q);
    }

This logic seems more straightforward. Check if q is negative and handle that. Else check if q is 0 and presumably handle that.

This may make the q is positive case run slightly slower (assuming the compiler doesn't do something clever like run both paths simultaneously), but it speeds up the q is negative case. Since the q is negative case does two assignments, it is already slower. This version balances things. But my main reason remains that I find that this way reads more naturally.

valueOf

In the valueOf(String) comments, "an decimal" should be "a decimal".

// As the estimation is imprecise, require one bit more.
    if (pqEstimatedLength > qpEstimatedLength + 1) return signum();
    if (pqEstimatedLength < qpEstimatedLength - 1) return -signum();

I might have written this

// As the estimation is imprecise, require one bit more.
    if (pqEstimatedLength > qpEstimatedLength + 1) return signum();
    if (qpEstimatedLength > pqEstimatedLength + 1) return -signum();

As this makes the last line match the comment.

times

if (this == a) return square(); // improbable but very fast

The this == a check is very fast, but the square() is less so. It may be accurate to say that square is faster than the more general case, but it's not a huge improvement. It basically saves the greatest common denominator check. You could have said "improbable but very fast to check" which wouldn't have implied anything about square().

You also might want to consider profiling this. How improbable is it? How much slower is the normal case when they are not the same object? It might be better to accept the slower method in the rare case in exchange for even a tiny improvement in the common case.

reciprocal

public BigRational reciprocal() {
    checkDivisionByZero(q.signum());
    return new BigRational(q, p);
}

It's interesting that you check for division by zero of q here. Zero is not a valid value for q, so that should never happen. However, if it did, you'd just end up with a valid number as the reciprocal. Did you perhaps mean to check if p is 0? That would be a problem.

signum

Technically speaking, you are taking a shortcut with signum. The correct implementation would do something like return p.signum() * q.signum(). You are relying on never having a negative q so that you can check just p. This will generally work and may make your code a tiny bit faster. It does increase the possibility of error though and should get unit test coverage.

Tests

You should probably write a test for BigRational.ZERO.reciprocal() to verify that it throws an exception. While you're there, you may want to write other reciprocal tests.

Along with reciprocal, you should test valueOf("1/0") and all the fraction methods to verify that they too throw exceptions when q is 0. Note: it may seem like you only have to check two methods since the other two just call the first one. However, that's an implementation detail that may change in the future. It's much easier to write two extra tests now than to rely on someone writing those two tests when they create the need for them.

You should consider beginning with a few tests that verify value by using the getters on p and q rather than matching valueOf arguments. Under the current methodology it would be possible to write code such that the bug was on both sides and cancelled out. Or to have two different bugs cancel. Try to have at least a few tests at the beginning that check that simple creation works as expected.

assertEquals(BigInteger.ONE, valueOf("1/3").p());
assertEquals(BigInteger(-2), valueOf("-.4").p());
assertEquals(BigInteger(3), valueOf("1/3").q());

assertEquals(BigInteger.ONE, fraction(1, 3).p());
assertEquals(BigInteger(-2), fraction(2, -1).p());
assertEquals(BigInteger(3), fraction(1, 3).q());

It's easy to view a unit test as a way to test that your code works. But in my opinion the real value of a unit test is its ability to make code continue to work the same way over time. Some people read the unit tests in order to understand what your code does. They view them as compilable comments. Where

StackExchange Code Review Q#73571, answer score: 3