My first C program: quadratic formula calculator code

Question

Recently I've started to learn the C programming language. This is my first program which finds zero places of quadratic formulas. Doesn't parse, just gets the discriminants, counts delta and x1, x2.

#include <stdio.h>
#include <stdlib.h>
#include <math.h>

void set_number_from_stdin(int *num, char* buf)
{
    int final_number;

    do
    {
        if (!fgets(buf, 1024, stdin))
        {
            // reading input failed
            break;
        }

        final_number = atoi(buf);
    } while (final_number == 0);

    *num = final_number;
}

int main(void) {
    int a, b, c, delta, deltaSqrt;
    float x1, x2;
    char buffer[1024];

    printf("Quadratic Formula Calculator\n\n \
        Given a function in normal form:\n \
        f(x) = ax^2 + bx + c\n\n");

    printf("Enter the `a` determinant: ");
    set_number_from_stdin(&a, buffer);

    printf("Enter the `b` determinent: ");
    set_number_from_stdin(&b, buffer);

    printf("Enter the `c` determinent: ");
    set_number_from_stdin(&c, buffer);

    printf("\n \
        With given determinents:\n \
        a = %d, b = %d, c = %d\n\n", a, b, c);

    delta = b * b - 4 * a * c;
    printf("The discriminator: %d \n", delta);

    if (delta < 0)
    {
        printf("Zero places are not in the set of real numbers.\n");
    }
    else if (delta == 0)
    {
        x1 = (float) -b / (2 * a);
        printf("Zero place: %.2f\n", x1);
    }
    else
    {
        deltaSqrt = sqrt(delta);

        x1 = (float) (-b + deltaSqrt) / (2 * a);
        x2 = (float) (-b - deltaSqrt) / (2 * a);

        printf("Zero places: %.2f and %.2f\n", x1, x2);
    }

    return 0;
}

It's a really simple program, but I don't think I avoided some bugs, as I'm not experienced in languages like this.

Besides, I'm really in love with these pointers. I was scared at first, but when I would write a program in a high-level language, I would return a value from a function and then modify my variable, but not anymore. Now I can send a memory... sort of, ID number and modify its value inside my function. It's brilliant. :)

Answer 1

Overall clear and readable. Some remarks:

• atoi is one of those standard library functions that should never be used. It is 100% equivalent to strtol family of functions, except that the latter have error handling and support for other number bases than decimal.

  Since you are using this function on user input which has not been sanitized, you should not use atoi, because it invokes undefined behavior when the input is incorrect.

• The error handling in case fgets fails should be better - you shouldn't continue execution. Similarly, when you swap out atoi for strtol, you will be able to do some more error handling as well. For simple beginner-level programs you probably just want to print an error and then exit.

• There is no obvious reason why buffer should be allocated in the caller, since it is only used internally by the function. It would have been better off as a local variable inside the function.

• In case you are aiming for floating point arithmetic, make it a habit to convert all operands in the expression to float. That eliminates the chance of accidental type-related bugs.

  x1 = (float)-b / (2.0f * (float)a);
  

  That is, unless the intention is do to part of the calculation in fixed point arithmetic. In such rare cases, split the expression in several lines.

• deltaSqrt = sqrt(delta); converts everything to fixed point integers. Is that really the intention here? Because the results will be very different compared to floating point.

