Return to Revisions

• BigInteger.gdc(BigInteger) always returns a non-negative value, so the line

   if(the_gcd.signum() == -1) the_gcd = the_gcd.abs();
  

in normalize() is redundant.

• Your Rational(String) constructor can be easily tricked into accepting a zero denominator:

   new Rational("5/00")
  

A safer way to check a String for representing an integer with the value of 0 would be to invoke Integer.parseInt(String) and then compare the returned int to 0. This would guard against all sorts of loopholes, including "-0". Alternatively, you could first create a BigInteger from the denominator String and then compare the resulting BigInteger to BigInteger.ZERO.

• The mod() method is broken: Consider a Rational where the low-order 32 bits of the numerator are 0x00000002 and the low-order 32 bits of the denominator are 0x00000001, and all higher-order bit-pairs are equal and at least one bit thereof is 1. mod() should now return 1, but it actually returns 0. It would also throw an ArithmeticException if the low-order 32 bits of the denominator are all 0, because then an illegal division by zero would be attempted. Your method would be more stable and predictable if you first calculated the modulo as a BigInteger, and only then, if you really want to return an int instead of a BigInteger, convert the result to an int.

• Your method toReal() checks the denominator for being zero. But the denominator of a Rational can/should not be zero in the first place – the constructors already see to that. If denominator is zero when toReal() is invoked, then there is a bug in your code. Instead of throwing a RuntimeException, there is a language construct specifically designed for this purpose: An assertion:

   assert !this.denominator.equals(ZERO);
  

If denominator equals ZERO, then an AssertionError will be thrown. An AssertionError is also a Throwable, so the effect is pretty much the same as if you throw an ArithmeticException, but an AssertionError carries a completely different meaning (i.e. that there's a bug in your code, as opposed to someone recklessly tried to divide by zero). The same goes for toString(). Be aware, however, that assertions are disabled by default, so the expression in the assert statement will not be evaluated unless you enable them.

• There are three bugs in your compares(Rational) method:
• In the second line, it should probably be q.numerator instead of only numerator).
• You confused the evaluations based on the signum: If expression.signum() == -1, then this is less than q, and vice versa.
• The algorithm is faulty: Comparing -2/1 to 3/-1 will produce incorrect results (it is possible that numerator is positive and denominator is negative)
• In the method toString(): Returning "" + numerator might save 6 characters of code, but, program-logic-wise, it is less direct that simply writing numerator.toString() and therefore more confusing to read.
• opposite() is a rather meaningless method name, mathematically speaking. A more informative name might be additiveInverse(), or, analogous to the BigInteger equivalent, negate().
• There's a potential typo in add(): Could it be that mcm should actually be lcm for "least common multiple"?

A final comment on using BigInteger. You list the fact that it has a built-in method for calculating the greatest common divisor as the only reason for using it. But a method for calculating the gcd is easily written in a few lines. If this is really your only reason for using BigInteger, then you might consider writing a simple method for calculating the gcd of two ints (or alternatively longs, as ponomandr has suggested), and reap the benefits of using primitive values instead of BigIntegers (e.g., as you have noticed, the use of operators). Of course, on the other hand, you would also have to deal with their limited range.

Update

• I found another bug: In divide(Rational), you need to check for q.numerator being zero, not q.denominator.