1
\$\begingroup\$

I've started to get far more familiar with C (and coding in general) and the way to use function to my advantage to create more complex programs. As a way of doing something actually useful instead of my "Starting Programming with C: For Dummies" book I decided to make a simple program to help me cheat my way through maths class, a program that calculates the quadratic equation.

However due to the fact that I've never seen a piece of code that is longer than 60 lines outside of a youtube videos and my for Dummies book, I don't exactly know if the way I format functions is readable to other people.

/*El programa calcula la ecuación general de segundo grado,
no tiene mayor proposito más que practicar C y hacer trampa en los exámenes -w-,
trato de usar funciones claras para hacer más legible el código, tómese nota de que me gusta 
main en primer lugar main y luego el resto de funciones.*/
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
#include<stdio.h>
#include<stdlib.h>
#include<math.h>
int a,b,c,total,resultado; //total se usa para el resultado de diferentes opercaciones de la ecuación, resultado es el resultado final de la ecuación
void value_input();
int discriminante(int b, int a, int c);
void display();
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
void main(){
        value_input();
        b=-b; //como todas las operaciones con "b" son su multiplicación *-1 y su cuadrado, decidí solo volver b negativo (o positivo dependiendo del caso)
        discriminante(b,a,c);
        
        display();
}
//-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
//value_input solo le pregunta al usuario los valores de a, b y c en la ecuación y los aloca a las variables correspondientes.
void value_input(){
        printf("Dame el valor de a: ");
        scanf("%d", &a);
        printf("Dame el valor de b: ");
        scanf("%d", &b);
        printf("Dame el valor de c: ");
        scanf("%d", &c);
}
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
//La función calcula la discriminante y devuelve su valor
int discriminante(int b, int a, int c){
        int result;
        b*=b;
        c=4*a*c;
        total=b-c;
        return total;
} 
//----------------------------------------------------------------------------------------------------------------------------------------------------------------------
//display calcula el divisor de la ecuación y determina como imprimir la ecuación basado en el discriminante
void display(){
        a*=2; //se calcula el divisor
        //se calcula valor del discriminante y dependiendo de éste se imprime el resultado
        if(total>0){    //es mayor que 0
                total=sqrt(total);
                printf("El resultado es: (%d±%d)/%d\n",b,total,a);
                }
        else if(total<0){       //es menor que 0
                total=abs(total);
                total=sqrt(total);
                printf("El resultado es: (%d±%di)/%d\n",b,total,a);
                }
        else if(total=0){       //es igual a 0
                printf("El resultado es: (%d\n",b/a);
                }
}

I think that it is mostly understandable for anyone that wants to review the code in general, but I think that it can become a little convoluted in some places, specially the display() function.

How could I make it clearer?

\$\endgroup\$
6
  • 1
    \$\begingroup\$ @IñakiUlibarri Please edit your question to include only your code file's content, not the visuals of your editor. In case you have only nano at hand, use M-# to disable the line numbering temporarily. \$\endgroup\$ – Zeta Apr 22 at 13:48
  • \$\begingroup\$ I see you've edited the question title, but it still only mentions your concerns about the code - here on Code Review, the title should describe what real-work task your program solves. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. I've edited with what I think the code does. \$\endgroup\$ – Toby Speight Apr 22 at 15:41
  • 1
    \$\begingroup\$ @IñakiUlibarri b*=b; c=4*a*c; both can overflow int math. Is that a concern for you? are you just making this code to handle small integer values? \$\endgroup\$ – chux - Reinstate Monica Apr 22 at 19:21
  • \$\begingroup\$ @chux, it's even worse - see the integer division right at the end! \$\endgroup\$ – Toby Speight Apr 23 at 6:51
  • \$\begingroup\$ @chux-ReinstateMonica Yes, I made this to practice my C and cheat my way through my Highschool maths class, you'll imagine that I rarely input negative values into it, let alone fractions and other whatnots. You'll also see that it's amateurish and generally sucky all around, that is because this is literally my first ever piece of functional, functioning code :-), I came here because I've found my "For Dummies" book to be quite unresponsive when I asked for suggestions, so I figured that having good feedback from the start would be good to avoid bad habits, and this place seems good for that. \$\endgroup\$ – Iñaki Ulibarri Apr 23 at 16:17
