8
\$\begingroup\$

Currently, I am interested in programming. I have programmed a calculator in C. Everything works, but I do not feel completely confident in my solution.

Are there any logic or efficiency issues with my code?

Supported operations: \$+\$, \$-\$, \$*\$, and \$/\$.

Algorithm: Flowchart

main.c

#include "practice.h"

int main() {
    char arr[100];
    char c;
    int length = 0;
    int result;

    while((c=(getchar())) != '\n')
        arr[length++] = c;

    result = EquationToQueue(arr,length);
    printf("%d", result);
}

practice.c

#include "practice.h"

queue* make_queue(size_t capacity, enum data_type type) {
    queue* new_queue;
    new_queue = (queue*)malloc(sizeof(queue));
    new_queue->top = 0;
    new_queue->bottom = 0;
    new_queue->capacity = capacity;
    new_queue->type = type;
    new_queue->element = (queue_element*)malloc(sizeof(queue_element) * capacity);

    return new_queue;
}
void push_queue(queue* object_queue, enum data_type type, ...) {
    va_list ap;
    va_start(ap,type);

    if(object_queue->top >= object_queue->capacity) {
        printf("Over %d is impossible!", object_queue->capacity);
        exit(0);
    }

    switch(type) {
        case queue_INT :
            object_queue->element[object_queue->top++].data.val_i = va_arg(ap,int);
            break;
        case queue_CHAR :
            object_queue->element[object_queue->top++].data.val_c = (char)va_arg(ap,int);
            break;
        default :
            printf("Unknown type in push_queue()\n");
            exit(0);
    }
    va_end(ap);
}
void pop_queue(queue* object_queue, void *return_val) {
    if (object_queue->top == object_queue->bottom) {
        printf("queue is empty!\n");
        exit(0);
    }

    switch(object_queue->type) {
        case queue_INT :
            *(int*)return_val = object_queue->element[object_queue->bottom++].data.val_i;
            break;
        case queue_CHAR :
            *(char*)return_val = object_queue->element[object_queue->bottom++].data.val_c;
            break;
        default :
            printf("Unknown type in pop_queue()\n");
            exit(0);
    }

}
void delete_queue(queue* object_queue) {
    free(object_queue->element);
    free(object_queue);
}
int calculate(int num1, char op, int num2) {
    switch(op) {
    case '+' :
        return num1+num2;
    case '-' :
        return num1-num2;
    case '*' :
        return num1*num2;
    case '/' :
        return num1/num2;
    default :
        printf("operation is incorrect in calculate()!\n");
        exit(0);
    }
}
int EquationToQueue(char* arr, int length) {
    queue* i_queue;
    queue* c_queue;
    int i;
    int before = 0;

    int num1;
    int num2;
    char op;
    char op_tmp;
    int num_tmp;
    int result;

    i_queue = make_queue(50, queue_INT);
    c_queue = make_queue(50, queue_CHAR); //make two queues;

    for (i=0;i<length;i++) {
        switch(arr[i]) {
            case '+' :
            case '-' :
            case '*' :
            case '/' :
                push_queue(i_queue, queue_INT, before);
                push_queue(c_queue, queue_CHAR, arr[i]);
                before = 0;
                break;
            default :
                before = before*10 + arr[i] - '0';
                break;
        }
    }
    push_queue(i_queue, queue_INT, before); //save numbers in int queue and operations in char queue.

    pop_queue(i_queue, &num1);
    pop_queue(c_queue, &op);
    pop_queue(i_queue, &num2); //initialize num1, op, num2;

    while(i_queue->top != i_queue->bottom) {
        pop_queue(c_queue, &op_tmp);
        switch(op_tmp) {
            case '+':
            case '-':
                num1 = calculate(num1, op, num2);
                op = op_tmp;
                pop_queue(i_queue, &num2);
                break;
            case '*':
            case '/':
                pop_queue(i_queue, &num_tmp);
                num2 = calculate(num2, op_tmp, num_tmp);
                break;
            default :
                printf("operation is incorrect in EquationToqueue()!\n");
                exit(0);
        }
    }
    result = calculate(num1, op, num2);// calculate algorithm.
    return result;
}

practice.h

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

enum data_type {
    queue_INT,
    queue_CHAR
};

typedef struct {
    union {
        int val_i;
        char val_c;
    } data;
}queue_element;

typedef struct {
    size_t top;
    size_t bottom;
    size_t capacity;
    queue_element *element;
    enum data_type type;
} queue;

queue* make_queue(size_t capacity, enum data_type type);
void push_queue(queue* object_queue, enum data_type type, ...);
void pop_queue(queue* object_queue, void *return_val);
void delete_queue(queue* object_queue);
int calculate(int num1, char op, int num2);
int EquationToQueue(char* arr, int length);
\$\endgroup\$
0

1 Answer 1

Reset to default
11
\$\begingroup\$

I see a number of things that may help you improve your code.

Add error checking

Data input usually requires a lot of careful consideration for most software. Checking for malformed input and handling it gracefully often takes a considerable amount of thought, time and code. In this code we have a 100 character input buffer, but nothing prevents a user from inputting more that that. The result is a classic stack buffer overflow vulnerability. The while loop input should check for (and prevent!) a buffer overflow.

