3
\$\begingroup\$

A few days ago I started writing my own compiler-like program which has the goal to compile plain text into executable python code. After I let my first version review here, I made huge changes to the overall code and added the possibility of adding rectangles. I try to add all the suggested improvements as best as I could, please do not feel offended, if I missed one of your suggestions.

My final goal is to write a program which compiles text into executable python code, which then will display a 3D-Diagram containing all geometric shapes when executed.

At the moment, the user has to provide a text similar to this:

(1.2,45,6)
(7,8,5)
(10,77,88)
(99999,1,1)
R((1,1,1),(2,2,2),(1,3,4))
(5,7,6)
(1,2,3)
(4,5,6)
R((2,3,4),(9,9,9),(3,4,5))
  P(2,1,1)

where R(...) symbolizes rectangles and P(...) is an acronym for a profane point. I added the formatting mistake of the last line, purposely, to show that the compiler does not care about spaces. This plain text will be compiled to a python-code similar (depending on the exact preferences of the code) like this.

points =  [
(1.2,45,6),
(7,8,5),
(10,77,88),
(99999,1,1),
]
points.extend([
(5,7,6),
(1,2,3),
(4,5,6),
(2,1,1),
])
rectangels =  [
((1,1,1),(2,2,2),(1,3,4)),
((2,3,4),(9,9,9),(3,4,5)),
]
print(points)
print(rectangels)

For the sake of me, you and all humankind I quickly realized that rewriting the code into a state machine is the proper way to go after suggested by @Edward.

So how could I further improve the following code?

#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <errno.h>
#include <float.h>
#include <ctype.h>

#define SPACE_FOR_POINTS 4
#define SPACE_FOR_RECTANGLES 4
/*For some reason, maximum size of any double represetend as a string:
 StackOverflow: What is the maximum length in chars needed to represent any double value?
 */
#define MAX_DIGITS_DOUBLE 3 + DBL_MANT_DIG - DBL_MIN_EXP
#define MAX_DIGITS_POINTS 5 + 3*MAX_DIGITS_DOUBLE
#define MAX_DIGITS_RECT 17+9*MAX_DIGITS_DOUBLE

void printPointList(char appened, char** points, int index);
void printRectangleList(char appened, char** rectangles, int index);

