Decimal expansion of a rational number

Question

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<Integer> remainders = new LinkedHashSet<Integer>();

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

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

        return result.toString();
    }
}

Answer 1

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<Integer> remainders = new ArrayList<Integer>();
    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<Integer> 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<Integer>();
        repeatingAt = -1;

        List<Integer> remainders = new ArrayList<Integer>();
        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 < fractionDigits.size(); i++) {
            if (showRepeatingGroup && i == repeatingAt) {
                result.append("{");
            }
            result.append(fractionDigits.get(i));
        }
        if (showRepeatingGroup && repeatingAt >= 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));

Answer 2

Looks pretty good overall. Some general, small changes I could mention:

• Some useless parenthesis.

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

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

• ArrayList uses the List Interface, so you can change

  ArrayList<Integer> remaindersList = new ArrayList<Integer>(remainders);

  List<Integer> remaindersList = new ArrayList<Integer>(remainders);

• Use packages. Even just for personal stuff. I usually do com.thisismyusername.project