6
\$\begingroup\$

I would find it easier to understand if the comments were in English, but I'm not the one who will be making changes later, so write your comments for that person (whatever is most useful to future-you).

What I find actively harmful is the very long lines such as

//----------------------------------------------------------------------------------------------------------------------------------------------------------------------

and

        b=-b; //como todas las operaciones con "b" son su multiplicación *-1 y su cuadrado, decidí solo volver b negativo (o positivo dependiendo del caso)

For the first, I think an empty line would be better; for the second, consider word-wrapping it as a block comment.

The global variables a, b, c, total and resultado make it harder to reason about each function in isolation. I can't even see where that last one is actually used. Prefer to pass values to and from functions rather than communicating via globals.

These function declarations would be better as prototypes (i.e. specify that they take no arguments, rather than unspecified ones):

void value_input(void);
void display(void);

I'd declare all the helpers with static linkage (this becomes more important when you write programs with multiple source files, but it's a good habit to form), and I'd accept the arguments to discriminante() in the less surprising order a, b, c.

void main() is incorrect - the main function must return an int, so we need int main(void). Your compiler should have warned you about this; make sure you're enabling enough warning options (e.g. gcc -Wall -Wextra).

When we read input using scanf(), it's important to use the return value that indicates how many conversions were successful. I would write a function to get a single value (and accept non-integer coefficients, since that's what we can expect from real equations - pun intended):

double read_double(const char *prompt)
{
    for (;;) {
        printf("%s: ", prompt);
        double x;
        int conversions = scanf("%lf", &x);
        if (conversions == 1) {
            return x;
        }
        if (conversions == EOF) {
            /* not much we can do now */
            fputs("Reading failed\n", stderr);
            exit(1);
        }
        /* consume the invalid input and start again */
        puts("Input should be a valid number");
        scanf("%*[%\n]");
    }
}

discriminante() declares total which it never uses, and returns a value that gets ignored. There's clearly something wrong there.