int main(int argc, char * argv[]) {
    FILE* fp = fopen(argv[1], "r");
    if(fp == NULL)return errno;

    //Do parsing

    char** points; //all future points will be stored here
    points = malloc(SPACE_FOR_POINTS * sizeof(char*)); //for the moment, make space for SPACE_FOR_POINTS
    if(points != NULL){
        for(int i = 0; i < SPACE_FOR_POINTS; i++){
            /*
             This is a point to be stored:
             (double,double,double)\0
             Calloc is better, because we can later break
             creating the list of points/rectangels, when first char is not '(',
             which is not necessarily the case for malloc.
             */
            points[i] = calloc(MAX_DIGITS_POINTS,1);

            if(points[i] == NULL){
                fprintf(stderr, "Could not allocate enough memory to perform compilation.");
                return -1;
            }
        }
    } else {
        fprintf(stderr, "Could not allocate enough memory to perform compilation.");
        return -1;
    }

    char** rectangles; //all future regtangles will be stored here
    rectangles = malloc(SPACE_FOR_RECTANGLES*sizeof(char*)); /*for the moment, make space for
                                                             SPACE_FOR_RECTANGLES */
    if(points != NULL){
        for(int i = 0; i < SPACE_FOR_RECTANGLES; i++){
            /*
             This is a rectangle to be stored:
             ((double,double,double),(double,double,double),(double,double,double))\0

             Calloc is better, because we can later break
             creating the list of points/rectangels, when first char is not '(',
             which is not necessarily the case for malloc.
             */
            rectangles[i] = calloc(MAX_DIGITS_RECT, 1);
            if(rectangles[i] == NULL){
                fprintf(stderr, "Could not allocate enough memory to perform compilation.");
                return -1;
            }
        }
    } else {
        fprintf(stderr, "Could not allocate enough memory to perform compilation.");
        return -1;
    }

    char bool_appended_rectangle = 0; //Was a list already printed?
    char bool_appended_points = 0; //Was a list already printed?
    int numbers_written = 0; //counter
    int points_index = -1;
    int rectangels_index = -1;
    size_t actual_index = 0; //counter

    enum{name, openparen, comma, number, endparen, error} state = endparen;
    enum{point, rectangle} shape = point;

    //State machine
    for (char ch = fgetc(fp); ch != EOF; ch = fgetc(fp)) {
        if(isspace(ch)){
            continue;
        }
        switch(state){
        case name:
            if(ch == '('){
                state = openparen;
                switch (shape) {
                    case rectangle:
                        rectangles[rectangels_index][actual_index++] = ch;
                        break;
                    case point:
                        points[points_index][actual_index++] = ch;
                        break;
                    default:
                        break;
                }
            } else {
                state = error;
            }
            break;
        case openparen:
            if(ch == '('){
                switch (shape) {
                    case rectangle:
                        rectangles[rectangels_index][actual_index++] = ch;
                        state = openparen;
                        break;
                    case point:
                        state = error;
                        break;
                    default:
                        break;
                }
            }
            if(isdigit(ch)){
                numbers_written++;
                switch (shape) {
                    case rectangle:
                        rectangles[rectangels_index][actual_index++] = ch;
                        state = number;
                        break;
                    case point:
                        points[points_index][actual_index++] = ch;
                        state = number;
                        break;
                    default:
                        break;
                }
            }
            break;
        case number:
                if(isdigit(ch)){
                    switch (shape) {
                        case rectangle:
                            rectangles[rectangels_index][actual_index++] = ch;
                            break;
                        case point:
                            points[points_index][actual_index++] = ch;
                            break;
                        default:
                            break;
                    }
                    break;
                }
                if(ch == ','){
                    switch (shape) {
                        case rectangle:
                            if(numbers_written <= 9){
                                rectangles[rectangels_index][actual_index++] = ch;
                                state = comma;
                            } else {
                                state = error;
                            }
                            break;
                        case point:
                            if(numbers_written <= 3){
                                points[points_index][actual_index++] = ch;
                                state = comma;
                            } else {
                                state = error;
                            }
                        default:
                            break;
                    }
                    break;
                }
                if(ch == ')'){
                        switch (shape) {
                        case rectangle:
                            if(numbers_written%3 == 0){
                                rectangles[rectangels_index][actual_index++] = ch;
                                state = endparen;
                            } else {
                                state = error;
                            }
                            break;
                        case point:
                            //printf("%d", numbers_written);
                            if(numbers_written == 3){
                                state = endparen;
                                points[points_index][actual_index++] = ch;
                            } else {
                                state = error;
                            }
                            break;
                        default:
                            break;
                    }
                    break;
                }
                if(ch == '.'){
                    points[points_index][actual_index++] = ch;
                    break;
                }
                state = error;
                break;
        case endparen:
                switch (state) {
                    case point:
                        points[points_index][actual_index++] = '\0';
                        break;
                    case rectangle:
                        rectangles[rectangels_index][actual_index++] = '\0';
                        break;
                    default:
                        break;
                }
                if(ch == '(' || ch == 'P'){
                    if(++points_index >= SPACE_FOR_POINTS){
                        printPointList(bool_appended_points, points, points_index-1);
                        bool_appended_points = 1;
                        //clear all Strings
                        for(int i = 0; i < points_index; i++){
                            points[i] = calloc(MAX_DIGITS_POINTS, 1);
                        }
                        points_index = 0;
                    }
                }
                if(ch == '('){
                    actual_index = 0;
                    points[points_index][actual_index++] = ch;
                    shape = point;
                    state = openparen;
                    numbers_written = 0;
                    break;
                }
                if(ch == 'P'){
                    actual_index = 0;
                    state = name;
                    shape = point;
                    numbers_written = 0;
                    break;
                }
                if(ch == 'R'){
                    if(++rectangels_index >= SPACE_FOR_RECTANGLES){
                        printRectangleList(bool_appended_rectangle, rectangles, rectangels_index-1);
                        bool_appended_rectangle = 1;
                        //clear all strings
                        for(int i = 0; i < rectangels_index; i++){
                            rectangles[i] = calloc(MAX_DIGITS_RECT, 1);
                        }
                        rectangels_index = 0;
                    }
                    actual_index = 0;
                    state = name;
                    shape = rectangle;
                    numbers_written = 0;
                    break;
                }
                if(ch == ','){
                    if(shape == rectangle){
                        if(numbers_written < 9){
                            rectangles[rectangels_index][actual_index++] = ch;
                            state = comma;
                        } else {
                            state = error;
                        }
                    }
                    break;
                }
                if(ch == ')'){
                    if(shape == rectangle){
                        rectangles[rectangels_index][actual_index++] = ch;
                        rectangles[rectangels_index][actual_index++] = '\0';
                        break;
                    }
                }
                state = error;
                break;
        case comma:
                switch (shape) {
                    case point:
                        if(isdigit(ch)){
                            numbers_written++;
                            state = number;
                            points[points_index][actual_index++] = ch;
                            break;
                        }
                        state = error;
                        break;
                    case rectangle:
                        if(isdigit(ch)){
                            numbers_written++;
                            rectangles[rectangels_index][actual_index++] = ch;
                            state = number;
                            break;
                        }
                        if(ch == '('){
                            rectangles[rectangels_index][actual_index++] = ch;
                            state = openparen;
                            break;
                        }
                        state = error;
                        break;
                    default:
                        break;
                }
                break;
        default:
            fprintf(stderr, "Error: Corrupted File.");
            return -1;
        }
    }

    printPointList(bool_appended_points, points, points_index);
    printRectangleList(bool_appended_rectangle, rectangles, rectangels_index);
    printf("print(points)\n");
    printf("print(rectangels)\n");

    fclose(stdout); //needed?
    fclose(fp);
    return 1;
}
void printPointList(char appended, char** points, int index){
    if(!appended){
        appended = 1;
        printf("points =  [\n");
    } else {
        printf("points.extend(\n");
    }
    for(int i = 0; i <= index; i++){
            printf("%s,\n", points[i]);
    }
    if(!appended){
        printf("]\n");
    } else {
         printf("])\n");
    }
}
void printRectangleList(char appended, char** rectangles, int index){
    if(!appended){
        appended = 1;
        printf("rectangle =  [\n");
    } else {
        printf("rectangle.extend(\n");
    }
    for(int i = 0; i <= index; i++){
        printf("%s,\n", rectangles[i]);
    }
    if(!appended){
        printf("]\n");
    } else {
        printf("])\n");
    }
}
\$\endgroup\$
  • \$\begingroup\$ Maybe I'm mistaken but I think this could be 2 separate questions that would target two very different set of reviewing skills. You have C code that translates text into python code, which will then print shapes? \$\endgroup\$ – IEatBagels Jun 21 at 14:09
  • \$\begingroup\$ You assume correctly. I only care for the C code. \$\endgroup\$ – TVSuchty Jun 21 at 16:37
  • \$\begingroup\$ Or at least not so much about the python code. \$\endgroup\$ – TVSuchty Jun 24 at 10:37
  • \$\begingroup\$ Is there any reason for points.extend at all? \$\endgroup\$ – klutt Jun 24 at 16:25
  • \$\begingroup\$ Sure, writing everything in one list makes it unreadable. So I thought chunking would sustain some readability... \$\endgroup\$ – TVSuchty Jun 24 at 20:52
