Primitive console string calculator in C

Question

Recenlty I have finished this primitive console string calculator in C. I need your balanced criticism, I'd like to know what I have to correct, remake and learn.

Permitted operations: + - * / ( ) ^

Sample input string: 11 * -10 + 15 - 35 / (35 - 11) + 2^2 =

main.c

#include "stack.h"
#include <stdio.h>
#include "calculating.h"
#include <errno.h>

#define EXPR_LENGTH 1000

main()
{
    while (1)
    {
        Stack* nums = createEmptyStack();
        ValType lastval, prevval, result;
        char* temp = NULL, temp_char;
        char num_buf[NUM_LENGTH], expr_buf[EXPR_LENGTH];
        CalcErrors err_flag = NO_ERR;

        system("cls"); // clear the screen
        printf("Enter the expression: ");

        /* Transform the infix notation to the reverse Polish notation and processing the error if necessary */
        if (transformToRPN(expr_buf) != NO_ERR) {
            err_flag = WRONGSYM_ERR;
            goto break_loop;
        }

        temp = expr_buf; // to don't change the pointer expr_buf

        /* Parsing the operands from expression in the reverse Polish notation */
        while (temp_char = getOperand(num_buf, temp, &temp))
        {
            switch (temp_char) {
                /* Processing the operators */
            case SUB: case ADD: case MUL: case POW: case DIV:
                if (nums->size < 2) { // if operators are less than 2 (to avoid a pop() of empty stack)
                    err_flag = LACK_OPERAND;
                    goto break_loop; // break the nested loop
                }

                lastval = pop(nums); // last added operand
                prevval = pop(nums);

                result = useOperator(prevval, lastval, temp_char);

                if (errno || (!lastval && temp_char == DIV)) { // math errors (out of range, zero division etc)
                    err_flag = MATH_ERR;
                    goto break_loop; // break the nested loop
                }

                push(nums, result);
                break;

                /* Adding a value in the stack */
            case VAL:   push(nums, atof(num_buf));  break;
                /* End of input or other errors */
            case EQ:    err_flag = END_CALC;        goto break_loop;
            default:    err_flag = WRONGSYM_ERR;    goto break_loop;
            }
        }

    break_loop: // if input was incorrect, loop will be broken

        printf("\n");

        /* Processing the errors or displaying an answer */
        switch (err_flag) {
        case WRONGSYM_ERR:  printf(MSG_WRONGSYM); break;
        case MATH_ERR:          printf(MSG_MATH); break;
        case END_CALC:      nums->size == 1 ? printf("= %f\n", pop(nums)) : printf(MSG_LACKOP); break;
        case LACK_OPERAND:  printf(MSG_LACKOP); break; // FIX THE TEXT
        }

        printf("\nDo you want to calculate something else? y/n > ");

        getchar(); // skip the '\n' symbol
        temp_char = getchar();

        if (temp_char != 'y' && temp_char != 'Y')
            break;
    }
}

stack.h

#define ELEMENTARY_SIZE_STACK 5 // initial the length of stack
#define RESIZE_ADDING 5 /* how much will be add when the stack size 
                            will be equal to ELEMENTARY_SIZE */
#include <stdlib.h> 

#define ValType double
#define ValType_IOSPECIF "%lf"

typedef struct stack
{
    ValType* data;
    size_t max; // such value when the stack size should be extend
    size_t size;
} Stack;

Stack*      createEmptyStack(void);
void        push(Stack* s, ValType val);
void        deleteStack(Stack* st);
static void resize(Stack* st);
ValType     pop(Stack* st);
ValType     peek(Stack* st);
void        printStack(Stack* st);

#define MSG_MALLOC "ERROR! MALLOC RETURNS NULL!\n"
#define MSG_REALLOC "ERROR! REALLOC RETURN NULL!\n "

calculating.h

#include <float.h>

/* Errors processing */
typedef enum calcerr {
    NO_ERR,
    MATH_ERR,
    WRONGSYM_ERR,
    LACK_OPERAND,
    END_CALC
} CalcErrors;

#define MSG_WRONGSYM "ERROR! You entered the wrong symbol(s).\n"
#define MSG_LACKOP  "ERROR! The input format is not correct.\n"
#define MSG_MATH "ERROR! Math error!\n"

/* Operators, delimiters, other characters */
#define VAL 1
#define SUB '-'
#define ADD '+'
#define MUL '*'
#define DIV '/'
#define EQ '='
#define POW '^'
#define OP_BRACE '('
#define CL_BRACE ')'

#define DELIM_DOT '.'
#define DELIM_COMMA ','
#define IS_DELIM(x) ((x) == DELIM_DOT || (x) == DELIM_COMMA)

#define SPACE ' '