This line doesn't do what you think it does:

   else if(total=0){

Thankfully, it's harmless, since if total>0 and total<0 are both false, we know that total is already zero, and the assignment has no effect.

When we know a number n is negative, we can make it positive simply with -n rather than calling abs().


Modified code

Here's what I ended up with after fixing the above:

/* El programa calcula la ecuación general de segundo grado, no tiene
   mayor proposito más que practicar C y hacer trampa en los exámenes
   -w-, trato de usar funciones claras para hacer más legible el
   código, tómese nota de que me gusta main en primer lugar main y
   luego el resto de funciones.
*/

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

static double read_double(const char *prompt);
static double discriminante(double a, double b, double c);
static void display(double a, double b, double d);

int main(void)
{
    const double a = read_double("Dame el valor de a");
    const double b = read_double("Dame el valor de b");
    const double c = read_double("Dame el valor de c");

    const double d = discriminante(a, b, c);

    display(a, b, d);
}

static double read_double(const char *prompt)
{
    for (;;) {
        printf("%s: ", prompt);
        double x;
        int conversions = scanf("%lf", &x);
        if (conversions == 1) {
            return x;
        }
        if (conversions == EOF) {
            /* not much we can do now */
            fputs("Reading failed\n", stderr);
            exit(1);
        }
        /* consume the invalid input and start again */
        puts("Input should be a valid number");
        scanf("%*[%\n]");
    }
}

// La función calcula la discriminante y devuelve su valor
static double discriminante(double a, double b, double c)
{
    return b * b - 4 * a * c;
}

// display calcula el divisor de la ecuación y determina como imprimir
// la ecuación basado en el discriminante
static void display(double a, double b, double d)
{
    printf("El resultado es: ");
    if (d > 0) {
        printf("(%g±%g)/%g\n", -b, sqrt(d), a);
    } else if (d < 0) {
        printf("(%g±%gi)/%g\n", -b, sqrt(-d), a);
    } else {                /* total == 0 */
        printf("%g/%g\n", -b, a);
    }
}
\$\endgroup\$
11
  • 1
    \$\begingroup\$ Yes. I was going to say something about that function being hard to do well, and I inadvertently proved it! Now improved. \$\endgroup\$ – Toby Speight Apr 22 at 19:00
  • \$\begingroup\$ From what I understand, scanf exhibits UB when the input isn't representable with the given type, so this still doesn't really work. C99, 7.19.6.2, p10: [The] input item [...] is converted to a type appropriate to the conversion specifier. [The] result of the conversion is placed in the object pointed to by the first argument following the format argument that has not already received a conversion result. If this object does not have an appropriate type, or if the result of the conversion cannot be represented in the object, the behavior is undefined. \$\endgroup\$ – JCC Apr 22 at 19:57
  • \$\begingroup\$ @JCC, what would you recommend instead? \$\endgroup\$ – Toby Speight Apr 23 at 6:49
  • 1
    \$\begingroup\$ The only way to do this correctly that I'm aware of is to read the entire line, and then parse it using strto*, which can indicate an error if one appears. See ramblings.implicit.net/c/2014/05/04/…. \$\endgroup\$ – JCC Apr 23 at 9:29
  • 1
    \$\begingroup\$ Ah, yes, that's good. That said, "reading an entire line" is harder than it sounds, in the general case. Thankfully, we can get away with reading, say, up to 40 chars of a line, and if the line is longer and fails conversion, then perform additional reading to discard the rest of the line. \$\endgroup\$ – Toby Speight Apr 23 at 9:41
3
\$\begingroup\$

Readability and (light) maintainability

Readability in code is highly subjective, so take everything in this section with a grain of salt.

Line lengths

Most styles used in the wild will explicitly limit the length of the lines to less than 100 characters if possible. Whether it's 72, 80, 96 or another random value isn't important, but >140 is too much.

Language

Unless you're the only developer working on the software, you should use English identifiers and documentation. But even if you're the only developer working on a piece of code at the moment, you might not be the only dev working on it forever. It's easier to start in an international format first than to change everything later.

Giving space to breathe

Have a look at the following two code snippets:

#include<stdio.h>
#include<stdlib.h>
#include<math.h>
int a,b,c,total,result; //total is used for the result of different operations of the equation, result is the final result of the equation.
void value_input();
#include <stdio.h>
#include <stdlib.h>
#include <math.h>

int a, b, c;
int total;  // total is used for the result of different operations of the equation, 
int result; // result is the final result of the equation.

void value_input();

Have a look at both variants. Both contain the same code. Which one would you prefer to read in a few weeks after you forgot about this snippet? Which one is easier to overview?

Space is important. While it doesn't matter to your compiler, it matters to you, the human. Patterns get a lot easier to recognize if they're isolated from each other, and space provides the necessary isolation.

Also, we were able to cut down the line length by a lot by splitting total and result into their own lines. There's no scrollbar anymore, and we can see the full comments without any hassle.

Spacing also makes it easier to recognize a bug in the current code:

void display(){
        // <snip>
        else if(total=0){       //es igual a 0
                printf("El resultado es: (%d\n",b/a);
                }
}

Do you notice the bug? How about now:

void display(){
        // <snip>
        else if(total = 0){       //es igual a 0
                printf("El resultado es: (%d\n",b/a);
                }
}

That's an assignment, not an equality test.

Note that I would use a single line per declaration, but before that, let's talk about the proper place about those declarations.

Global variables and hard maintainability

You probably wondered about the "(light)" part in the previous section. While those changes in your formatting and spacing make it easier to read your code, global variables handicap reasoning about code.

In order to know how a, b, c, total and result change, we need to inspect all functions, because any function from the same file might change it. There might even be a typo somewhere, e.g.

//          whoops:                 v 
int discriminante(int b, int a, int d) {
        int result;
        b *= b;
        c = 4 * a * c;                   // oh no, global c changed!
        total = b - c;
        return total;
}

Suddenly, we're changing the global c instead of the local one, because we accidentally called our argument d. Usually, a compiler with proper configuration will throw a warning at our face at that point, but that might not be the default configuration. Also, the unused result variable should yield a warning, but that's another issue.

\$\endgroup\$

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.