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<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();
}
}
Solution
The conversion
Your program does not produce the correct output in all cases.
-
10000/3
produces33{33.3}
instead of3333.{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
produces0.120
instead of0.{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
produces0.40
instead of0.4
The reason is that you only check for a repeating remainder, but not
if the numerator becomes zero. Therefore an additional0
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));
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