/* Priority of operators */
#define PRIOR_SUB 1                 
#define PRIOR_ADD 1
#define PRIOR_MUL 2
#define PRIOR_DIV 2
#define PRIOR_POW 3
#define PRIOR_OP_BR 0
#define PRIOR_CL_BR 0

#define PRIOR(x) getPriority(x)

/*Calculation and parsing expressions on the RPN (Reverse Polish Notation) */
int     getOperand(char* num, char expression[], char** end);
double  useOperator(double leftval, double rightval, char oper);

/*transform the infix notation to the reverse Polish notation*/
int         isOperator(char op);
static int  getPriority(char op);
CalcErrors  transformToRPN(char result[]); 

#define NUM_LENGTH DBL_DIG // maximum length of number

calculating.c

#include "calculating.h"
#include "stack.h"
#include <stdio.h>
#include <math.h>

/* Function for working with an expression on the RPN */

int getOperand(char* num, char expression[], char** end)
{
    unsigned counter = 0;
    char* ptr = expression;

    /* NULL pointer checking*/
    if (!ptr) return 0;

    /* Spaces skipping before */
    while (isspace(*ptr))
        ptr++;

    /* The unary minus checking */
    int negative_flag = 0;
    if (*ptr == SUB)
    {
        if (isdigit(*++ptr))   // if the next character is a digit
            negative_flag = 1;
        else
            ptr--;
    }

    /* The return of the any not digit character */
    if (!isdigit(*ptr))
    {
        *end = ++ptr;
        return *(ptr - 1);
    }

    /* Making a float number and the delimiter processing */
    while (isdigit(*ptr) || IS_DELIM(*ptr))
    {
        if (*ptr == DELIM_COMMA)
            *ptr = DELIM_DOT; // for atof()

        if (negative_flag)
        {
            num[counter++] = SUB;
            negative_flag = 0; // in order to add the minus symbol one time in the head of array
        }

        num[counter++] = *ptr;
        ptr++;
    }

    num[counter] = '\0';
    *end = ptr; // pointer to the next character (to continue reading an operand from a new location)
    return VAL;
}

double useOperator(double leftval, double rightval, char oper)
{
    double result;
    switch (oper) {
    case ADD: result = leftval + rightval; break;
    case SUB: result = leftval - rightval; break;
    case MUL: result = leftval * rightval; break;
    case DIV: result = leftval / rightval; break;
    case POW: result = pow(leftval, rightval); break;
    default: exit(1);
    }

    return result;
}

/* Function for working with an expression on the infix notation */

int getPriority(char ch) {
    switch (ch) {
    case SUB: return PRIOR_SUB;
    case ADD: return PRIOR_ADD;
    case MUL: return PRIOR_MUL;
    case DIV: return PRIOR_DIV;
    case POW: return PRIOR_POW;
    case OP_BRACE: return PRIOR_OP_BR; // opening parethesis
    case CL_BRACE: return PRIOR_CL_BR; // closing parethesis
    default: return -1;
    }
}

int isOperator(char x)
{
    return (x == SUB || x == ADD || 
            x == MUL || x == DIV || 
            x == POW);
}

CalcErrors transformToRPN(char result[])
{
    Stack* ops = createEmptyStack();
    char temp_ch;
    unsigned counter = 0;

    while ((temp_ch = getchar())) 
    { 
        /* Spaces skipping before */
        while (isspace(temp_ch))  
            temp_ch = getchar();

        /* Ending the input, fully write the remaining contents of stack*/
        if (temp_ch == EQ) 
        {
            while (ops->size)
            { 
                result[counter++] = (char)pop(ops);
                result[counter++] = SPACE;
            }
            result[counter++] = EQ;
            result[counter] = '\0';
            return NO_ERR;
        }

        /* Cheking the unary minus, and if it's not unary, return character after minus used to check back 
        and process a minus as an operator*/
        if (temp_ch == SUB)
        {
            temp_ch = getchar(); // read next symbol and if the next character is a digit
            if (isdigit(temp_ch)) { 
                if (temp_ch != '0') // to don't allow the '-0'
                    result[counter++] = SUB;
            }
            else {
                ungetc(temp_ch, stdin);
                temp_ch = SUB;
            }
        }

        /* Making a number */
        if (isdigit(temp_ch))
        {
            while (isdigit(temp_ch) || IS_DELIM(temp_ch) )
            { 
                result[counter++] = temp_ch;
                temp_ch = getchar();
            }   
            ungetc(temp_ch, stdin); // return the extra character
            result[counter++] = SPACE;
        }

        /* Else check the operator and push it to the stack */
        else 
        {
            if (isOperator(temp_ch)) 
            {
                if (!ops->size) // if stack is empty (to avoid an error after pop)
                    push(ops, (double)temp_ch);
                else 
                {
                    if (PRIOR(temp_ch) <= PRIOR((char)peek(ops))) // > if priority of new operator is higher than operator 
                    {                                             // in the top of stack, then the old operation will be
                        result[counter++] = (char)pop(ops);       // display and new operator push in the stack < 
                        result[counter++] = SPACE;
                    }

                    push(ops, (double)temp_ch);
                }
            }
            /* Operators inside parathesises processing */
            else if (temp_ch == OP_BRACE)
                push(ops, (double)temp_ch);
            else if (temp_ch == CL_BRACE)  // if it's a closing parethesis, then it write all of operators before the
            {                              // opening parethesis not including it
                char tmp;
                while ((tmp = (char)pop(ops)) != OP_BRACE) // until the opening parathesis
                {
                    result[counter++] = tmp;
                    result[counter++] = SPACE;
                }
            }
            /* Any other symbols */
            else  
                return WRONGSYM_ERR;
        }   
    }
}

