2
\$\begingroup\$

In the main() function of the following code, I wrote a very naive command parsing. Is there any more efficient way?

This program is aim to deal with ADD, SUB, MUL, DIV these four arithmetic operations for integers. The input would be in form like: ADD 1 1, MUL 12 90. And also there would be a special input character % which means use the last result. For example, ADD 1 1 would return 2, then ADD % 1 would return 3.

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

typedef int (*calc_function)(int, int);

typedef struct operation {
        char *name;
        calc_function calculate;

} Operation;

int add(int a, int b) { return a+b; }
int sub(int a, int b) { return a-b; }
int mul(int a, int b) { return a*b; }
int divi(int a, int b) { return a/b; }


int calc(int a, int b, int (*c)(int, int)) {
        return c(a, b);
}

int main() {

        char *command = malloc(9);
        int result = 0;
        int a;
        int b;

        Operation ADD = {"ADD", add};
        Operation SUB = {"SUB", sub};
        Operation MUL = {"MUL", mul};
        Operation DIV = {"DIV", divi};

        Operation ops[4] = {ADD, SUB, MUL, DIV};

        while((command = fgets(command, 9, stdin)) != NULL) {

                for(int i = 0; i < 4; ++i) {
                        if (0 == strncmp(ops[i].name, command, 3)) {
                                command = strchr(command, ' ');
                                command++;
                                if (*command == '%') {
                                        a = result;
                                } else {
                                        sscanf(command, "%d", &a);
                                }
                                command = strchr(command, ' ');
                                command++;
                                if (*command == '%') {
                                        b = result;
                                } else {
                                        sscanf(command, "%d", &b);
                                }
                                result = ops[i].calculate(a, b);
                                printf("%d\n", result);
                        }
                }
        }
        free(command);

        return 0;
}

Also, any advice on improving the performence and style of this program would be highly appreciated!

\$\endgroup\$
  • 1
    \$\begingroup\$ The current question title of your question is too generic to be helpful. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How do I ask a good question?. \$\endgroup\$ – BCdotWEB Apr 27 at 8:10
1
\$\begingroup\$

Overall Impression

The code is pretty good. The structure of the code is good, good use of typedef. The code uses that safe version of string comparison strncmp(string1, string2, maxLength). Each Variable is declared on a separate line.

One overall observation is the code is not very extendable.

The program might be more understandable to the user if = is used to get the result rather than %. This would also possibly allow the addition of a modulus operation.

Error Checking

There are at least 2 places in the code where error checking should be performed, the first is always check the return value of malloc(size_t size). If malloc() fails for some reason, it returns NULL access through a null pointer results in UB (Unknown Behavior). Quite often it terminates the program in some nasty manner.

It's always best to check user input for errors so that they can be reported and the user can correct their error.

Magic Numbers

There are Magic Numbers in the main() function (9, 4 and 3), it might be better to create symbolic constants for them to make the code more readable and easier to maintain. These numbers may be used in many places and being able to change them by editing only one line makes maintenance easier.

Numeric constants in code are sometimes referred to as Magic Numbers, because there is no obvious meaning for them. There is a discussion of this on stackoverflow.

In the case of the array of Operations called ops you don't need to specify 4 in the array definition, and you can get the size after the creation of the array using this formula: sizeof(ops)/sizeof(*ops);

In the cases of using 9 and 3 define a constant values that can change the value throughout the program:

At the top of the file:

#define INPUT_COMMAND_SIZE 9
#define OPERATION_SIZE 3

Complexity

The function main() is too complex (does too much). As programs grow in size the use of main() should be limited to calling functions that parse the command line, calling functions that set up for processing, calling functions that execute the desired function of the program, and calling functions to clean up after the main portion of the program.

There is also a programming principle called the Single Responsibility Principle that applies here. The Single Responsibility Principle states:

that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by that module, class or function.

Declare Variables When Needed

The original version of the C programming language did require variables to be defined at the top of the function, however, this hasn't been true since 1989. It is better to declare the variables when they are necessary to reduce the scope of the variables and make the code easier to understand and maintain.

Initialize Local Variables

The C programming language does not automatically initialize variables. Uninitialized variables can sometimes be the cause of UB. It is generally a best practice to always initialize variables when they are declared.

Horizontal Spacing

Code is much more readable when there is a space separating operators and operands in an expression: return a - b;.

System Macros for Exit Status

The code already includes stdlib.h for the definitions of malloc() and free(). Since stdlib.h is already included it would be better to use the system constants EXIT_SUCCESS and EXIT_FAILURE which can make the code more portable. These constants are defined in stdlib.h.

Example Using Suggestions

Note error checking on user input hasn't been added.

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

#define INPUT_COMMAND_SIZE 9
#define OPERATION_SIZE 3

typedef int (*calc_function)(int, int);

typedef struct operation {
    char *name;
    calc_function calculate;

} Operation;

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

int sub(int a, int b)
{
    return a - b;
}
int mul(int a, int b)
{
    return a * b;
}

int divi(int a, int b)
{
    return a / b;
}

int programLoop(Operation *ops, int opsCount)
{
    int programStatus = EXIT_SUCCESS;
    char *command = malloc(INPUT_COMMAND_SIZE);
    if (command != NULL){
        while((command = fgets(command, INPUT_COMMAND_SIZE, stdin)) != NULL) {

            for(int i = 0; i < opsCount; ++i) {
                int result = 0;
                int a = 0;
                int b = 0;
                if (0 == strncmp(ops[i].name, command, OPERATION_SIZE)) {
                    command = strchr(command, ' ');
                    command++;
                    if (*command == '%') {
                        a = result;
                    } else {
                        sscanf(command, "%d", &a);
                    }
                    command = strchr(command, ' ');
                    command++;
                    if (*command == '%') {
                        b = result;
                    } else {
                        sscanf(command, "%d", &b);
                    }
                    result = ops[i].calculate(a, b);
                    printf("%d\n", result);
                }
            }
        }
        free(command);
    }
    else
    {
        fprintf(stderr, "Malloc returned NULL\n");
        programStatus = EXIT_FAILURE;
    }

    return programStatus;
}

int main() {
    Operation ADD = {"ADD", add};
    Operation SUB = {"SUB", sub};
    Operation MUL = {"MUL", mul};
    Operation DIV = {"DIV", divi};

    Operation ops[] = {ADD, SUB, MUL, DIV};

    int opsCount = sizeof(ops)/sizeof(*ops);

    return programLoop(ops, opsCount);
}
| improve this answer | |
\$\endgroup\$

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.