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.

• William Morris: What is the advantage of making minus an int8_t? I see no reason for this; int is fine.
• syb0rg: @WilliamMorris Memory conservation mainly.
• William Morris: That is premature optimization that doesn't optimize. It depends very much on the processor. Most likely the code will be an instruction bigger each time minus is used or passed to a function. I always prefer using the natural processor width (i.e. int) unless there is a good reason not to. There are often good reasons, but here there is none.
• syb0rg: @WilliamMorris The other reason is for self-documenting code. By using int8_t, I now know the exact number range that the variable is supposed to store, and that they can be both negative and positive. With only using int, I can somewhat know the number range, but the intent of the variable is a tidbit more obscured (albeit not by much).
• William Morris: to make it self-documenting I might make minus a bool and call it negative. Then I'd do an if (negative) {num = -num;} at the appropriate points. I most certainly wouldn't give it spurious precision of an int8_t. I think we'll have to agree to disagree.

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.