Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The following program is supposed to be a (command line) calculator that parses expression following syntax similar to Lisp's S-expressions.

Some examples:

$ echo "(+ 5 5)" | ./a.out

10.000000

$ echo "(+ (- 3 2) (* 9 2))" | ./a.out

19.000000

$ echo "(/ 24 6 2)" | ./a.out

2.000000

$ echo "(/ 24 (/ 6 2))" | ./a.out

8.000000

  • The program is not supposed to deal with error handling (invalid S-expr, division by zero, ...). We assume the input is always valid. (The program is meant as an exercise and is not going to production).

  • Separators can be space, tabs or newline characters.

  • The only operations considered are +, -, *, /

  • The program is supposed to deal with integers and float input.

Here's my headers file:

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

#define NUMBER '0'
#define OPERATOR '+'
#define MAX_NUM_SIZE 100
#define MAX_DEPTH 100


typedef double (*doublefun_t) ();

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


typedef struct args args_t;
struct args {
    double value;
    args_t *next;
};

typedef struct sexpr sexpr_t;
struct sexpr {
    char operation;
    args_t *arguments;
};


sexpr_t sstack[MAX_DEPTH];
 /*
   Initial value is -1 because the stack is empty.
   Will be incremented to 0 by the first opening paren.
 */
int current_level = -1;

double final_result = 0;

int getop(char s[]);
void create_sexpr();
void add_operation(char op);
void add_argument(double a);
void evaluate_sexpr();

And the actual code:

int main(int argc, char *argv[])
{
    int type;
    char s[MAX_NUM_SIZE];

    while ((type = tokenize(s)) != EOF) {
        switch(type) {
        case '(':
            create_sexpr();
            break;
    case OPERATOR:
        add_operation(s[0]);
        break;
    case NUMBER:
        add_argument(atof(s));
        break;
    case ')':
        evaluate_sexpr();
        break;
    default: break; /* Purposfully ignoring error handling */
    }

    if (current_level < 0)
        break;
    }

    printf("%f\n", final_result);

    return 0;
}

/*
  Parses input from stdin.
  returns NUMBERS for numbers or ascii value for any of ( ) + - * /
*/

int tokenize(char s[])
{
    int c;
    static int buf = EOF;

    if (isalnum(buf)) {
        c = buf;
        buf = EOF;
        return c;
    }

    if (buf == EOF || buf == ' ' || buf == '\t') 
        while ((*s = c = getchar()) == ' ' || c == '\t')
            ;
    else 
        *s = c = buf;

    buf = EOF;
    *(s + 1) = '\0';

    if (c == 42 || c == 43 || c == 45 || c == 47)
    return OPERATOR;

    if (!isdigit(c) && c != '.')
        return c;       /* not a number */

    if (isdigit(c))     /* collect integer part */
        while (isdigit(*++s = c = getchar()))
            ;
    if (c == '.')       /* collect fraction part */
        while (isdigit(*++s = c = getchar()))
            ;
    *s++ = '\0';
    buf = c;

    return NUMBER;
}

/*
  Create new sexpr and put it on the sstack.
  increment current_level index
*/
void create_sexpr()
{
    sexpr_t *new = malloc(sizeof(sexpr_t));
    new->arguments = NULL;
    sstack[++current_level] = *new;
}


void add_operation(char op)
{
    sstack[current_level].operation = op;
}

void add_argument(double a)
{
    args_t *new_argument = malloc(sizeof(args_t));
    args_t *args_iterator = sstack[current_level].arguments;

    new_argument->value = a;
    new_argument->next = NULL;

    if (args_iterator == NULL)
    sstack[current_level].arguments = new_argument;

    else {
    while (args_iterator->next != NULL)
        args_iterator = args_iterator->next;

    args_iterator->next=new_argument;
    }
}

void evaluate_sexpr()
{
    char op = sstack[current_level].operation;
    doublefun_t f = NULL;

    /* variable holders used for the accumulation
     */
    double a, b;

    args_t *new_argument = NULL;
    args_t *args_iterator = sstack[current_level].arguments;

    a = args_iterator->value;

    switch(op) {
    case '+':
    f = &add;
    break;
    case '-':
    f = &sub;
    break;
    case '*':
    f = &mul;
    break;
    case '/':
    f = &dvs;
    break;
    }

    while (args_iterator->next) {
    b = args_iterator->next->value;
    a = (*f)(a, b);
    args_iterator = args_iterator->next;
    }

    if (--current_level >= 0) {
    new_argument = malloc(sizeof(args_t));
    new_argument->value = a;
    new_argument->next = NULL;

    if (sstack[current_level].arguments == NULL) {
        sstack[current_level].arguments = new_argument;
    } else {
        args_iterator = sstack[current_level].arguments;

        while (args_iterator->next != NULL)
        args_iterator = args_iterator->next;

        args_iterator->next= new_argument;
    }
    }
    else {
    final_result = a;
    }
}

I would like to know how to improve the readability of the code, how to make it easier for other programmers to pick it up and understand it.

I am notably unsure about my use of function pointers. It's literally the first time I ever use them (although it was pretty trivial).

Of course, any general remark about my coding style would be highly apreciated.

share|improve this question

Know someone who can answer? Share a link to this question via email, Google+, Twitter, or Facebook.

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.