stack.c

#include "stack.h"
#include <string.h>
Stack* createEmptyStack(void)
{
    Stack* st = malloc(sizeof(Stack));
    if (st == NULL) {
        printf(MSG_MALLOC);
        exit(EXIT_FAILURE);
    }

    st->data = malloc(sizeof(ValType)*ELEMENTARY_SIZE_STACK);
    if (st->data == NULL) {
        printf(MSG_MALLOC);
        exit(EXIT_FAILURE);
    }

    st->max = ELEMENTARY_SIZE_STACK; 
    st->size = 0;
    return st;
}

void deleteStack(Stack* st)
{
    free(st->data);
    free(st);
    st = NULL;
}

static void resize(Stack* st)
{
    st->max += RESIZE_ADDING;
    st->data = realloc(st->data, st->max*sizeof(ValType));
    if (st->data == NULL) {
        printf(MSG_REALLOC);
        exit(EXIT_FAILURE);
    }
}

void push(Stack* st, ValType val)
{
    if (st->size == st->max)
        resize(st);
    st->data[st->size++] = val;
}

ValType pop(Stack* st)
{
    if (!st->size) { // if there is a request to pop the value from empty stack
        printf("ERROR (FUNCTION POP): STACK SIZE < 0\n");
        exit(EXIT_FAILURE);
    }
    st->size--;
    return st->data[st->size];
}

/* In this function just look to the top of the stack */
ValType peek(Stack* st)
{
    if (!st->size) { // if there is a request to pop the value from empty stack
        printf("ERROR (FUNCTION PEEK): STACK SIZE < 0\n");
        exit(EXIT_FAILURE);
    }
    return st->data[st->size-1];
}

void printStack(Stack* st)
{
    int i;
    for (i = 0; i < st->size; i++)
        printf(ValType_IOSPECIF " ",st->data[i]);
    printf("\n");
}

Answer 1

Generalities and trivialities:

I recommend #includeing all wanted system headers before any private headers. This avoids any possibility of changing the meaning of any system header by defining a macro that happens to be meaningful to it.

static functions and variables generally should not be declared in header files, unless you actually want every file that includes the header to provide (for functions) or have (for variables) its own copy. In your code, this applies to the resize() and getPriority() functions. If you want to prototype those then put each prototype near the top of the C file where the corresponding function definition appears.

The string constant for MSG_REALLOC contains a trailing space character that you probably did not intend.

There's no point in deleteStack() setting its argument to NULL, as the caller will not see any effect of it.

On the other hand, you never call deleteStack() or printStack(). Inasmuch as this is a program, not a reusable library, it is wasteful to define functions that you never call.

You do not declare the type of main(). It does default to int, which is the required type, but failing to declare that is poor style.

It looks like you could replace much of function getOperand() with a call to strtod().

Regarding ValType:

If you see value in defining the value data type as a macro -- and I think that may be a bit overkill -- then at least define it conditionally. The point of using a macro would be to make it easy to switch to a different type, and you've gone only half way on that. If you make the definition(s) conditional, then it would not be necessary to modify the header at all to change type. For example:

#idndef ValType
#define ValType double
#endif

#ifndef
#define ValType_IOSPECIF "%f"
#endif