• Minor issue - using \ to split strings is fishy practice - it is more readable to just to let the pre-processor do automatic string literal concatenation:

  printf("Quadratic Formula Calculator\n\n"
         "Given a function in normal form:\n"
  

  This is 100% equivalent to "Quadratic Formula Calculator\n\nGiven a function in normal form:\n"

• Minor - You are using inconsistent brace placements here: int main(void) {. This is likely caused by an IDE configured to use a certain style or give a default main() - if so, you might want to look up how to set the IDE to cooperate with you instead of working against you. It's a major waste of time to have a code editor spit out code formatting which doesn't correspond with your preferred style.

Answer 2

set_number_from_stdin has several bugs: if fgets fails, you assign the uninitialised variable final_number to *num. That’s undefined behaviour. In practice a compiler might assume that UB never happens, and it might thus for instance remove the break statement entirely.

You need to handle the input error. Since your current function has no way of signalling an error, you’ll need to change the function prototype. One common way is to return a status code (conventionally, a value ≠ 0 signals an error):

int set_number_from_stdin(int *num);

(I’ve also removed the buf parameter — Lundin’s answer explains why having it is a bad idea.)

Alternatively you might also want to return a bool.

Note that it’s not sufficient for the caller of this function to test errno, since fgets does not set errno when it encounters EOF. If you want to use errno, your function could set errno in this case; but it would still be conventional to also signal success via the return value.

Another bug is that your function set_number_from_stdin reads too much from input, and discards the buffer. This isn’t how stdin is supposed to be used. In fact, your program will fail if somebody pipes input into it. Reading stdin into a temporary buffer in this way is fundamentally a bad idea. Using (f)scanf would solve this issue.

Answer 3

Consider double

Rather than int and float objects, consider double.

Printing floating point

Use "%e" or "%g" to see more information. "%f" prints many large values with dozens of uninformative digits. "%f" prints small values as 0.000000. Using "%f" takes the float out of floating point.

// suggestions if using float math,  
printf(`"%.8e\n", x1);
printf(`"%.9g\n", x1);
// if using double math,  
printf(`"%.16e\n", x1);
printf(`"%.17g\n", x1);

Prevent severe loss of precession

When |b| is near the value of deltaSqrt, then one of -b +/- deltaSqrt loses many common leading digits. Result is an imprecise x1 or x2.

To avoid that, recall a(x-x1)(x-x2) == a*x*x + b*x + c, so a*x1*x2 = c.

// int a, b, c, delta, deltaSqrt;
...
//    x1 = (float) (-b + deltaSqrt) / (2 * a);
//    x2 = (float) (-b - deltaSqrt) / (2 * a);
//    printf("Zero places: %.2f and %.2f\n", x1, x2);

int a, b, c;
float delta, deltaSqrt;

  if (b < 0) {
    x1 = (-(float)b + deltaSqrt) / (2.0f * a);
    x2 = c/(a*x1); 
  } else if (b > 0) {
    x2 = (-(float)b - deltaSqrt) / (2.0f * a);
    x1 = c/(a*x2); 
  } else {
    x1 = +deltaSqrt / (2.0f * a);
    x2 = -deltaSqrt / (2.0f * a);
  }            

This approach does not harm results when |b| is far from deltaSqrt.

float mixed with double

Use either float constants/functions or double ones. Convert to FP before negating an int to avoid UB of -INT_MIN.

   //x1 = (float) -b / (2 * a);
   //...
   // deltaSqrt = sqrt(delta);

   // float
   x1 = -(float)b / (2.0f * a); // add f
   ...
    deltaSqrt = sqrtf(delta); // add f

   // double
   x1 = -(double)b / (2.0 * a);
   ...
    deltaSqrt = sqrt(delta); //no f

Answer 4

You should know that double is the normal floating-point type, and float is half sized and should only be used where you really need to save the space.

Your program is mostly dealing with reading input, which is not the problem you came to solve! A real utility program would take command line arguments, not prompt the user to type things. That is, you might issue the command prompt> ./quadform 34 -18.2 87 No retries to worry about; just an error if the args don't parse properly.

You should put the real work in a function, not directly in main. Then you can call it from testing code with built-in values that runs automatically. Right now you have to re-type all your tests every time you re-test after a change! Put the input and output in separate functions too.

Don't use in/out parameters. Your set_number_from_stdin has a perfectly good return value that's not even being used, so why is it functioning by changing the parameter?

Declare variables where you need them, not all at the top. This has been the standard in C for over twenty years now.

E.g. const double delta = b * b - 4 * a * c;

Answer 5

Result

Thank you all for your answers. :) I think I've come to a really satisfactory result to me. I've learned a lot. In this stadium I think I'm done with this program unless there's some bug I don't know about (may be, if I know myself).

Rewrote the original program to making it a running script instead of reading the result from stdin. Used struct to organise my result.

It was fun to me to write it and I hope you have honed your C/C++ skills, too. :) Thank you all for your answers once again! You guys are absolute chads.

Source code

#include <stdio.h>
#include <ctype.h>
#include <math.h>
#include <stdlib.h>

double string_to_double(char* str)
{
    double returned_float;
    char *endptr;

    returned_float = strtod(str, &endptr);

    if (str == endptr)
    {
        printf("error: Enter parameters in as digits.\n");
        exit(1);
    }

    // prevent junk at the end
    if (*endptr != 0)
    {
        printf("error: Enter parameters as only digits.\n");
        exit(1);
    }

    return returned_float;
}

typedef struct Roots
{
    int     n;
    double  roots[2];
} Roots;

Roots find_zero_places(double a, double b, double c)
{
    const double DELTA = b * b - 4 * a * c;
    printf("The discriminator: %.1f\n", DELTA);

    Roots root_result;
    
    if (DELTA > 0)
    {
        const double DELTA_SQRT = sqrt(DELTA);

        root_result.n = 2;
        root_result.roots[0] = ( -b + DELTA_SQRT ) / ( 2.0f * a );
        root_result.roots[1] = ( -b - DELTA_SQRT ) / ( 2.0f * a) ;
    }
    else if (DELTA == 0)
    {
        root_result.n = 1;
        root_result.roots[0] = -b / (2.0f * a);
    }
    else
    {
        root_result.n = 0;
    }

    return root_result;
}

void print_results(Roots *root_results)
{
    if (root_results->n == 0)
    {
        printf("Zero places are not in the set of real numbers.\n");
    }
    else
    {
        printf("Zero place(s): ");

        for (int i = 0; i < root_results->n; i++)
        {
            if (i != 0)
                printf(", ");

            printf("%.1f", root_results->roots[i]);
        }

        printf("\n");
    }
}

int main(int argc, char *argv[])
{
    double a, b, c;

    if (argc < 4)
    {
        printf("Please enter a, b and c parameter.\n");
        exit(1);
    }
    else
    {
        if (argv[1])
        {
            a = string_to_double(argv[1]);

            if (a == 0)
            {
                printf("When a = 0 the function is not quadratic.\n");
                exit(1);
            }
        }
        if (argv[2])
            b = string_to_double(argv[2]);
        if (argv[3])
            c = string_to_double(argv[3]);

        Roots root_results = find_zero_places(a, b, c);
        print_results(&root_results);
    }
}