Division of two values from array always return 0

Question

I have this function that is suppouse to return a number that should be or 1 or 2 or 4, but when the division happens, the variable duracion always get the value 0 as result. I have tried a lot of changes but no one was the solution.

// Converts a fraction formatted as X/Y to eighths
int duration(char* fraction)
{
    // TODO
    if (strlen(fraction) == 3)
    {
        // Asignacion de los caracteres de la fraccion a un array para 
           convertirlos en numeros despues.
        int a = atoi(&fraction[0]);
        int b = atoi(&fraction[2]);
        // Busqueda de errores --------------------------------------- 
        if ((fraction[0] != '1') && (fraction[0] != '3'))
        {
            fprintf(stderr, "octave most be formated as X/Y, where X 
                    can't be greater than 8\n");
            return 1;
        }
        else if (fraction[1] != '/')
        {
            fprintf(stderr, "octave most be formated as X/Y\n");
            return 1;
        }
        else if ((fraction[2] % 2 != 0) || (fraction[2] < 0))
        {
            fprintf(stderr, "octave most be formated as X/Y, where Y 
                    most be a positive pair number\n");
            return 1;
        }
        // Fin de busqueda de errores -------------------------------

        float duracion = (a / b) * 8;
        return duracion;
    }
    else
    {
        fprintf(stderr, "Note lenght most be formated as X/Y\n");
        return 1;
    }
}

What is `if ((fraction[0] != '1') && (fraction[0] != '3'))` supposed to mean? Seems very weird. — klutt, Aug 18 '18 at 23:49
`char* fraction` ... `int a = atoi(&fraction[0]); int b = atoi(&fraction[2]);` This code doesn't make any sense. If fraction is a single string, you can't pass it to atoi like this. If it is supposed to be a table of strings, then you are using the wrong types. — Lundin, Aug 20 '18 at 13:55

score 3 · Accepted Answer · edited Aug 19 '18 at 06:51

While there are several problems with this code, I'll first answer your immediate question. Your problem lies in the line

float duracion = (a / b) * 8;

From your code I can see that you assume a < b. This means that: a / b < 1. Since a and b are ints, you are performing an integer division, and this means that the integer result of your division is always truncated to zero.

Here is one possible solution (not sure this is what you actually want, though):

return (8.0 * a / b);

Here is another (maybe this is what your actually need?):

return b / a;

Now some general remarks (assuming your using C99).

Your question title states "Division of two values from array always return 0" when, in fact, these are values from a string. But this is irrelevant since if you would just say "Division of two integer values always return 0" then you would have Google find the answer for you.

If you assume that both parts of the fraction are always a single ASCII character, you could just write:

int a = fraction[0] - '0';
int b = fraction[2] - '0';

If this is not the assumption then using "sscanf" is preferred, since it will handle most your use cases, even fractions like "-16/8".

int a,b;
int ret = sscanf(fraction,"%d / %d", &a, &b);

Also note that this line is just wrong and will not work the way you want:

else if ((fraction[2] % 2 != 0) || (fraction[2] < 0))

In general, you should perform all check on the values "a" and "b" and not on the string.

Also it is customary to try and accept any input that looks valid and not restricting your user to some over-specified pattern. So you should not limit the length of the string to 3, and you should allow spaces between the elements.

In addition, unless this is your intent, and that should be clearly marked, you should not return a valid reply, "1" in your case, when an error was detected. Select a number like "-1" or, better yet, define an enumeration of negative values and return the specific error, instead of printing it inside the function.

P.S. A note duration more than the base is perfectly legal in music. So things like 16/8 are usually OK. But this is a musical theory and not a programming question.

Thanks for your solution @Equilibrius, was very helpfull and actually solved my problem. The thing is that I though that in C the variable getting the final result of the operation (duracion) was the one which had to be the right type (float), I never knew that all other in the operation had to be the same type. Your other comments where well acepted too, and respect to limiting the string to length to of 3 is because I'm getting this imput from a function that will always give a length of 3, that's why. Thank you very much. — ManKino, Aug 20 '18 at 22:41
@ManKino To (really) simplify, the most important variable is the one the calculation starts from. In your case "a". A "trick" I use to make sure a calculation is of the type float is to add "1.0 *" before the first calculated value (this is not always the left one). In this case all other variables can be of any type convertible to float. Regarding the string length, if your function accepts only length of 3 you might change its prototype to "int duration(char fraction[3])". The compiler will treat it EXACTLY as your code, but this is clearer to anyone who reads your code. — Equilibrius, May 21 '19 at 17:03
@ManKino Actually you should try to avoid strings when you can. Or convert them to some data as soon as possible. In your case I would do this: typedef struct { int a, b; } NoteLength; int duration(const NoteLength * fraction) {...}; bool StringToFraction(..., NoteLength * fraction); NoteLength note; if (StringToFraction(...,&note)) foo = duration(&note); This way you clearly separate between the parsing (converting from input string to internal data) and the actual handling of the data. P.S. I would appreciate if you could mark the answer as "accepted". Thanks ;-) — Equilibrius, May 21 '19 at 17:10

score 0 · Answer 2 · answered Aug 18 '18 at 23:42

0

The isssue is that both a and b are int. So it is performing int division.

Try casting to float as (float)a/b * 8

Or you can use atof instead of atoi and declare a and b as float

answered Aug 18 '18 at 23:42

Raj Sappidi

156
8

score 0 · Answer 3 · answered Aug 18 '18 at 23:50

0

Just this: float duracion = (a / b) * 8;

Modify: float duracion = (a * 1.0 / b) * 8;

Try it

answered Aug 18 '18 at 23:50

asdf

221
1
10

Division of two values from array always return 0

3 Answers3