Simple calculator

Question

I created my first program in C: a simple calculator. I want to know if it is possible to make this code more effective. Is it, for example, possible to change the calculator function so that I don't have to use switch, but the code knowing what to do because of the character that is entered?

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

char input[20];
char num[20];
int i;
int minus = 1;
float answer, num1, num2;
char operations;

void extractNumbers();
void calculate();

int main()
{
    gets(input);
    extractNumbers();
    calculate();
    printf("Answer: %f", num1);
    return 0;
}

void extractNumbers(){
    for(i = 0; i < 20; i++){
        if(isdigit(input[i]) || input[i] == '.'){
            strncat(num, &input[i], 1);
        }
        if((input[i] == '-' && i == 0) || (input[i] == '-' && !isdigit(input[i-1]))){
            minus = -1;
            continue;
        }
        if(input[i] == '+' || input[i] == '-' || input[i] == '*' || input[i] == '/' || input[i] == '^'){
            num1 = atof(num) * minus;
            minus = 1;
            strcpy(num, "");
            operations = input[i];
        }
    }
}

void calculate(){
    num2 = atof(num) * minus;
        switch(operations){
            case '+':
                num1 += num2;
                break;
            case '-':
                num1 -= num2;
                break;
            case '*':
                num1 *= num2;
                break;
            case '/':
                num1 /= num2;
                break;
    }
}

Answer 1

A few notes:

• You see those global variables that you have at the top?

  char input[20];
  char num[20];
  int i;
  int minus = 1;
  float answer, num1, num2;
  char operations;
  

  Those shouldn't be there. Those should be declared within functions. If another function required the value of one of those variables, pass it as a parameter. If you need to store a value from one function in another, return the value and store it in a variable.

• You have the magic number 20 in your code.

• Never ever ever EVER use gets(). It is a dangerous function that is not to be used. Use fgets() instead, or gets_s() from C11.

• Declare your counter variable within your for loops.

  for(int i = 0; i < 20; ++i)
  

• You use some functions from the ctype.h library, but do not #include it.

• Always declare what parameters your function takes in, even if nothing.

  int main(void)
  

  You might wonder why we have to do this. Imagine we have the function foo() declared as such:

  int foo()
  

  In C, this is known as an identifier list and means that it "can take any number of parameters of unknown types". We can actually pass values to the function even though we don't mean to or intend to. If the caller calls the function giving it some argument, the behavior is undefined. The stack could become corrupted for example, because the called function expects a different layout when it gains control.

  Using identifier lists in function parameters is depreciated. It is much better to do something like:

  int foo(void)
  

  In C, this is known as a parameter type list and defines that the function takes zero arguments (and also communicates that when reading it) - like with all cases where the function is declared using a parameter type list, which is called a prototype. If the caller calls the function and gives it some argument, that is an error and the compiler spits out an appropriate error.

  The second way of declaring a function has plenty of benefits. One of course is that amount and types of parameters are checked. Another difference is that because the compiler knows the parameter types, it can apply implicit conversions of the arguments to the type of the parameters. If no parameter type list is present, that can't be done, and arguments are converted to promoted types (that is called the default argument promotion). char will become int, for example, while float will become double.

• Use %g when printing float values. It also allows you to print double if you decide to "upgrade" the type later.

• You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard knows how frequently this is used, and lets you not bother.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a value of 0.

• minus should be declared as a int8_t from <stdint.h>, since you only use the values -1 and 1.

Answer 2

Not bad at all for the first program.

Typical mistakes

• Don't use gets. It is dangerous and deprecated. Use fgets instead.

• Globals are evil. Try to abstain from them. Learn how to pass parameters and return values.

Advanced mistake

extractNumbers doesn't do exactly what it claims. Instead it parses the input string, so calling it parse() seems more appropriate. Also I'd recommend to structure it a bit differently, to emphasize a correct structure of input.

Your calculator accepts strings in form of number, opcode, number, so the parsing procedure should do exactly that

In pseudocode,

parse_input(string)
    num1 = parse_number(string)
    opcode = parse_opcode(string)
    num2 = parse_number(string)
    return (num1, opcode, num2)

Of course, all parsing routines need to adjust the string for characters it consumed, they should be able to report problems, etc. For parsing numbers, C standard library provides strtol family of functions. Returning multiple values shall also be addressed.