12
\$\begingroup\$

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. :)

\$\endgroup\$
7
  • 4
    \$\begingroup\$ While it's great that a review enabled you to improve your code, please do not update the code in your question to incorporate feedback from answers. Doing so goes against the Question + Answer style of Code Review, as it unfortunately invalidates the existing review(s). This is not a forum where you should keep the most updated version in your question, so I rolled your changes back to the previous version. Please see see what you may and may not do after receiving answers for ways to announce your new code. \$\endgroup\$
    – Martin R
    Nov 9, 2021 at 20:23
  • 1
    \$\begingroup\$ Alright, thank you :). I thought that updating the answer with the updated code would help people which would like to learn by my mistakes. For the sake of consistency, ofc, won't do again. \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 20:26
  • 3
    \$\begingroup\$ Feel free to post a follow-up question, or post your new code with a summary of which changes you made and why. \$\endgroup\$
    – Mast
    Nov 9, 2021 at 20:38
  • \$\begingroup\$ Alright, thank you! \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 20:46
  • 3
    \$\begingroup\$ Not a coding, but a naming problem: a, b, c are coefficients, whereas discriminant (of a quadratic equation) specifically denotes the expression b²-4ac \$\endgroup\$ Nov 10, 2021 at 18:33

5 Answers 5

10
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for your answer so much, it helped a lot :). I figured out, that maybe keeping the descriminents, delta and it's square root as intigers is not the best idea as I would later count the zero places with a wrong number, so the result would be wrong. I made all my variables float, and also rewrote the set_number_from_stdin function. Updated my question with the modified code. I might return to it and work with that scanf function. \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 15:54
  • 3
    \$\begingroup\$ The concatenation is done by the compiler, not the preprocessor. See this pre-processor output: gcc.godbolt.org/z/dEPd41hGx \$\endgroup\$ Nov 9, 2021 at 20:52
  • 2
    \$\begingroup\$ @AyxanHaqverdili It is done in translation phase 6, before phase 7 where preprocessing tokens are converted to tokens. It would be strange to say that preprocessing has ended before phase 7. However, which translation phases that "belong" to the pre-processor and which that "belong" to the compiler is muddy, especially since the pre-processor is part of the compiler. It's overall not a very meaningful topic to debate. \$\endgroup\$
    – Lundin
    Nov 10, 2021 at 7:20
  • \$\begingroup\$ @AyxanHaqverdili As for what gcc -E gives, I suppose it is the output after phase 4 where all macros are expanded. \$\endgroup\$
    – Lundin
    Nov 10, 2021 at 7:22
6
\$\begingroup\$

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.

\$\endgroup\$
3
  • \$\begingroup\$ Hello, thank you for your answer :). I've used fgets function, because I've read that usage of the scanf function is dangerous and can potentially create many hard to find undefined behaviours. \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 15:50
  • 1
    \$\begingroup\$ @Cholewka scanf is unsafe when used with the %s string format specifier, since it is vulnerable to buffer overflows in the absence of a length specifier. Conversely, using it to parse numbers is safe. Some implementations of sscanf additionally have a denial-of-service vulnerability, but this only affects sscanf, not scanf. \$\endgroup\$ Nov 9, 2021 at 16:01
  • \$\begingroup\$ Oh, that's good to know, thank you! I'm sure I won't murder myself with creating functions parsing input from stdin (which was actually pretty fun to me if you ask, for some strange reason) and just use scanf. :) \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 20:15
5
\$\begingroup\$

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
\$\endgroup\$
4
\$\begingroup\$

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;

\$\endgroup\$
6
  • \$\begingroup\$ Hello, thank you for your advices! I've taken some time to create a program taking parameters as you've suggested and pasted it into my question aswell, but I don't think that's the best way I've wrote this. Something tells me that it can be buggy, but overall I'm happy with the result, as the program is easier to test providing a function which returns the results of algebra equations. Oh, also, I'm not sure that's the best way to return an array of doubles, but that's the way I found it on some SO page. \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 20:14
  • \$\begingroup\$ I don't have time for a detailed essay, but in general when you want to return something complicated, use a struct. In this case, it would contain an int with the number of roots (0,1,2) and an array of two values, some of which will be populated based on the first field. \$\endgroup\$
    – JDługosz
    Nov 9, 2021 at 20:17
  • \$\begingroup\$ I'd definitely take a look at this approach, thank you! \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 20:21
  • \$\begingroup\$ The problem with returning a pointer to a static array is that the function is not thread-safe or re-enterent, and you can't call it again before copying the result out since the subsequent call will overwrite. int size = sizeof zero_places / sizeof *zero_places; that will not work: zero_places is not an array but a pointer, and the right value is only available at run-time anyway. \$\endgroup\$
    – JDługosz
    Nov 9, 2021 at 20:21
  • \$\begingroup\$ I took this approach in a summary listed below. :) \$\endgroup\$
    – Cholewka
    Nov 9, 2021 at 21:17
0
\$\begingroup\$

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);
    }
}
\$\endgroup\$
4
  • 6
    \$\begingroup\$ Perhaps you should post a new question, quadratic formula calculator code version 2. Your revision still has a lot of issues =) \$\endgroup\$
    – jdt
    Nov 9, 2021 at 21:52
  • \$\begingroup\$ @chux-ReinstateMonica hello, thank you for pointing these issues :). I've updated the code above with some patches. E.g checking if the a == 0. It fixes (I believe) the arithmethic issues. \$\endgroup\$
    – Cholewka
    Nov 10, 2021 at 7:09
  • \$\begingroup\$ In if (argv[3]) c = string_to_double(argv[3]); what will be the value of c if the condition is not met? Can the program continue sensibly after that? Can that condition ever be false? \$\endgroup\$
    – ilkkachu
    Nov 10, 2021 at 20:05
  • \$\begingroup\$ @ilkkachu Given the code change to if (argc < 4), argv[1], argv[2], argv[3] will not be NULL. (Yes, if argv[3] was, somehow, NULL, c would be indefinite with the find_zero_places(a, b, c); call.) \$\endgroup\$ Nov 11, 2021 at 2:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.