[m-dev.] diff: rat - rational numbers module.

[Fergus Henderson]

On 26-Mar-1998, Vanessa Joy TEAGUE <vjteag at students.cs.mu.oz.au> wrote:
> 
> Here is the revised version of rat.m with most of the changes suggested.
...
> :- module rat.
...
> :- implementation.
...
> /* compare/3 doesn't behave correctly on rationals */ 

I think that comment should go somewhere prominent in the interface,
not just in the implementation.

If rats are not always kept normalized, then =/2 will also break.
Thus, I think you should maintain (and document!) the invariant
that rats are always normalized. 
(Unlike the previous one, this comment should go in the implementation.)

Note that normalization should be careful to pick a canonical
representation for negative numbers and zero (e.g. -(3/2) should always
be represented as (-3)/2, not 3/(-2), and zero should always be 0/1,
not 0/2 or 0/42).  Looking at your code, I think it already does that.
But it's probably worth documenting this.

> :- func ratnorm(rat) = rat.

Our usual style is to put func/pred declarations for preds/funcs
defined in the interface adjacent to their definitions.
The declaration of this function was a long way from the definition.

> rat:numer(Rat) = Num :-
> 	ratnorm(Rat) = r(Num,_).
> 
> rat:denom(Rat) = Den :-
> 	ratnorm(Rat) = r(_,Den).

If you maintain the invariant that rats are always normalized,
then the calls to ratnorm here become unnecessary.

> ratnorm(r(Num, Den)) = r(Sgn*iabs(Num)//Gcd, iabs(Den)//Gcd) :-
> 	(
> 		Num > 0,
> 		Den > 0
> 	->
> 		Sgn = 1
> 	;
> 		Num > 0
> 	->
> 		Sgn = -1
> 	;
> 		Den > 0
> 	->
> 		Sgn = -1
> 	;
> 		Sgn = 1
> 	),
> 	( Den = 0 -> error("zero denominator!") ; true),
> 	Gcd0 = gcd(iabs(Num), iabs(Den)),
> 	( Gcd0 = 0 -> error("zero gcd!") ; Gcd = Gcd0 ).

The code here is a bit inefficient, since there are a lot of tests,
particularly when you count the ones inside iabs/1.
Because ratnorm/1 is called for almost every rat operation,
it is likely to be a bottleneck.
I'd advise computing AbsNum, AbsDen, and SignTimesAbsNum inside the
if-then-else chain, and then using them instead of iabs(Num), iabs(Den),
and Sign*iabs(Num) respectively.  Also use a nested if-then-else
so that you don't have to repeat the `Num > 0' test.

Also you don't really need to explicitly check for Gcd0 = 0,
because `//' will, on almost all current hardware, raise a runtime
exception.

Changing it from ratnorm(rat) to ratnorm(int, int) would make it
clearer when the invariant about rats being normalized applies
("always", instead of "always, except for the rats that are passed
to ratnorm/1").  It may also make things more efficient.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.