2
\$\begingroup\$

Choice of language

I understand you probably want to do this in C, but I felt it was worth mentioning that Python is a much more suitable language for this task.

malloc call

You're using points = malloc(SPACE_FOR_POINTS * sizeof(char*)). I'll advice you to instead use points = malloc(SPACE_FOR_POINTS * sizeof(*points)) to prevent problems if you change type of points in the future.

Comparing pointers to NULL

Not a big deal, but checking if a pointer is valid with if(ptr) or not valid with if(!ptr) is so common in C that it adds almost no readability.

Comments

You're using some unmotivated comments. Like when you are wasting 7(!) lines just to explain why you are using calloc instead of malloc.

return instead of exit

Using return instead of exit works as long as you're in the main function. So if you have a return statement that should quit the program, then instead use exit(EXIT_FAILURE) so that it does not matter if you refactor it into a function.

Massive main function

The thing that strikes me most is that you have a massive main function with a really huge switch statement. The first thing I would do put the whole switch statement into a function with the signature enum state DFA(enum state state, char ch) and then have a main loop like this:

for (char ch = fgetc(fp); ch != EOF; ch = fgetc(fp)) {
    state = DFA(state, ch);
    if(state == error) { /* Handle error*/ }
}

But this would not be enough, since your states are much more complicated than that. The actual state of your machine is a combination of state and shape. So I would rename the current state. I will choose category but I'm sure you can come up with something better. Then I would do this:

struct state {
    enum category;
    enum shape;
};

struct state DFA(struct state state, char ch);

Of course, you would also need to pass rectangles, points, rectangles_index, points_index and actual_index somehow. I would use a struct for that:

struct data {
    char **rectangles;
    char **points;
    size_t rectangle_index;
    size_t points_index;
    size_t actual_index;
};

struct state DFA(struct state state, char ch, struct data * data);

Another alternative would be to just declare them as globals. Don't think that would be a too terrible idea in your project.

Massive switch

I would consider rewriting the whole automata. I would have rewritten it a bit like this:

switch(state) {
case name:
    nameFunction(ch, data); break;
case openparen:
    openparenFunction(ch, data); break;
\$\endgroup\$
  • \$\begingroup\$ Hey thank you for your suggestion; I also considered reducing the size of main but was not sure about the concrete design-choice. My goal is indeed writing this in C, so I learn C. In Python, I would have considered this way to easy, since there are fundamentals I do not have to care about. I think I will create more complex compiler-exercises in python in the future and hope for your review :) \$\endgroup\$ – TVSuchty Jun 24 at 10:36
  • \$\begingroup\$ Why would you not pass pointers into the DFA and return void? What does state mean? \$\endgroup\$ – TVSuchty Jun 27 at 9:04
  • \$\begingroup\$ Okay, while refactoring I figured out why. Because you have to dereference tons of time! \$\endgroup\$ – TVSuchty Jun 27 at 9:30

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.