HiveBrain v1.2.0
Get Started
← Back to all entries
debugjavaMinor

Decimal expansion of a rational number

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
numberdecimalexpansionrational

Problem

I wrote a program that does division and either does or doesn't show repeating groups. After designing it the first time I had to redesign it because although it worked it was unnecessarily complex. And now I am presenting to you my refined division program, for feedback and improvement ideas:

```
import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.Scanner;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class Division
{
public static void main(String[] args)
{
Pattern p = Pattern.compile("(\\d+)(/)(\\d+)");
Scanner in = new Scanner(System.in);

System.out.print("Enter a fraction in the form x/y: ");
String input = in.nextLine();
Matcher m = p.matcher(input);

while(!m.matches())
{
System.out.print("Enter a fraction in the form x/y: ");
input = in.nextLine();
m = p.matcher(input);
}
in.close();

int top = Integer.parseInt(m.group(1));
int bottom = Integer.parseInt(m.group(3));
System.out.println("Result: " + divide(top,bottom,true));
}

public static String divide(int numerator, int denominator, boolean showRepeatingGroup)
{
int whole = (int)numerator / denominator;
numerator = (numerator - (whole denominator)) 10;
StringBuilder result = new StringBuilder(whole + ".");
Set remainders = new LinkedHashSet();

while(remainders.add(numerator))
{
whole = (int)numerator / denominator;
result.append(whole);
numerator = (numerator - (whole denominator)) 10;
}

ArrayList remaindersList = new ArrayList(remainders);
if (result.charAt(result.length() - 1) != '0' && showRepeatingGroup)
{
int position = remaindersList.indexOf(numerator) + 2;
result.insert(position, "{");
result.append("}");
}

Solution

The conversion

Your program does not produce the correct output in all cases.

-
10000/3 produces 33{33.3} instead of 3333.{3}.

The reason is that in

int position = remaindersList.indexOf(numerator) + 2;


you assume that the integer part of the fraction is exactly one digit.
This can be (e.g.) solved by remembering the string length right after the integer part has been build:

StringBuilder result = new StringBuilder(whole + ".");
int initialLength = result.length();


-
120/999 produces 0.120 instead of 0.{120}

The reason is that in

if (result.charAt(result.length() - 1) != '0' && showRepeatingGroup)


the last fraction digit is checked as indicator if the fraction is
repeating or not. This does not work, you have to check if
numerator becomes zero.

-
2/5 produces 0.40 instead of 0.4

The reason is that you only check for a repeating remainder, but not
if the numerator becomes zero. Therefore an additional 0 is appended
to all non-repeating fractions.

Instead of a (ordered) hash set for the reminders which is converted
to a list later, I would use an (array) list in the first place.
Then you can check for repeating remainders and remember the position
to insert the group markers at the right place.

Other small things: The cast in

int whole = (int)numerator / denominator;


is not necessary, and

numerator = (numerator - (whole * denominator)) * 10;


can be simplified to

numerator = (numerator % denominator) * 10;


Taking all this into account, the divide() function becomes

public static String divide(int numerator, int denominator, boolean showRepeatingGroup)
{
    int whole = numerator / denominator;
    numerator = (numerator % denominator) * 10;

    StringBuilder result = new StringBuilder(whole + ".");
    int initialLength = result.length();

    List remainders = new ArrayList();
    int repeatingPos = -1;

    while (numerator > 0 && repeatingPos == -1) {
        remainders.add(numerator);
        whole = numerator / denominator;
        numerator = (numerator % denominator) * 10;
        result.append(whole);
        repeatingPos = remainders.indexOf(numerator);
    }

    if (repeatingPos >= 0 && showRepeatingGroup) {
        result.insert(initialLength + repeatingPos,  "{");
        result.append("}");
    }

    return result.toString();
}


But what I don't like is that the calculation of the fractional digits
is mixed with the conversion to a string. If we define a dedicated
DecimalFraction class then the two tasks can be separated clearly:

class DecimalFraction {
    int wholePart;      // Integer part
    List fractionDigits; // Fractional digits
    int repeatingAt;    // Position of first repeating digit, or (-1)

    DecimalFraction(int numerator, int denominator) {
        wholePart = numerator / denominator;
        numerator = (numerator % denominator) * 10;
        fractionDigits = new ArrayList();
        repeatingAt = -1;

        List remainders = new ArrayList();
        while (numerator > 0 && repeatingAt == -1) {
            remainders.add(numerator);
            int whole = numerator / denominator;
            numerator = (numerator % denominator) * 10;
            fractionDigits.add(whole);
            repeatingAt = remainders.indexOf(numerator);
        }
    }

    String toString(boolean showRepeatingGroup) {
        StringBuilder result = new StringBuilder(wholePart + ".");
        for (int i = 0; i = 0) {
            result.append("}");
        }
        return result.toString();
    }
}


This can now be used as

DecimalFraction dfrac = new DecimalFraction(numerator, denominator);
System.out.println("Result: " + dfrac.toString(true));


The main program

As already mentioned in a comment, the slash does not need a pattern group:

Pattern p = Pattern.compile("(\\d+)/(\\d+)");


The code duplication in the "read until input is valid loop" can be
avoided by using a do { } while() :

Scanner in = new Scanner(System.in);
Matcher m;
do  {
    System.out.print("Enter a fraction in the form x/y: ");
    String input = in.nextLine();
    m = p.matcher(input);
} while (!m.matches());
in.close();


And finally, I don't see why the variables are called top/bottom
instead of numerator/denominator:

int numerator = Integer.parseInt(m.group(1));
int denominator = Integer.parseInt(m.group(2));

DecimalFraction dfrac = new DecimalFraction(numerator, denominator);
System.out.println("Result: " + dfrac.toString(true));

Code Snippets

int position = remaindersList.indexOf(numerator) + 2;
StringBuilder result = new StringBuilder(whole + ".");
int initialLength = result.length();
if (result.charAt(result.length() - 1) != '0' && showRepeatingGroup)
int whole = (int)numerator / denominator;
numerator = (numerator - (whole * denominator)) * 10;

Context

StackExchange Code Review Q#87522, answer score: 4

Revisions (0)

No revisions yet.