2
\$\begingroup\$

Previous question

After applying the suggestions, I wanted to revise the question with the new code because I don't think this is good enough.

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

/* global loop counters */
int i=0, j, k;

/* to find a, b, and c from the input */
float A(const char* in){
    char c[5];
    if(in[0] != 'x'){ /* if first letter isn't x
                      ** eg there is sth in front of x */
        while(in[i] != 'x'){ /* until x in found */
            i++; /* increment i */
        }
        strncpy(c, in, i); /* copy first i characters of in to c */
        return atoi(c); /* make c an int and return */
    }

    if(in[0] == '-' && in[1] == 'x') /* if first two letters are "x" */
        return -1; /* -x = -1 * x */

    return 1; /* if nothing is in front of x, a is 1 */
}

float B(const char* in){
    char c[5];
    int b=1;

    j = i += 3; /* three characters after x (^2+ or ^2-) */
    while(in[j] != 'x') { /* until x is found */
        j++; /*increment j */
    }

    if(in[j-1] != '+' && in[j-1] != '-'){ /* if not -x or +x
                                          ** eg if there is b */
        strncpy(c, in+i, j); /* copy j characters starting from i */
        b = atoi(c); /* make c an int and assign this to b */
    }

    if(in[j-1] == '-') /* if there is a minus sign */
        b = -b; /* negate b */

    return b; /* return b */
}

float C(const char* in){
    char c[5];
    k = j += 1; /* one character after x (+ or -) */
    while(in[k] != '=') { /* until we find a = */
        k++; /* increment k */
    }
    strncpy(c, in+j, k); /* copy k characters starting from j */
    return atoi(c); /* make c an int and return */
}