Perform input sanitation

Closely related to error checking, but slightly different, is to sanitize user input. For example, if the user enters the string "3 + 2 * 5" (with spaces), the rather surprising result is 247394 on my machine, rather than the expected answer of 13 which is returned when the same equation is entered as "3+2*5" (without spaces). It's usually better to adjust user input for such things, or at least throw an error, but to simply give a wrong result is usually unacceptable.

Isolate the internal representation from the interface

The code uses a variadic function push_queue and requires <stdarg.h> to implement it, but I would note that the interface (that is, the portion in the "practice.h" file) only requires <stdio.h> (for the definition of size_t) and doesn't require either <stdarg.h> or <stdlib.h> listed there. Instead, those two should be moved to the practice.c file. The implementation details should be isolated to the extent possible. This will make it easier to modify the code without needing to change calling code and provides for a clear and minimal interface specification.

Fix the bug

If the user enters "12/2/3" the expected result is 2 but in fact what happens is that the program evaluates 2/3 first (as integers) and gets a result of 0. It then tries to evaluate 12/0 which results, on my machine, in a floating point exception error (divide by zero) and a crash. Evaluating this expression from left to right (and also checking for and handling divide by zero) would be a better way to handle this.

Check the return value of malloc before dereferencing

The primary way that the operating system uses to tell your program that it's running out of memory is to return NULL when the program requests a memory allocation. For that reason, you must check the return value before dereferencing it to make sure it's not NULL.

Use include guards

There should be an include guard in the .h file. That is, start the file with:

#ifndef PRACTICE_H
#define PRACTICE_H
// file contents go here
#endif // PRACTICE_H

Be careful with assumptions about size_t

The current code includes capacity which is defined as a size_t type, which is a good choice for that variable. However the code also includes this line:

printf("Over %d is impossible!", object_queue->capacity);

The problem with this, is that the %d specifier expect an int and size_t may or may not be an int. In fact, on my 64-bit Linux machines, it's a long int and so the compiler complains. The code can be written to accommodate either by using the z prefix for the argument; that is, make it %zd or %zu to have the compiler automatically use an appropriate type for size_t.

Separate input, output and calculation

To the degree practical it's usually good practice to separate input, output and calculation for programs like this. By putting them in separate functions, it isolates the particular I/O for your platform (which is likely to be unique to that platform or operating system) from the logic of your program (which does not depend on the underlying OS). For example, instead of having various error messages and calls to exit() scattered within functions such as push_queue and pop_queue, it might be better to have those functions return an error code that the caller can then decide how to handle. For example, if you ever wanted to make this into a GUI program without console I/O, then the current code would not work as well as if the code were divided as I suggest.

Eliminate "magic numbers"

In a number of cases, the code uses "magic numbers" such as 10 and 50 that have no obvious meaning. These would be better as named constants. In some cases, the limit seems quite arbitrary, and might reasonably need to be altered by a user of the code.

Don't leak memory

This code has a delete_queue function, but it's never called and the result is therefore a memory leak. That should be fixed (by deleting the two allocated queues in EquationToQueue) so that the code can be reliably reused.

Consider better naming

Although it uses queues internally, the EquationToQueue function is misnamed. What it really does is to create a single integer evaluation of the passed numeric equation. The fact that it uses queues or any other particular data structure is, and probably should be, irrelevant to the user. For that reason, I'd suggest renaming it to something like EvaluateEquation. It also may be that instead of having all of the queue-related functions in the interface, that only that single function would be exposed to users and all the rest of those details might be hidden in the implementation file. You'll have to consider how you want the code to be used to evaluate that suggestion.

Use return values

The pop_queue function has a parameter named return_val. It would make much more sense to use the actual return value rather than such a parameter. That is, instead of this:

void pop_queue(queue* object_queue, void *return_val);

redefine the function to be:

int pop_queue(queue* object_queue);

Then instead of calling it like this:

pop_queue(i_queue, &num_1);

It would be called like this:

num1 = pop_queue(i_queue);
\$\endgroup\$
3
  • \$\begingroup\$ Thanks very very very much. I didn't know so many problems existed in my code before i saw this answer. But i have a question in "Use return values". Because i use two types data in queue, so return type is not decided and i use void function. If i redefine return value of pop_queue function as int, isn't there any problem? \$\endgroup\$ Commented Aug 25, 2016 at 14:56
  • 1
    \$\begingroup\$ @DennyKim: Good question. No, there won't be a problem because the char undergoes an implicit conversion to int when being returned from pop_queue. Since it was originally a char, converting back to char from int is guaranteed to work. \$\endgroup\$ Commented Aug 25, 2016 at 15:08
  • \$\begingroup\$ @Edward "Since it was originally a char converting back to char from int is guaranteed to work." I agree that it'll work but not because it was originally a char: Converting void * to char and back wouldn't necessarily work. I'm pretty sure C doesm't madate that at least. \$\endgroup\$ Commented Aug 31, 2016 at 8:32

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.