Problem

This program is a practise exercise in using instance methods, and also in recursion which I struggle to get my head around. I want to know how my technique for the recursion could be improved, and if there’s anything else which could be made more efficient.

The program creates a Rational object type (a fraction) which has numerator and denominator variables. It runs a method to find the greatest common divisor using Euclid’s algorithm, and then prints the simplified form of the fraction.

Any feedback is helpful, but things I’m interested in:

- Is there a more efficient way for me to take the two integers of input (which may both be positive or negative and in any size order) and turn them into two positive integers with the largest first? The if/else section I’ve used feels excessively big for the simplicity of the task but I couldn’t think of another way.
- Is there a way that I could combine the getGcd and reduce methods into a single method? I couldn’t work out how to make a method where a subsection of it is recursive but the rest isn’t.
- Is there anything that could be improved about my recursive Euclid’s algorithm bit? It’s pretty simple but I wonder if I missed any tricks to make it extra efficient.

Code:

```
public class Rational {
public static void main(String[] args) {
Rational test = new Rational(-462, 1071);
System.out.println(test.reduce());
}
private int nume;
private int deno;
public Rational(int nume, int deno) {
this.nume = nume;
this.deno = deno;
}
public String toString() {
return this.nume + "/" + this.deno;
}
public Rational reduce() {
int gcd = this.getGcd();
return new Rational(this.nume / gcd, this.deno / gcd);
}
public int getGcd() {
// Set up variables
int a;
int b;
int gcd;
if (Math.abs(this.nume) > Math.abs(this.deno)) {
a = Math.abs(this.nume);
b = Math.abs(this.deno);
} else {
a = Math.abs(this.deno);
b = Math.abs(this.nume);
}
// Euclid's algorithm
if (a%b == 0) {
gcd = b;
return gcd;
} else {
gcd = new Rational(b, a%b).getGcd();
return gcd;
}
}
}
```

Solution

First thing: let’s make the `getGcd`

`static`

:

```
public static int gcd(int a, int b){
if(a % b == 0)
return b;
else
return gcd(b, a % b);
}
```

This makes things faster if you want to stick with recursion since random instances are not made on every recursive step (otherwise, the while loop tends to win).

In order to handle the signs, I would implement the following (to make later code more readable):

```
public static boolean isNeg(int a){
return a < 0;
}
```

and add a condition to your constructor (this would make `this.nume`

the only possible negative value, beautifying printing and making later code easier):

```
public Rational(int n, int d){
if(isNeg(n) == isNeg(d)){
this.nume = Math.abs(n);
}else{
this.nume = -1*Math.abs(n);
}
this.deno = Math.abs(d);
}
```

Then, `reduce`

would be the following:

```
public Rational reduce(){
int tNume = Math.abs(this.nume);
//Math.max and Math.min get maximal and minimal values. This is great to
//reduce the amount of code.
int gcd = gcd(Math.max(tNume, this.deno), Math.min(tNume, this.deno));
return new Rational(this.nume / gcd, this.deno / gcd);
}
```

I would actually probably always `reduce`

for efficient comparison and ease of use (especially for math).

Also: consider throwing an error if `this.deno`

is ever `0`

in the constructor.

Euclid’s algorithm for computing the greatest common divisor does not

require that the two numbers are sorted by absolute value. It works

well without that preliminary step:

```
public int getGcd() {
int a = Math.abs(this.nume);
int b = Math.abs(this.deno);
if (a % b == 0) {
return b;
} else {
return new Rational(b, a % b).getGcd();
}
}
```

Note also that the `int gcd`

variable is not needed. But this computes

the absolute values in each recursion step. Euclid’s algorithm works

with signed numbers as well, you can take the absolute value once, as the

final step:

```
public int getGcd() {
int a = this.nume;
int b = this.deno;
if (a % b == 0) {
return Math.abs(b);
} else {
return new Rational(b, a % b).getGcd();
}
}
```

But why create an instance of the `Rational`

object in each recursion

step? The GCD is a property of two (or possibly more) numbers, and

you might want to compute it in other places as well. So make it a

static function of the `Rational`

(or some other) class, taking

two integers as parameters:

```
static public int gcd(int a, int b) {
if (a % b == 0) {
return Math.abs(b);
} else {
return gcd(b, a % b);
}
}
```

and call it as

```
public Rational reduce() {
int gcd = gcd(this.nume, this.deno);
return new Rational(this.nume / gcd, this.deno / gcd);
}
```

There is still a caveat: Your constructor allows the denominator

to be zero (and the `gcd()`

function would throw a “division by zero”

exception in that case).

You should check the denominator in the constructor and throw an

exception for invalid input. You might also want to normalize the

fraction and make the denominator *positive*, in order to avoid

a result like `22/-51`

.

The `gcd()`

function itself can be modified slightly to work if some

arguments are zero:

```
static public int gcd(int a, int b) {
if (b == 0) {
return Math.abs(a);
} else {
return gcd(b, a % b);
}
}
```