7
\$\begingroup\$

I finished this code a few minutes ago. It started off as an explanation of a basic calculator in C to a friend. Then we decided to take it further and I made this. Please tell me where I can make changes, no matter how small, you think needs to be changed/made better. Also I'm new to the C language, I've self studied it for around 4-5 months.

I did not get any errors and it works exactly like I expected it to. I am just looking for help improving myself and the way I code. Also this is my biggest code in C yet.

And please feel free to share your own similar calculators or creations.

Code:

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

#define pi 3.14159265359

float add(float a, float b);
float sub(float a, float b);
float mult(float a, float b);
float divide(float a, float b);
float sqroot(float a);
float power(float a, float b);
float sine(float a);
float cosine(float a);
float tangent(float a);
float logarithm(float a);


int main() {

    float a, b, val;
    int op;
    char choice;
    
    printf("to choose an operation, enter the number of the operation 1)+  2)-  3)*  4)/  5)power  6)sqrt  7)sin  8)cos  9)tan  10)log(base 10) : ");
    scanf("%d", &op);

    if (op > 0 && op < 6) {
        printf("enter first number: ");
        scanf("%f", &a);

        printf("enter second number: ");
        scanf("%f", &b);

    }
    else if (op > 5 && op < 11) {
        printf("enter a number: ");
        scanf("%f", &a);

    }

    switch(op) {
        case 1: val = add(a, b);      break;
        case 2: val = sub(a, b);      break;
        case 3: val = mult(a, b);     break;
        case 4: val = divide(a, b);   break;
        case 5: val = power(a, b);    break;
        case 6: val = sqroot(a);      break;
        case 7: val = sine(a);        break;
        case 8: val = cosine(a);      break;
        case 9: val = tangent(a);     break;
        case 10: val = logarithm(a);  break;

        default: printf("invalid operator..."); break;

    }

    if (op > 0 && op < 11) {
        printf("answer is: %f", val);
        
        getchar();

        printf("\ncontinue? (y/n) ");
        scanf("%c", &choice);
        
        if (choice == 'y') {
            main();
        }
        else if (choice == 'n') {
            printf("ok cool bye");            
        }

    }

    return 0;
}

float add(float a, float b) {
    return a + b;
}

float sub(float a, float b) {
    return a - b;
}

float mult(float a, float b) {
    return a * b;
}

float divide(float a, float b) {
    return a / b;
}

float sqroot(float a) {
    return sqrt(a);
}

float power(float a, float b) {
    return pow(a, b);
}

float sine(float a) {
    return sin(a * pi / 180);
}

float cosine(float a) {
    return cos(a * pi / 180);
}

float tangent(float a) {
    return tan(a * pi / 180);
}

float logarithm(float a) {
    return log10(a);
}





/*if (op == '+') {
        printf("\n %f + %f = %f", a, b, a + b);
    }
    else if (op == '-') {
        printf("\na - b = %f", a - b);
    }
    else if (op == '-' && a < b) {
        printf("\nb - a = %f", b - a);
    }
    else if (op == '*') {
        printf("\na * b = %f", a * b);
    }
    else if (op == '/') {
        printf("\na / b = %f", a / b);
    }*/
\$\endgroup\$

1 Answer 1

8
\$\begingroup\$

Imprecise pi

Use at least DBL_DECIMAL_DIG significant digits. DBL_DECIMAL_DIG is commonly 17.

Check input return values

Detect input success

// scanf("%f", &a);
if (scanf("%f", &a) != 1) {
  Handle_bad_input();  // TBD code
}

Minor: Use correct sentence casing

// printf("enter first number: ");
printf("Enter first number: ");

Avoid naked magic numbers

Document constants like 1,5,6... as to what they represent.

