7
\$\begingroup\$

I'm trying to make a small parabola calculator just for fun. Is this code good?

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

#define _X_UNTIL_Y(q,w)          \
    while (argv[1][q] != w) {    \
        q++;                     \
    }

#define _ATOI(s,d,f,g)           \
    strncpy(s, argv[1]+f, g);    \
    d = atoi(s)

int main(int argc, char const *argv[]) {
    int i=0, j, l;
    float D, a=1, b=1, c=1, r, k;
    char A[5], B[5], C[5], d;

    if(argv[1][0] == 'x');
    else{
        _X_UNTIL_Y(i,'x');
        _ATOI(A,a,0,i);
    }
    if(argv[1][0] == '-' && argv[1][1] == 'x') a=-1;

    j = i += 3;
    _X_UNTIL_Y(j,'x');
    if(argv[1][j-1] == '+' || argv[1][j-1] == '-');
    else{
        _ATOI(B,b,i,j);
    }
    if(argv[1][j-1] == '-') b=-b;

    l = j += 1;
    _X_UNTIL_Y(l,'=');
    _ATOI(C,c,j,l);

    D = pow(b,2)-4*a*c;

    if(D < 0){
        d = 'n';
        printf("Parabola doesn't have real roots.\n");
    }else if(D == 0){
        d = 's';
        printf("Parabola has one real root.\n");
    }else if(D > 0){
        d = 'p';
        printf("Parabola has two real roots.\n");
    }else
        return -1;

    r = -(b/(2*a));
    k = -(D/(4*a));

    a < 0 ? printf("Parabola look downwards.\n") : printf("Parabola look upwards.\n");
    printf("Vertex: %.1f,%.1f\n", r, k);
    printf("Axis of symmtery: %.1f\n", r);
    printf("y-intercept: %.1f\n", c);

    if(d=='n');
    else if(d=='s')
        printf("x-intercept: %.1f\n", r);
    else if(d=='p'){
        float x1, x2;
        x1=(-b + sqrt(D)) / (2*a);
        x2=(-b - sqrt(D)) / (2*a);
        printf("x-intercepts: %.1f, %.1f\n", x1, x2);
    }
    return 0;
}
\$\endgroup\$
10
\$\begingroup\$

I see a number of things that may help you improve your code.

Avoid macros

The macros _X_UNTIL_Y and _ATOI do not help with readability. I'd advise not using macros like those, preferring functions instead.

Use more descriptive variable names

All of the single-letter variable names make it difficult to decipher the algorithm. More meaningful variable names make the code easier to read and maintain.

Break up the code into smaller functions

Rather than having everything in one long main function, it would be easier to read and maintain if each discrete step were its own function.

Think of the user

If the user runs the code with no arguments, he or she is rewarded with a segfault and crash. It would be much nicer instead to prompt the user with a string describing how the program is intended to be used instead.

Use idiomatic C

Lines like these:

if(argv[1][0] == 'x');
else{

are quite odd. It would make it much easier to read and understand if it were written instead like this:

if(argv[1][0] != 'x') {

Add comments

Code like this is difficult enough to read for the reasons listed above. Just a few well-placed comments about why the code is doing what it's doing would greatly aid readers of the code to understand your intent.

Eliminate return 0 at the end of main

Since C99, the compiler automatically generates the code corresponding to return 0 at the end of main so there is no need to explicitly write it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for suggestions! I've done everything you said, except variable names. I changed "D" and "d" into "discriminant" and "sign", but "a", "b", "c", "r" and "k" is used with these equations, so it makes more sense like that. i (and following letters) are used everywhere with counters. \$\endgroup\$ – betseg Apr 30 '16 at 17:46
  • 3
    \$\begingroup\$ @betseg you shouldn't edit the original code in the question. \$\endgroup\$ – Joseph Farah Apr 30 '16 at 19:39
3
\$\begingroup\$

Identifier names with leading underscores

This is particularly arcane, but your function-like macro names _X_UNTIL_Y and _ATOI should not be used because they conflict with identifier patterns that are reserved. Specifically, any identifier beginning with an underscore followed by a capital letter are always reserved, no matter what, in what's called the "compiler's namespace". From the C FAQ, Question 1.29 (How can I determine which identifiers are safe for me to use and which are reserved?)

What do the above rules really mean? If you want to be on the safe side:

1,2. Don't give anything a name with a leading underscore.

  1. Don't give anything a name which is already a standard macro (including the "future directions" patterns).
  2. Don't give any functions or global variables names which are already taken by functions or variables in the standard library, or which match any of the "future directions" patterns. (Strictly speaking, "matching" means matching in the first six characters, without regard to case; see question 11.27.)
  3. Don't redefine standard typedef or tag names.

Mixing multiple declarations and initialization in a single line

You have,

int i=0, j, l;
float D, a=1, b=1, c=1, r, k;

Many coding standards prefer only one declaration per line. I tend to agree with that, but personally I am lax on that rule if I am declaring two trivial (i.e., simple loop counter) or interrelated variables on the same line.

But you have mixed multiple declarations where only some of the variables are initialized at declaration (such as int i=0) with uninitialized variables on the same line.

Furthermore, along with @Edward's suggestion re: descriptive variable names, you use i, j, and l as integers, but the k in the sequence is a float. Why? It is often idiomatic to use single letter variable names (especially i, j, etc) as simple loop counters when it is absolutely obvious they are trivial loop indices. But your use of them is non-idiomatic.

Consistent whitespace around operators and keywords

Get into the habit of being consistent and liberal with space characters separating binary operators, keywords, and following commas. For instance, your calls to printf(), such as printf("Vertex: %.1f,%.1f\n", r, k);, are nicely spaced.

But the statement D = pow(b,2)-4*a*c; should be cleaned up a bit: D = pow(b, 2) - 4 * a * c;.

Similarly, do not squeeze parentheses or braces next to keywords; put a space between if/while/do/for and the condition / loop parameters.

  • Do: if (D < 0) {
  • Don't: if(D < 0){

And

  • Do: } else if (D == 0) {
  • Don't: }else if(D == 0){

Don't abuse the ternary operator (?:)

You have abused the ternary operator in this line:

a < 0 ? printf("Parabola look downwards.\n") : printf("Parabola look upwards.\n");

There is no need to economize characters in your source code; The following is much more readable:

if (a < 0) {
    printf("Parabola look downwards.\n");
} else {
    printf("Parabola look upwards.\n");
}

The benefit of the ternary operator is not for control flow (such as do "x" if condition, otherwise do "y"). Indeed, as a control flow operator, it is quite limited (you can't perform multiple actions in either branch). Rather, its purpose is to conditionally produce one l-value or another. For instance, one way to achieve what you were trying to conditionally print was with the following:

char* s;
if (a < 0) {
    s = "downwards";
} else {
    s = "upwards";
}
printf("Parabola looks %s.\n", s);

Two correct, idiomatic uses of the ternary operator would be either of:

printf("Parabola looks %s.\n", a < 0 ? "downwards" : "upwards");

or

char* s = a < 0 ? "downwards" : "upwards";
printf("Parabola looks %s.\n", s);

The choice of which of those two examples to use comes down to readability and personal preference.

Comparing floats for equality

You compare a float D directly to zero: } else if (D == 0) {. Don't do that. Unfortunately, I can't give a completely accurate and succinct answer here that will work for all floating point comparisons of small differences or comparisons around zero. See the following resources to understand the issues, and suggested general approaches:

\$\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.