Note in the above example code that I have changed the default definition of ValType_IOSPECIF. The result is the correct form, per the standard, for a printf() field descriptor for a double (there is none specific to float because float arguments to printf() are automatically promoted to double, as a consequence of those falling among printf()'s variable arguments). Note also that printf() and scanf() are asymmetric in this particular regard, so if you needed to support both then you would need separate macros for the two contexts.

Regarding function transformToRPN():

Function transformToRPN() does not just transform an expression, it inputs one. At minimum, therefore, the function is poorly named, but it's questionable that these two behaviors are combined in a single function.

In function transformToRPN(), you have an else block whose sole contents is an if/elseif/else tree, and moreover, that conditions in that inner tree test the same variable that those on the outer tree do. I recommend merging the inner tree into the outer tree for clarity and symmetry.

Function transformToRPN() receives a pointer to a buffer into which to record an RPN transformation of the input expression, but it does not receive the size of that buffer, and it performs no bounds checking. It would be extremely easy for a user to intentionally cause a buffer overrun, which is a favorite cracking tactic.

Function transformToRPN() assigns the return value of getchar(), an int, to a variable of type char. If char happens to be a signed type, then this produces implementation-defined behavior for some possible inputs. Whether char is signed or unsigned, you cannot distinguish one possible input char from an EOF.

Moreover, the while() loop seems unlikely to terminate, because at the end of the stream getchar() returns EOF, which normally has value -1, and always has a value outside those that can result from converting a valid char value, including 0, to type unsigned char. The loop will terminate only if getchar() returns 0.

Answer 2

Design

1. #define VAL 1 #define SUB '-' #define ADD '+' etc. are too generically named. Suggest a naming convention that does not so readily conflict with other code. Same for function names push(), resize(), pop(), peek(). In general, the .h files add name all over the name-space. Suggest a corner of it instead.

2. I would expect code guards in the .h files

   // or some uppercase variant
   #ifndef stack_h
   #define stack_h
      ....body of .h file
   #endif
   

3. Rather than allocating per the size of the type, allocate based on the size of the variable. Less error prone and easier to maintain.

   //Stack* st = malloc(sizeof(Stack));
   //st->data = malloc(sizeof(ValType)*ELEMENTARY_SIZE_STACK);
   Stack* st = malloc(sizeof *st);
   st->data = malloc(sizeof *(st->data) * ELEMENTARY_SIZE_STACK);
   

4. Unclear why code is avoiding -0. No need for that.

   if (temp_ch != '0') // to don't allow the '-0'
   

5. getOperand() does not seem to allow .123 as input. I would expect this to pass.

Minor Stuff

1. Suggest bool return type for isOperator().

2. The test for a , to change it into . for atof() is interesting, yet a problem. atof() accepts a , or . depending on locale. I would not folding these two characters together without checking locale and then calling a locale sensitive function.

3. Missing check for unary +. Code check for unary -. Complete code would check for both.

4. Ensure code passes a value in the range of unsigned char to isspace(). Aside from EOF, passing negative value to isspace() is UB.

   // while (isspace(*ptr))
   while (isspace((unsigned char) *ptr))
   

5. As the return ValType pop() may be different from double, ensure a matching type to %f.

   // printf("= %f\n", pop(nums))
   printf("= %f\n", (double) pop(nums))
   

6. For debugging, using "%e" is more illuminating.

   // printf("= %f\n", (double) pop(nums))
   printf("= %e\n", (double) pop(nums))
   

---

Quite a bit more exist to review, but GTG.

Answer 3

First, things my compiler complains about with warnings turned on:

• As PellMel has already said, you haven't declared the return type of main. It's good practice to do so.
• In calculating.c you forgot to include ctype.h. This header contains declarations for isspace and isdigit among others.
• In addition, you forgot to include stdio.h in stack.c, where you use printf.

So on to running your program. The first thing I noticed was the following line:

sh: cls: command not found

In your program, you use system("cls") to clear the screen. This is unfortunately not cross platform compatible, as the cls command is Windows specific. Let me quote this post by @Edward on the topic.

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything.

The most common library for cross platform advanced console manipulation is ncurses. There is a Windows version available from here.

On the other hand, I don't think clearing the screen is necessary at all from a user experience perspective. Think of any interactive interpreter you might have used: python, irb, or even cmd. After one line of input is received, a new one can be entered without the screen being cleared. This is useful because the user might want to look at their previous calculations when making new ones. Therefore, I think an interaction between user and program should look more similar to this:

Calculator by luckystrrrike. Type quit to exit.
expression> 2 * 3 =
= 6
expression> 6 + 1 =
= 7
expression> quit

Alternatively, a user could also press CtrlC to exit the program.

If you choose to go down the route of making the calculator more "interpreter like", adding line history etc seems like the next step. Line history is where pressing up on the keyboard scrolls through previously entered lines, which could be really helpful, e.g. if you typed something out wrong. You could implement this by yourself, but fortunately there is a library readily available for this as well called readline.

Turning to the expression syntax, I wonder why they have to be terminated by = instead of a newline. For a user (at least me), newlines are a more intuitive terminator than the equals sign, but YMMV.

Overall, writing a calculator is an interesting project to learn about parsing I've done myself. I think you've done a good job.

Answer 4

Don't call the functions from <ctype.h> with a char as an argument -- always cast it to unsigned char. See the NetBSD documentation for an explanation.