// if (op > 0 && op < 6) {
#define OP2_MIN 1
#define OP2_MAX 5
if (op >= OP2_MIN && op < OP2_MAX) {

Even better, derive these values from the length of the function array (see next).

Rather than a switch, consider a table look up

if (op >= OP2_MIN && op < OP2_MAX) {
  float (*ops2)() = { add,  sub, ... , power}; 
  float f() = ops2[op - OP2_MIN];
  val = f(a, b);
  ...

Simplify

// getchar();
printf("\ncontinue? (y/n) ");
// scanf("%c", &choice);
scanf(" %c", &choice);  // Space added to consume prior white-space.

Avoid re-entrant main()

Re-entrant main() is often considered an undesirable coding style. Re-work code.

If staying with recursion:
if (choice == 'y') { main(); should have been if (choice == 'y') { return main();

Improve degree arguments

Since code is using degrees, reduce the range before calling. This will improve the results of large degree arguments as pi only approximates the irrational π.
More details: Sin and Cos give unexpected results for well-known angles.

a = fmodf(a, 360); // add this reduction.
sin(a * pi / 180);

float or double

Code is using float arguments with double functions.

Use sqrtf, powf, ... with float arguments or ...
even better, use double a, b, ...

More common to end output with a line-feed

// printf("answer is: %f", val);
printf("answer is: %f\n", val);

It is floating point

printf("answer is: %f", val); prints with a fixed precision format - worthless for tiny results and noisy for large ones.

Use %g to print general floating point output.

// printf("answer is: %f", val);
printf("Answer is: %g", val);

If you go with double variables (recommended), use:

printf("Answer is: %.*g", DBL_DIG, val);

Detect common issues

Consider detecting and printing a warning with common problems.

  • Division by zero.

  • Negative sqrt(), logarithm() calls.


Following is some code tossed together to illustrate some of the above ideas and a few more (structure driven function/key, re-organized flow, a zero-arg function call).

This illustrative code compiles, but is not tested. If I reviewed it like OP's code, I likely find 10+ issues with it, but without spending too much more time, I wanted to present OP with some real alternative code.

#include <errno.h>
#include <float.h>
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>

#define DEGREES_IN_A_CIRCLE 360.0
// pi/180 --> 0.01745...
#define DEGREES_TO_RADIANS(d) ((d)*0.01745329251994329576923690768489)

double add(double a, double b) {
  return a + b;
}

double sub(double a, double b) {
  return a - b;
}

double mult(double a, double b) {
  return a * b;
}

double divide(double a, double b) {
  return a / b;
}

double sqroot(double a) {
  return sqrt(a);
}

double power(double a, double b) {
  return pow(a, b);
}

double sine_d(double a_degree) {
  a_degree = fmod(a_degree, DEGREES_IN_A_CIRCLE);
  return sin(DEGREES_TO_RADIANS(a_degree));
}

double cosine_d(double a_degree) {
  a_degree = fmod(a_degree, DEGREES_IN_A_CIRCLE);
  return cos(DEGREES_TO_RADIANS(a_degree));
}

double tangent_d(double a_degree) {
  a_degree = fmod(a_degree, DEGREES_IN_A_CIRCLE);
  return tan(DEGREES_TO_RADIANS(a_degree));
}

double logarithm10(double a) {
  return log10(a);
}

double now(void) {
  return (double) time(0);
}

typedef struct {
  union {
    double (*f0)(void);
    double (*f1)(double a);
    double (*f2)(double a, double b);
  } f;
  unsigned n;
  const char *token;
} func;

// Maximum argument count
#define ARG_N 2

const func operator[] = { //
    { {0}, 0, "exit"}, //
        { .f.f2 = add, 2, "add"}, //
        { .f.f2 = sub, 2, "sub"}, //
        { .f.f2 = mult, 2, "mul"}, //
        { .f.f2 = divide, 2, "div"}, //
        { .f.f1 = sqroot, 1, "sqrt"}, //
        { .f.f2 = power, 2, "pow"}, //
        { .f.f1 = sine_d, 1, "sin"}, //
        { .f.f1 = cosine_d, 1, "cos"}, //
        { .f.f1 = tangent_d, 1, "tan"}, //
        { .f.f1 = logarithm10, 1, "log10"}, //
        { .f.f0 = now, 0, "now"}, //
    };

int operator_n = sizeof operator / sizeof operator[0];

int main(void) {
  for (;;) {
    printf("Choose an operation");
    for (int i = 0; i < operator_n; i++) {
      printf(", %s", operator[i].token);
    }
    printf(": ");
    char buf[100];
    if (fgets(buf, sizeof buf, stdin) == NULL) {
      break;
    }
    buf[strcspn(buf, "\n")] = '\0'; // Lop off potential trailing \n

    int op = operator_n;
    while (op-- > 0) {
      if (strcmp(buf, operator[op].token) == 0) {
        break;
      }
    }

    if (op < 0) {
      printf("Operation not found\n");
      continue;
    }
    if (op == 0) {
      break;
    }

    double arg[ARG_N];
    unsigned a;
    for (a = 0; a < operator[op].n && a < ARG_N; a++) {
      printf("Enter a number: ");
      if (fgets(buf, sizeof buf, stdin) == NULL) {
        break;
      }
      char *endptr;
      errno = 0;
      arg[a] = strtod(buf, &endptr);
      if (buf == endptr || errno || endptr[0] != '\n') {
        printf("Bad input\n");
        break;
      }
    }
    double val = NAN;
    if (a == operator[op].n) {
      switch (a) {
        case 0:
          val = operator[op].f.f0();
          break;
        case 1:
          val = operator[op].f.f1(arg[0]);
          break;
        case 2:
          val = operator[op].f.f2(arg[0], arg[1]);
          break;
      }
    }
    printf("answer is: %.*g", DBL_DIG, val);
  }
  return EXIT_SUCCESS;
}
```
\$\endgroup\$
14
  • 1
    \$\begingroup\$ Thanks a lot! Could you maybe explain the first half of your answer a little simpler? Or maybe provide a source from where I can understand what you're saying easier? Quite a lot of what you told me literally flew over my head. And why is it that scanf("%f ", &a); will give no visible output and wont print the printf statement before it but a space before %f works? I don't quite understand it. And the 3rd and 4th bold pointers don't make any sense to me :D. I'm gonna look into them myself too but I would love it if you can provide some additional help? Thank you so much again! \$\endgroup\$ Feb 24 at 2:57
  • 1
    \$\begingroup\$ @ThePowerHex_YT Let us start with scanf("%f ", &a);. That obliges user to type a number, some 0 or more spaces and then something else. Drop the following " " so scanf() will return once some non-numeric input follows the number. The "space before" is a red herring. It is the "space after" that cause you woe. \$\endgroup\$ Feb 24 at 4:44
  • 1
    \$\begingroup\$ @ThePowerHex_YT What is unclear about "// printf("enter first number: "); printf("Enter first number: ");"? \$\endgroup\$ Feb 24 at 4:47
  • 1
    \$\begingroup\$ @ThePowerHex_YT What is unclear about "Avoid naked magic numbers"? Code that uses if (op > 0 && op < 6) lacks clarity. Why 6? what is so special about 6? Code should convey a constant as a phrase and not a bare 6 to establish some meaning. \$\endgroup\$ Feb 24 at 4:51
  • 1
    \$\begingroup\$ @ThePowerHex_YT What do you think a " " after "%f" does? Hint: it refers to 0 or more white-spaces. \$\endgroup\$ Feb 24 at 5:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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