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

Converting a decimal into a fraction

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

Problem

I have written a small program that converts a decimal number into a reduced fraction representation. I also designed this little program to be fairly robust, so that it can give the correct output for various inputs.

For example:

./a.out 3,000.375
24003/8
./a.out "3 000,375"
24003/8
./a.out 3.000,375
24003/8
./a.out 3000.375
24003/8


Here is the complete code for the program:

#include 
#include 
#include 

int isDigit(char);
void simplify(unsigned long long *, unsigned long long *);

int main(int argc, char *argv[]) {
    if (argc = 0; i--, denominator *= 10) {
        c = input[i];
        if (!isDigit(c)) {
            break;
        }
        previous = numerator;
        numerator = (c - '0') * denominator + numerator;
        if (previous > numerator) {
            return EXIT_FAILURE;
        }
    }
    if (c == '-' || isDigit(c)) {
        printf("%llu/1\n", numerator);
        return EXIT_SUCCESS;
    }
    simplify(&numerator, &denominator);
    int j, negative = 0;
    unsigned long long whole = 0;
    for (j = 0; j  whole) {
                return EXIT_FAILURE;
            }
        }
    }
    previous = numerator;
    numerator += whole * denominator;
    if (previous > numerator) {
        return EXIT_FAILURE;
    }
    simplify(&numerator, &denominator);
    if (negative) {
        printf("-");
    }
    printf("%llu/%llu\n", numerator, denominator);
    return EXIT_SUCCESS;
}

int isDigit(char c) {
    return c >= '0' && c <= '9';
}

void simplify(unsigned long long *numerator, unsigned long long *denominator) {
    while (1) {
        if ((*numerator % 2 == 0) && (*denominator % 2 == 0)) {
            *numerator   /= 2;
            *denominator /= 2;
        } else if ((*numerator % 5 == 0) && (*denominator % 5 == 0)) {
            *numerator   /= 5;
            *denominator /= 5;
        } else {
            break;
        }
    }
}


My algorithm is basically to first process the number right to left. As soon as a non-digit is

Solution

I think you're being too lenient with your input checking, which results in some unexpected results:

-3,000,0 results in: -3000/1

3,000 results in: 3/1

3 results in 3/1

3-1 results in 1/1

Personally, I'd pick an expected format and then ensure that the input complies with it. If you want to support multiple decimal marks, then I would suggest having a function for each supported format that validates the input and returns it in one common format which is then used by the rest of the code.

argc & argv

In your example you have ./a.out "3 000,375". You're quoting the space in order to ensure it's all pushed as part of argv[1]. It seems likely that this could be forgotten. If it was, then you would end up with the output of 3/1. Your argc checking only checks for no parameters being supplied:

if (argc < 2) {
    fprintf(stderr, "Please enter a number to fractionize.\n");
    return EXIT_FAILURE;
}


Unless you're going to continue through the arguments supplied, appending them to the input it would be better to do something like this:

if (argc != 2) {
    fprintf(stderr, "Usage: %s \n", argv[0]);
    return EXIT_FAILURE;
}


One declaration per line

This is obviously subjective, but I don't particularly like multiple declarations on a single line. I don't mind it so much when you're declaring several uninitialised variables, however when you're assigning values to the variables it would be better to have each declaration on a new line. For me, denominator and previous get a bit lost on this line:

unsigned long long previous, denominator = 1, numerator = 0;

Code Snippets

if (argc < 2) {
    fprintf(stderr, "Please enter a number to fractionize.\n");
    return EXIT_FAILURE;
}
if (argc != 2) {
    fprintf(stderr, "Usage: %s <number to fractionize>\n", argv[0]);
    return EXIT_FAILURE;
}
unsigned long long previous, denominator = 1, numerator = 0;

Context

StackExchange Code Review Q#134294, answer score: 7

Revisions (0)

No revisions yet.