18
\$\begingroup\$

I made a very simple calculator in C that allows the user to perform calculations like addition, subtraction, multiplication and division. This is my first C program that I've made outside of my University course. It would be great to get some feedback on this program so that I can avoid bad practices early on in my coding.

//Calculator program
#include <stdio.h>
#include <stdlib.h>

int main()
{
    int menu; //Variable for the number the user inputs
    float num1, num2, result; //Float variables for the user input and output, used floats in case the user enters e.g. 14.7

    printf("Enter a number from the list below\n\n");

    printf("1. Addition\n"); //Calculator menu, user must enter a value from 1 - 4 for the program to work
    printf("2. Subtraction\n");
    printf("3. Multiplication\n");
    printf("4. Division\n\n");

    printf("Enter number: "); //User input for the calculator menu
    scanf("%d", &menu);

    printf("\n");

    switch(menu) //switch statement for menu
    {
    case 1:
        printf("You entered Addition\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for second number
        scanf("%f", &num2);

        printf("\n");

        result = num1 + num2; //Addition calculation

        printf("%.2f + %.2f = %.2f\n", num1, num2, result); //Addition output
        break;
    case 2:
        printf("You entered Subtraction\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for second number
        scanf("%f", &num2);

        printf("\n");

        result = num1 - num2; //Subtraction calculation

        printf("%.2f - %.2f = %.2f\n", num1, num2, result); //Subtraction output
        break;
    case 3:
        printf("You entered Multiplication\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for second number
        scanf("%f", &num2);

        printf("\n");

        result = num1 * num2; //Multiplication calculation

        printf("%.2f * %.2f = %.2f\n", num1, num2, result); //Multiplication Output
        break;
    case 4:
        printf("You entered Division\n\n");

        printf("Enter first number: "); //User input for first number
        scanf("%f", &num1);

        printf("Enter second number: "); //User input for first number
        scanf("%f", &num2);

        printf("\n");

        result = num1 / num2; //Division calculation

        printf("%.2f / %.2f = %.2f\n", num1, num2, result);
        break;
    default:
        printf("Enter correct number e.g 1 - 4\n"); //Outputted if the user enters a value other than 1 - 4
        break;
    }
    return 0;
}
//End of code
\$\endgroup\$
  • 3
    \$\begingroup\$ I'm not a fan of the comments. Some of them state things that are obviously the case, where no comment is needed. For example: switch(menu) //switch statement for menu or result = num1 + num2; //Addition calculation. And better naming of variables as well as moving some statements out to well-named functions would also remove the need for comments. \$\endgroup\$ – 404 May 26 '15 at 9:34
  • 1
    \$\begingroup\$ Follow-up question \$\endgroup\$ – 200_success May 27 '15 at 4:29
14
\$\begingroup\$

I must say, I respectively disagree with @ishyfishy's example of the addition function. Although, I do agree with the point about splitting the arithmetic functions away from the main function.

I believe that the arithmetic functions should only handle performing the arithmetic; all the I/O should be handled by main function (I have been taught that this is a good practice, although I do not have a source right now).

So, the "1" (addition) section of the switch statement in main would look something like this:

scanf("%f", &num1);
scanf("%f", &num2);

result = addition(num1, num2);

printf("...", num1, num2, result);

And the addition function simply looks like this:

float addition(float num1, float num2) {
    return num1 + num2;
}

This doesn't make a much of a difference, but it speeds up your code a little bit.

Instead of storing the result of a calculation in result, it would be faster to just pass the return value an arithmetic functions into the statement/function that you will it next in.

For example, the end of the "1" (addition) section of the switch statement would look like this:

printf("%.2f + %.2f = %.2f\n", num1, num2, addition(num1, num2)); 

This is faster because it reduces memory interaction(however, not by much).


In this section:

printf("Enter a number from the list below\n\n");

printf("1. Addition\n"); //Calculator menu, user must enter a value from 1 - 4 for the program to work
printf("2. Subtraction\n");
printf("3. Multiplication\n");
printf("4. Division\n\n");

You make 5 calls to the printf function when you could easily just put all the strings into one and split them up by newline in a single printf call:

printf("Enter a number from the list below\n\n"
       "1. Addition\n"
       "2. Subtraction\n"
       "3. Multiplication\n"
       "4. Division\n");

This is also faster as you don't have to call a function 5 times.


Only use printf when you are need to use the formatting feature. Other than that, you are over-complicating things.

If you merely need to output a string with a newline here or there, use the built-in puts function; it automatically outputs a string with a newline on the end (and you can add more if needed).

For example, this line:

printf("You entered Addition\n\n");

Becomes:

puts("You entered Addition\n");
\$\endgroup\$
  • \$\begingroup\$ I agree with your function definition. However, would it not be better to not declare result and instead replace the variable with `addition(num1, num2) in the printf-statement? \$\endgroup\$ – Ishaan May 25 '15 at 21:45
  • \$\begingroup\$ I believe so, as by putting the return value of addition in to result and then using result in printf, you are making 2 memory interactions. However, if you just put addition(num1, num2) in the printf call, you are making 0 memory interactions. \$\endgroup\$ – SirPython May 25 '15 at 21:57
  • \$\begingroup\$ I think the terms you are looking for about the first item would be separation of concerns or single responsibility. A function is better if doing one thing only, e.g.: one gets the user input, the other processes it. \$\endgroup\$ – glampert May 25 '15 at 22:21
  • 5
    \$\begingroup\$ Moving the variables and function call into the printf statement might improve performance, but if you optimise sensibly, like you should at all times do, (-O2), the compiler will almost certainly do that for you. If using the variables is clearer, use them. I'm not certain about the printf call chaining, you might reduce the number of instructions that way, but I've seen with my own eyes the printf -> puts conversion happening. This post depends on that sole fact. \$\endgroup\$ – tomsmeding May 26 '15 at 10:13
10
\$\begingroup\$
  1. I would give the user a chance to correct typing errors instead of just dumping them out of the program. Something like this, maybe:

    bCont = false;      // flag to denote good input, will be true with a good input.
    
    printf("Enter a number from the list below\n\n");
    printf("1. Addition\n"); 
    printf("2. Subtraction\n");
    printf("3. Multiplication\n");
    printf("4. Division\n\n");
    
    do
    {
        printf("Enter number: "); //User input for the calculator menu
        scanf("%d", &menu);
    
        if((menu < 1) || (menu > 5))
        {
             printf("invalid entry, please try again.");
        }
        else
        {
              bCont = true;
        }
     } while(!bCont);
    
  2. You should check for the possibility of the user entering zero for the denominator in the division case, and print a warning. Something like this:

    if(0 != num2)
    {
        result = num1 / num2; //Division calculation
    }
    else
    {
        printf("Attempting division by zero, exiting");
        exit(1)
    }
    
  3. I dislike repeated code, and you have a lot of it. I would rewrite the switch statement something like what is shown below.

    printf("Enter first number: "); //User input for first number
    scanf("%f", &num1);
    
    printf("Enter second number: "); //User input for second number
    scanf("%f", &num2);
    
    printf("\n");
    
    switch(menu) //switch statement for menu
    {
    case 1:
        result = num1 + num2; //Addition calculation
        break;
    case 2:
        result = num1 - num2; //Subtraction calculation
        break;
    case 3:
        result = num1 * num2; //Multiplication calculation
        break;
    case 4:
        result = num1 / num2; //Division calculation
        break;
    default:
        printf("Enter correct number e.g 1 - 4\n"); 
        exit(1);
    }
    printf("result is%.2f\n", result);
    
  4. If you really want the operator in the output line, you can used a variable to hold this and set it in each branch of the switch statement.

  5. I would allow the user to jump back to the beginning if they wanted. I would do this something like this:

    int main(int argc, char** argv)
    {
        float  num1, num2, result;
        char   choice, opcode;
        bool   bCont = true;  // flag to denote good input
    
        while(1)
        {
            puts("Enter a choice from the list below\n\n");
            puts("1 Addition\n"); /
            puts("2 Subtraction\n");
            puts("3 Multiplication\n");
            puts("4 Division\n\n");
            puts("q Quit\n\n");
    
            do
            {
                bCont = true;
                puts("Enter choice: "); //User input for the calculator menu
                scanf("%c", &choice);
    
                if(choice == 'q')
                {
                    exit(0);
                }
    
                if((choice < '1') || (choice > '4'))
                {
                    puts("enter 1 - 4 or q, please try again");
                    bCont = false;
                }
            } while (!bCont);
    
            printf("Enter first number: "); //User input for first number
            scanf("%f", &num1);
    
            printf("Enter second number: "); //User input for second number
            scanf("%f", &num2);
    
            switch(choice) //switch statement for menu
            {
                case '1':
                    result = num1 + num2; //Addition calculation
                    opcode = '+'
                    break;
    
                case '2':
                    result = num1 - num2; //Addition calculation
                    opcode = '-'
                    break;
    
                case '3':
                    result = num1 * num2; //Addition calculation
                    opcode = '*'
                    break;
    
                case '4':
                    if(0 != num2)
                         result = num1 / num2; //Addition calculation
                    else
                         result = Inf;
                    opcode = '/'
                    break;
            } // end of switch
             printf(printf("%.2f %c %.2f = %.2f\n", num1, opcode, num2, result);
        } // end of infinite while loop
    }
    

Just a note, notice that I chanced the menu choice from an integer to a character so I could accept both numeric and character values. Also look at the conditional I used: if((choice < '1') || (choice > '4')) this is based on the knowledge that the ASCII character code for the digits are consecutive and run from 0x30 to 0x39, so I can test for a valid input by determining if the character codes are in this range. This probably is not portable to systems using other encoding than ASCII.

\$\endgroup\$
  • \$\begingroup\$ EBCDIC also has numerals in consecutive positions, it's just the alphabet that's all over the place. :) \$\endgroup\$ – luser droog May 26 '15 at 6:17
  • 1
    \$\begingroup\$ "This probably is not portable to systems using other encoding than ASCII" -- The C standard guarantees that '0'-'9' are contiguous as seen in many answers to this post \$\endgroup\$ – Spikatrix May 26 '15 at 9:00
  • 1
    \$\begingroup\$ Point 3 prints an uninitialized result if the default case is executed. \$\endgroup\$ – 404 May 26 '15 at 9:46
  • \$\begingroup\$ @eurotrash You're correct, I missed that (probably was already thinking of doing the input check earlier in the program). I changed that break to an exit -- not optimal, but at least keeps with the spirit of the original program. \$\endgroup\$ – thurizas May 26 '15 at 12:46
  • \$\begingroup\$ Your bCont = true; should be in the inner loop. Otherwise, you wont be able to exit the loop except with 'q' after the first wrong choice. \$\endgroup\$ – OxTaz May 26 '15 at 15:24
4
\$\begingroup\$
  • You should really consider making functions for each mathematical operation and keep the main() simple.

As an example:

float addition(float num1, float num2){

    return num1 + num2;
}

//or

void addition(float num1, float num2){

    float result = num1 + num2;
    printf("%0.2f", result);

}

In the second example, you create and destroy result each function call.

  • Consider asking the user if they would like to return to the menu, in case they want to do multiple calculations.

  • You don't need multiple printf statements for the menu options. Simply continuing as such is needed:

    printf("1. Addition\n2. Subtraction\n3. Multiplication\n4. Division\n\n");

\$\endgroup\$
  • 2
    \$\begingroup\$ this functions does way too many things. things that are common with all other functions. \$\endgroup\$ – njzk2 May 26 '15 at 18:00
  • \$\begingroup\$ I realized that after I posted it, probably should fix it. \$\endgroup\$ – Ishaan May 26 '15 at 18:20
1
\$\begingroup\$

Other answers have reviewed your code quite nicely, but all of them haven't mentioned one thing: Checking the return value of scanf.

For example, a piece of your code:

printf("Enter number: ");
scanf("%d", &menu);

does not check the return value of scanf. Change it to:

int retVal;              // Variable for capturing the return value
printf("Enter number: ");

while( (retVal = scanf("%d", &menu)) != 1  || menu < 1 || menu > 4)
{
    int c;
    while( (c = getchar()) != EOF && c != '\n'); // Clear the stdin

    printf("Invalid input; try again \n"); // Or use puts
}

Here, retVal captures the return value of scanf. It can be EOF, 0 or 1 in your case.

  • Return value of 1 signifies that scanf was successful in getting a number from the user.
  • Return value of 0 signifies that scanf was unsuccessful in getting a number from the user. This means that the user has inputted invalid data like characters instead of a number.
  • Return value of -1 (EOF) signifies that scanf has encountered a reading error or EOF.

The body of the while loop will execute in case of invalid input. The first two lines of the body of this loop clears/flushes the stdin. This means that the standard input stream (stdin) is cleared of characters. This is done by reading and discarding all characters until EOF or \n (newline character).

This is done so that scanf does not go on reading the same invalid data ( when user enters characters or other invalid data) again and again and the loop from turning into an infinite loop.

The last line prints the error message and prompts the user to enter valid data.

You can use the same technique for the other scanfs in your code too.

\$\endgroup\$
-2
\$\begingroup\$

First of all, create functions for the operators you use. For instance, if you are performing addition, write the function as:

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

Same for subtraction, multiplication and division.

Also, why do you want to get the values from the user for each case? Try to use function for it:

int get()
{int a;
  scanf("%d",&a);
  return a;
}

Call the function get() whenever you want to get the input from the user.

\$\endgroup\$
  • 4
    \$\begingroup\$ Your answer has some interesting observations, but the formatting and presentation are horrible. If you improve your answer's presentation it may get upvotes. \$\endgroup\$ – rolfl May 26 '15 at 13:09

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.