debugjavaMinor
Decimal expansion of a rational number
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("}");
}
```
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.
-
The reason is that in
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:
-
The reason is that in
the last fraction digit is checked as indicator if the fraction is
repeating or not. This does not work, you have to check if
-
The reason is that you only check for a repeating remainder, but not
if the numerator becomes zero. Therefore an additional
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
is not necessary, and
can be simplified to
Taking all this into account, the
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
This can now be used as
The main program
As already mentioned in a comment, the slash does not need a pattern group:
The code duplication in the "read until input is valid loop" can be
avoided by using a
And finally, I don't see why the variables are called
instead of
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.4The reason is that you only check for a repeating remainder, but not
if the numerator becomes zero. Therefore an additional
0 is appendedto 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 becomespublic 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/bottominstead 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.