My simplify function not working when I compile

Question

#include <iostream>
using namespace std;

struct Fraction {
    int numerator;
    int denominator;
};

int simplify(int numerator, int denominator) {
    for (int i = denominator * numerator; i > 1; i--) {  
        if ((denominator % i == 0) && (numerator % i == 0)) {  
            denominator /= i;  
            numerator /= i;  
        }  
    }        
}    
    
int output(int numerator, int denominator) {
    cout << numerator << "/" << denominator << endl;
}

int main() {
    int num;
    int den;
    cout << "numerator: "; cin >> num;
    cout << "denominator: "; cin >> den; 
    cout << endl;
    cout << "entered: " << output(num, den) << endl;
    cout << "simplified: " << simplify(num, den) << endl;
}

When I am running the code my first function is working but not the second function can you please help me with this? I have found this simplify function and trying to use it in my code but it is outputting exitted, illegal instruction. The simplify function must take struct Fraction as a parameter and return that Fraction in its reduced form. The output function must take struct Fraction as a parameter and print the fraction in the form "x/y". I am not sure how to use this struct in my code.

Example output:

numerator: 4
denominator: 8

entered: 4/8
simplified: 1/2

Can you please edit your question to add expected and actual results? — , Oct 26 '20 at 03:44
Turn on your compiler warnings. All major compilers should be able to tell you exactly what's wrong here. — chris, Oct 26 '20 at 03:46
`simplify` exhibits undefined behavior, by way of reaching the closing brace of a non-`void` function without encountering a `return` statement. So does `output` — Igor Tandetnik, Oct 26 '20 at 03:47
you are declaring a simplify function with int return type but you dont have any return statement. if you you simply want to print the output use void as a return type of function. — abhishekrn, Oct 26 '20 at 03:50
@Chipster I have edited the question. You can see example output. — Angaste Dandil, Oct 26 '20 at 03:55
[Why should I always enable compiler warnings?](https://stackoverflow.com/questions/57842756/why-should-i-always-enable-compiler-warnings) — JaMiT, Oct 26 '20 at 03:58
Thanks for the edit. As a side note, somewhat un-related to your problem, why do you declare a `Fraction` struct but not use it? That kind of gives me the feeling that you either have a design problem, or just aren't using your design properly. — , Oct 26 '20 at 04:00
@Chipster Yeah I must use this struct Fraction in my code. I have edited the question. — Angaste Dandil, Oct 26 '20 at 04:22
@AngasteDandil by the way shouldn'y you be using [Euclidean Algorithm](https://en.wikipedia.org/wiki/Euclidean_algorithm) for this? There is no reason to check every number between 1 and numerator*denominator. As there can't be a multiple of A less than every A apart. — beothunder, Oct 26 '20 at 05:13
@AngasteDandil Even if you want to stick to your brute force approach (rather than Euclid's), you could (in most cases) greatly reduce the number of iterations by starting at `std::min(denominator, numerator)` instead of `denominator * numerator`. No positive integer can be divided evenly by a larger integer. — JaMiT, Oct 26 '20 at 20:55

Skeff Igor · Answer 1 · 2020-10-31T15:12:39.807

In the output function, you declare that it is of type int, but does not return any variables. Just like in the simplify function. I also realized that the simplification cycle is much longer than necessary.

The struct at the beginning of the code is not being used, to use it just declare it as if it were a type of variable.

Correcting the errors mentioned above and implementing the struct in the code, your code should look like this:

#include <iostream>
using namespace std;

struct Fraction {
    int numerator;
    int denominator;
};

Fraction simplify(Fraction numbers) {
    for (int i = numbers.denominator; i > 1; i--) {
       if ((numbers.denominator % i == 0) && (numbers.numerator % i == 0)) {
            numbers.denominator /= i;
            numbers.numerator /= i;
        }
    }
    return numbers;
}

int main()
 {
    Fraction numbers;
    Fraction num_simplify;
    cout << "numerator: "; cin >> numbers.numerator;
    cout << "denominator: "; cin >> numbers.denominator;
    cout << endl;
    num_simplify = simplify(numbers);
    cout << "entered: " << numbers.numerator << "/" << numbers.denominator << endl;
    cout << "simplified: " << num_simplify.numerator << "/" << num_simplify.denominator << endl;
}

Note that I simply declared a Fraction type variable number, and when I wanted some information from it I simply called numbers dot the name of the variable I want.

Also note that the output function was unnecessary, because it would return the values you were sending to it. That's why I took it out of the code.

In addition it was also necessary to return some variable in the simplify function, which in this case is returning a struct.

I also changed the loop that used to go from the denominator * numerator to 1 and now goes from the denominator to 1 only.

That Fixes the OP's problem, but if someone actually wants to use this, they should probably use a fast algorithm to find the Greatest Common Denominator, and then divide the denominator and numerator by the GCD. Euclid's algorithm would do. https://en.wikipedia.org/wiki/Greatest_common_divisor#Euclid's_algorithm — gmatht, Oct 26 '20 at 07:51
Why pick the numerator instead of the denominator to control the loop? — JaMiT, Oct 26 '20 at 20:59

My simplify function not working when I compile

1 Answers1