int main(int argc, char const *argv[]) {
    if(argc == 1){ //if no arguments given
        printf("Usage: %s <quadratic equation>\n", argv[0]);
        return EXIT_FAILURE;
    }

    /* variables for main function */
    float discriminant;
    float a, b, c; /* coefficients */
    float vx, vy; /* vertex */
    char sign; /*positive or negative or zero */

    a=A(argv[1]);
    b=B(argv[1]);
    c=C(argv[1]);

    /*                   b^2 - 4ac     */
    discriminant = pow(b, 2) - 4 * a * c;

    if(discriminant < 0) {
        sign = 'n';
        printf("Parabola doesn't have real roots.\n");
    } else if(discriminant == 0.00f) {
        sign = 'z';
        printf("Parabola has one real root.\n");
    } else if(discriminant > 0) {
        sign = 'p';
        printf("Parabola has two real roots.\n");
    }

    /* vertex points */
    vx = -( b / (2*a) );
    vy = -( discriminant / (4*a) );

    printf("Parabola looks %s\n", a < 0 ? "downwards" : "upwards");
    printf("Vertex: V(%.2f,%.2f)\n", vx, vy);
    printf("Axis of symmetry: %.2f\n", vx);
    printf("y-intercept: %.2f\n", c);

    if(sign=='n'); /* if negative, doesn't have roots */
    else if(sign=='z') /* if zero, root is vertex' abcsissa is root */
        printf("x-intercept: %.2f\n", vx);
    else if(sign=='p'){ /*if positive, roots can be calculated... */
        float x1, x2;
        x1=(-b + sqrt(discriminant)) / (2*a);
        x2=(-b - sqrt(discriminant)) / (2*a);
        printf("x-intercepts: %.2f and %.2f\n", x1, x2);
    }
}
\$\endgroup\$
3
\$\begingroup\$
  1. General: Do not use int for float values.

  2. Either use float and float functions like sqrtf() or use double.

  3. When fabs(b) is close to sqrt(discriminant), OP's approach suffers severe loss of precision due to the + or -. More numerically stable code posted. Still needs some divide by 0.0 checks.

    //x1=(-b + sqrt(discriminant)) / (2*a);
    //x2=(-b - sqrt(discriminant)) / (2*a);
    float s = sqrtf(discriminant);
    if (b < 0) {
      x1 = (-b + s)/(2*a);
      x2 = c/x1;
    } else if (b > 0) {
      x2 = (-b - s)/(2*a);
      x1 = c/x2;
    } else {
      x1 = s/(2*a);
      x2 = -x1;
    }
    
  4. pow(b, 2) - 4 * a * c; also suffers from severe loss of precession when b*b is about 4*a*c. If OP is interested there are method to better calculate that too. It comes down to how quick you want to write/execute the code versus the need for correctness on edge conditions.

  5. Unclear format usage. About 45% of all float numbers will print as 0.00 with "%.2f". Suggest a more meaningful format

       // printf("Vertex: V(%.2f,%.2f)\n", vx, vy);
       printf("Vertex: V(%e,%e)\n", vx, vy);
    
  6. Dangerous code. Nothing stop code from incrementing into la-la land. If this A() and B() are only used in this controlled environment, make them static, else they should robustly handle all sorts of string inputs.

    float A(const char* in) {
      ...
      while(in[i] != 'x'){ /* until x in found */
        i++; /* increment i */
      }
    
  7. More weak code. Nothing insures that c will be a string after the strncpy call. Just use strtol() or strtof().

    strncpy(c, in, i); /* copy first i characters of in to c */
    return atoi(c);
    
  8. Unclear why code is using atoi(), yet returning float.

  9. Corner case: With b = atoi(c);, should c have the textual value of -INT_MIN, then assignment to b is UB.

  10. Consider printing something when sign=='n'

  11. Make a clean separation of parsing form quadratic calculation. (2 functions)

\$\endgroup\$
  • \$\begingroup\$ About #1 and #8, I just forgot that. About #11, I've done that (Sjoerd Job Postmus' answer). About #2, I began using doubles. \$\endgroup\$ – betseg May 4 '16 at 18:46
3
\$\begingroup\$

Self-documenting code

There's a concept in programming called self-documenting code (1, 2, 3), which means that the code itself is descriptive, uses meaningful function names (especially verb-subject -style naming), meaningful variable / state names, etc. It does not mean comments-per-line describing each step along the way. Nobody sees

    i++; /* increment i */

and is confused or misunderstands it. I'm not saying comments are bad; rather, comments, like code, are written once but read many times. The more you throw at your reader, the more you are asking them to keep "in their mental stack". Be respectful of your reader and be economical with their attention.

Rather than name your command line parsing functions A(), B(), and C(), all operating on the same input string, they can be replaced by a single well-named and well-documented function, starting with:

int parsePolynomialString(char *s, double *a, double *b, double *c) {
   /*
    * parsePolynomialString - parses a string representing
    *      a polynomial of order 2, in the following format:
    *                  ax^2+bx+c=0
    *      where a, b, c are the polynomial coefficients. Implicit coefficients
    *      (values of +/- 1) are allowed; the following are valid input strings:
    *                  -x^2-x-1=0
    *                  -1.618x^2+3.145x-3=0
    *
    * a, b, c      output parameters, containing the coefficients
    *              parsed from the input string.
    *
    * Returns:     The number of parsed parameters, or -1 if a parse error occurred
    */
    ....
}

Then you would call the parser in main():

int main(int argc, char* argv[]) {
   if (argc != 2) {
      // print error / usage info
      exit(EXIT_FAILURE);
   }
   double a, b, c;  // Polyomial coefficients: ax^2 + bx + c = 0
   int parseCount = parsePolynomialString(argv[1], &a, &b, &c);
   if (parseCount != 3) {
      // determine the error, print error info, and exit
      exit(EXIT_FAILURE):
   }

   // do the math stuff....
   exit(EXIT_SUCCESS);
 }

References:

  1. Self Documenting Code Vs. Commented Code
  2. What is self-documenting code and can it replace well documented code?
  3. Self-documenting code (Wikipedia)

Parsing the command line / Command line usage

I suggest you rework your command-line argument parsing. If the format of the command line arguments is up to you, I suggest you go with something like this:

Usage: parabola <a> <b> <c>    where <a>, <b>, and <c> are coefficients
           to a polynomial equation of the form: a*x^2 + b*x + c = 0

If you use this command line interface, you will require much less code to parse the command line, as well as conforming to more typical patterns of command line tools. The parsing would be something along the lines of:

#include <errno.h>  // in addition to the other headers you need

#define NUM_ARGS  3
int main(int argc, char* argv[]) {
   if (argc != NUM_ARGS + 1) {
      // print error/usage information
      exit(EXIT_FAILURE);
   }
   double coeffs[NUM_ARGS];
   char *arg = argv[1];
   for (int i = 0; i < NUM_ARGS; i++) {
      char *endptr;
      errno = 0;
      coeffs[i] = strtod(arg, &endptr);
      if (errno == ERANGE) {
         if (coeffs[i] == HUGE_VAL || coeffs[i] == -HUGE_VAL) {
            // input number too big for double. Print error info
         } else {
            // input number too small (underflow). Print error info
         }
         exit(EXIT_FAILURE);
      } else if (endptr == arg) {
         // no digits were found. Print error info
         exit(EXIT_FAILURE);
      }
      // If you got here, strtod() parsed a number. But...
      if (*endptr != '\0') {
         // non-parseable characters after the number.
         // If this is an error to you, then print error info & exit
      }
      ++arg;
   }
   ...

strtod() References:

  1. cppreference.com
  2. man strtod 3 - Linux Programmer's Manual
\$\endgroup\$
  • 1
    \$\begingroup\$ Very good discussion about betting coding and layout. p1. Note: "parses a null-terminated string" could be "parses a string" in C, all strings are null character terminated, else the character array is not a string. \$\endgroup\$ – chux May 4 '16 at 22:50
  • \$\begingroup\$ @chux: thank you! Agreed, and updated it to "parses a string". \$\endgroup\$ – scottbb May 5 '16 at 0:06
2
\$\begingroup\$

Just a quick overview:

  • global state: i, j and k are globals. Better not do that.
  • time-dependencies: Correctness requires A to be called before B and B before C, but that is not clear from the signature.
  • float/int confusion: A uses an int internally, and returns that value, but A is declared to return a float.
  • buffer overflows. What happens on parsing 9999999999999999999999999999999999999999999999999x^2 - 5x - 3?

Suggestions:

  • declare a struct "quadratic" with three fields a, b and c.
  • define a function parse_quadratic which parses the a/b/c from the string into a struct.
\$\endgroup\$
  • \$\begingroup\$ If I define a function, how can I get a, b and c? Global struct or something else? \$\endgroup\$ – betseg May 2 '16 at 13:40
  • 1
    \$\begingroup\$ You can just return the struct. Alternatively, just accept it as an out-parameter. Try not to use global variables at all. \$\endgroup\$ – Sjoerd Job Postmus May 2 '16 at 14:44
  • \$\begingroup\$ One last question: do you think I should use quadratic.a in equations or declare int/float a = quadratic.a? \$\endgroup\$ – betseg May 2 '16 at 15:16

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.