Read CSV, compute some stuff and prepare data to be sent to GNUPlot

Question

I would really appreciate some good advice on how to refactor a small app:

It basically reads a CSV (energy/heat values?), computes some stuff, and then prepares some data which should later be sent to GNUplot.

For example, I'm unsure about this:

Is it good practice to send variables into a function as pointers and manipulate them within the function body? Being more familiar with OOP (specifically Ruby), I'm used to being very careful with such things and try to change variables only by sending them to some method and assigning the method's return value to the variable again, e.g. x = sum(x, y) (or with ! methods).

I'm sure there is a lot more to optimize in the code. I would be thankful for some basic hints on how to optimize the code. It doesn't have to be perfect, but it's quite a mess at the moment, and before refactoring it into the "wrong" direction, it would be nice to get some feedback here.

/*
 ============================================================================
 Name        : Energie.c
 Author      : Benjamin Pfammatter
 Version     : 0.1
 Copyright   : Your copyright notice
 Description : Energy in C, Ansi-style
 ============================================================================
 */

#include <stdio.h>
#include <stdlib.h>
#include "Datenstruktur.h"
//#include "logger.h"


int main(void) {

    item *myList = NULL; //Erzeugen einen Pointer vom Type item
    plot *myList2 = NULL; //Erzeugen einen Pointer vom Type plot
    int bar = 0;
    int counter = 0;
    double E_max = 0;
    double E_min = 0;
    double range = 0;
    double width = 0;

    printf("########## Dynamische Liste ##########\n\n");

    printf("-- erzeuge leere Liste\n");
    myList = initList(myList); //initialisiere den neuen Pointer, indem er auf NULL gesetzt wird
    //LOG_PRINT("-- initList(): myList");


    printf("-- pruefe auf leere Liste\n");
    printIsemtyList(myList);
    //LOG_PRINT("-- printIsemptyList(): myList ist leer");


    printf("-- Einlesen von Daten\n");
    myList = einlesensortiert(myList);

    printf("########## Liste vorwaerts ausgeben ##########\n");
    printListForward(myList);
    //LOG_PRINT("++ printListForward()");


    printf("########## Anzahl Elemente: ##########\n");
    counter = countItems(myList);
    printf("++%d\n", counter);

    printf("########## Groesster Wert der Liste ist: ##########\n");
    E_max = getEnergyMax(myList);
    printf("++%lf\n", E_max);

    printf("########## Kleinster Wert der Liste ist: ##########\n");
    E_min = getEnergyMin(myList);
    printf("++%lf\n", E_min);

    printf("########## Der Messbereich betraegt: ##########\n");
    range = effective_range(E_max, E_min);
    printf("++%lf\n", range);
    /*Wenn R¸ckgabe Wert = 0 muss neue Abfrage gemacht werden!!!!!!!*/

    printf("########## Anzahl Balken: ##########\n");
    bar = number_of_bars(counter);
    printf("++%d\n", bar);

    printf("########## Abschnittswert: ##########\n");
    width = section_value(range, bar);
    printf("++%lf\n", width);

    printf("##########  Neue Dynamische Liste ##########\n\n");

    printf("-- erzeuge leere Liste\n");
    myList2 = initList2(myList2);

    printf("-- pruefe auf leere Liste\n");
    printisemptyList2(myList2);

    printf("-- sortiere Liste\n");
    myList2 = sortierenfuergnu(myList2, myList, width, E_max, E_min, counter,
            bar);

    printf("-- Liste ausgeben\n");
    printListForward2(myList2);

    //printf("++test for schleife\n");
    //probeforsch( E_min, width, E_max);
    //printf("%lf\n",i);


    return EXIT_SUCCESS;
}

Other file:

/*
 * Datenstruktur.c
 *
 *  Created on: 30.12.2013
 *      Author: Captain Morgan
 */

#include <stdio.h>
#include <stdlib.h>
#include <math.h>
#define MAX 20

/*--------------------------------------------------*/
struct daten {
    char date[MAX];
    char time[MAX];
    double spannung;
    double strom;
    double wirkfaktor;
    double energie;
    struct daten *next;
};
/*--------------------------------------------------*/

typedef struct daten item;

/*--------------------------------------------------*/

item *initList(item *list) {

    return (list = NULL);
}

/*--------------------------------------------------*/

int isemptyList(item *list) {
    return (list == NULL);
}

/*--------------------------------------------------*/

void printIsemtyList(item *list) {
    item *iter_List = list;
    if (isemptyList(iter_List))
        printf("++ Liste ist leer\n");
    else
        printf("++ Liste ist nicht leer\n");
}

/*--------------------------------------------------*/

//FILE dateiOpen(void) {
//  FILE *datei;
//
//  datei = fopen("Maschine_02.csv","r");
//  if (NULL == datei) {
//      printf("Konnte Datei nicht ˆffnen!\n");
//      return (*datei);
//  }
//  return (*datei);
//}


/*--------------------------------------------------
 Sortiert einlesen und Berechnen der Energie
 Sortiert nach der Energie. Von klein nach gross.
 --------------------------------------------------*/
item *einlesensortiert(item *list) {

    item *tmp;
    FILE *datei;
    item *iter_List = list;

    datei = fopen("Maschine_01.csv", "r");
    if (NULL == datei) {
        printf("Konnte Datei nicht ˆffnen!\n");
        return (0);
    }

    while (!feof(datei)) {

        tmp = (item *) malloc(sizeof(item));

        fscanf(datei, "%s \t %s \t %lf \t %lf \t %lf\n", tmp->date, tmp->time,
                &tmp->spannung, &tmp->strom, &tmp->wirkfaktor);

        tmp->energie = ((sqrt(3)) * (tmp->spannung) * (tmp->strom) * cos(
                (tmp->wirkfaktor))) * 1; //1 f¸r Sek.
        // P(Leistung) = (Wurzel3) * Strom * Spannung / W (Energie) P * t (in sek)
        tmp->next = NULL;

        if (isemptyList(iter_List) || tmp->energie < iter_List->energie) {
            tmp->next = iter_List;
            list = tmp;
        } else {
            while (iter_List->next != NULL && tmp->energie
                    > iter_List->next->energie)
                iter_List = iter_List->next;
            tmp->next = iter_List->next;
            iter_List->next = tmp;
        }

        iter_List = list;
    }
    return (list);
}

/*--------------------------------------------------*/

void printListForward(item *list) {
    item *iter_List = list;

    if (iter_List) {
        printf("++ %s\t", iter_List->date);
        printf("++ %s\t", iter_List->time);
        printf("++ %lf\t", iter_List->spannung);
        printf("++ %lf\t", iter_List->strom);
        printf("++ %lf\t", iter_List->wirkfaktor);
        printf("++ %lf\n", iter_List->energie);

        printListForward(iter_List->next);
    }
}

/*--------------------------------------------------*/

int countItems(item *list) {
    item *iter_List = list;
    int counter = 0;

    while (iter_List) {
        counter++;
        iter_List = iter_List->next;
    }

    return (counter);
}

/*--------------------------------------------------
 Achtung funktioniert nur mit positiven Werten!!!
 --------------------------------------------------*/
double getEnergyMax(item *list) {
    item *iter_List = list;
    double tmp = 0;

    while (iter_List != NULL) {
        if (iter_List->energie > tmp) {
            tmp = iter_List->energie;

        }
        iter_List = iter_List->next;
    }

    return (tmp);
}

/*--------------------------------------------------*/

double getEnergyMin(item *list) {
    item *iter_List = list;
    double tmp = iter_List->energie;

    while (iter_List != NULL) {
        if (iter_List->energie < tmp) {
            tmp = iter_List->energie;

        }
        iter_List = iter_List->next;
    }

    return (tmp);
}

/*--------------------------------------------------*/
double effective_range(double E_max, double E_min) {
    double range = 0;

    range = E_max - E_min;

    if (range == 0) //Wenn die Differenz gleich 0 ist, wird 0 zur¸ckgegeben. Mit Kevin anschauen welchen Wert er will!!
        return 0;

    else
        return (range);
}
/*--------------------------------------------------*/

int number_of_bars(int counter) {

    if (counter < 5)
        return (0);

    else if (counter <= 10)
        return (2);

    else if (counter <= 100)
        return (5);

    else
        return (10);

}
/*--------------------------------------------------*/

double section_value(double range, int bar) {
    double width = 0;

    width = range / (bar);

    return (width);
}

/*--------------------------------------------------*/

struct gnuplot {
    int yAchse;
    double xAchse;
    double prozent;
    struct gnuplot *next;
};

/*--------------------------------------------------*/

typedef struct gnuplot plot;

/*--------------------------------------------------*/

plot *initList2(plot *list2) {
    return (list2 = NULL);

}

/*--------------------------------------------------*/

int isemptyList2(plot *list2) {
    return (list2 == NULL);
}

/*--------------------------------------------------*/

void printisemptyList2(plot *list2) {
    plot *iter_List2 = list2;
    if (isemptyList2(iter_List2))
        printf("++Liste ist leer\n");
    else
        printf("++Liste ist nicht leer\n");

}

/*--------------------------------------------------*/

plot *sortierenfuergnu(plot *list2, item *list, double width, double E_max,
        double E_min, int counter, int bar) {

    item *iter_List = list;
    plot *iter_List2 = list2;
    plot *tmp2;
    double i;
    double b = E_min + width;
    int counter2;
    int z = 0;

    while (z < bar) {

        for (i = b; i < E_max; i += width) {
            z++;
            tmp2 = (plot *) malloc(sizeof(plot));
            tmp2->xAchse = i;
            counter2 = 0;

            while (i >= iter_List->energie) {
                counter2++;
                //printf("%lf\t",i);
                //printf("%lf\n",iter_List->energie);
                iter_List = iter_List->next;
                //printf("\t\t%lf\n",iter_List->energie);

            }

            tmp2->yAchse = counter2;
            tmp2->prozent = (100 / counter) * counter2;
            tmp2->next = NULL;

            if (isemptyList2(iter_List2))
                list2 = tmp2;

            else {
                while (iter_List2->next != NULL)
                    iter_List2 = iter_List2->next;
                iter_List2->next = tmp2;
            }
            iter_List2 = list2;

        }

    }
    return (list2);
}

/*--------------------------------------------------*/

void printListForward2(plot *list2) {
    plot*iter_List2 = list2;

    if (iter_List2) {
        printf("++ %d\t", iter_List2->yAchse);
        printf("++ %lf\t", iter_List2->xAchse);
        printf("++ %.2lf%%\n", iter_List2->prozent);

        printListForward2(iter_List2->next);
    }
}

/*--------------------------------------------------*/

double probeforsch(double E_min, double width, double E_max) {

    double i;
    double b = E_min + width;

    for (i = b; i < E_max; i += width) {
        printf("%lf\n", i);
    }

    return (i);

}

Code can also be downloaded from here.

Answer 1

I've seen much worse C code. For the most part I think your code is reasonably well organized and your approach of manipulating data via pointers into functions is standard C practice when dealing with large data structures.

C is not OO and you can only go so far before you re-invent C++.

You do a good job of having a single function do a single thing. Where things can go wrong in C code is when a function has side effects that aren't apparent from either the function name or the calling arguments.

This is a potential memory leak.

 plot *initList2(plot *list2) {
     return (list2 = NULL);

 }

Assigning a pointer to NULL means that you are leaking whatever memory that pointer originally referenced. Generally you assign NULL at pointer creation, using a subroutine like this is just asking for trouble.

You have some helper functions that would more typically be written as C macros and you use numeric constants. All of these should be defined in an include file and/or put into an array/struct that you pass into the function. Best practice in C is for functions to have EVERYTHING they need passed in as arguements. In large complex codes, there is often an "environment" or context struct passed in as the first arguement that includes all the global variables required.

All in all, this code is headed in the right direction.

Answer 2

I think the code is pretty good.

I'm not sure how to "refactor" (i.e. restructure) it. Does it already do everything it's supposed to, or do you want to extend it in some way? I usually don't restructure something unless I need to change it and the current structure seems fine: the step-by-step code in main is straightforward so it seems that the functions provided in Datenstruktur.c are adequate.

Therefore I'll review the details (not the structure) of Datenstruktur.c

---

It's conventional to combine the typedef with the struct:

typedef struct daten {
    char date[MAX];
    char time[MAX];
    double spannung;
    double strom;
    double wirkfaktor;
    double energie;
    struct daten *next;
} item;

---

Invoking initList is less readable than simply initializing the pointer to NULL.

---

There's a curious mix of naming conventions:

• using_underscores like number_of_bars
• alllowercase like einlesensortiert
• camelCase like printListForward
• also iter_List

It would look better if you sticked with one. If using camelCase, the 'E' in "Empty" should be capitalized e.g. in isemptyList and printIsemtyList. And there are spelling mistakes like missing 'p' in printIsemtyList.

---

fscanf risks overrunning the input buffer if the data (e.g. a string) in the input CSV file is longer than MAX:

fscanf(datei, "%s \t %s \t %lf \t %lf \t %lf\n", tmp->date, tmp->time,
    &tmp->spannung, &tmp->strom, &tmp->wirkfaktor);

---

It's strange the use recursion in the printListForward function. It would be more normal to use a for loop:

for (item *iter_List = list; iter_List; iter_List = iter_List->next)
{ ...; }

Similarly countItems and similar methods could use a for loop instead of a while loop.

---

getEnergyMax will return 0 if the list is empty, but getEnergyMin might crash.

---

I'm not sure what the point of the if (range == 0) statement is in the effective_range function.

---

initList2 is a pretty bad name to distinguish it from initList. It would be better to use the name of the type in the function name, for example:

• initItemList and initPlotList (sounds English-like)
• item_init_list and plot_init_list (puts the name of the type at the beginning)

---

You leak all the memory you allocate (there are malloc statements but no free statements). IMO that's OK for programs which run once and then exit (memory is reclaimed by the O/S).

You also don't close the CSV file which you opened using fopen.

---

You didn't post Datenstruktur.h and I don't know what's in it. Normally I'd expect to see the typedef of the structs and pointers in the header file.

---

In his answer, @fred said, "C is not OO and you can only go so far before you re-invent C++."

If you want to make it more OO-like, consider the fact that a C++ class is:

• A data structure (i.e. member data instances)
• Associated methods which manipulate that data

The data which you have in this program are:

• daten/item
• gnuplot/plot
• Lists (of item and plot nodes)
• The CSV file (represented by FILE *datei)

At a minimum you could split Datenstruktur.c into two separate modules (where by "module" I mean "a header file and a C file"):

• One which defines item and functions related to item (including reading from the CSV file)
• One which defines plot and functions related to plot (including creating plots from items)

Because C doesn't support polymorphism nor templates, I'd guess it would be overkill (too much extra, difficult code) to try to create a generic list module (a module to work with lists of an arbitrary type, including plots and items). These are a simple type of list (a "singly-linked" list consisting of one forward pointer) which a C programmer should be able to do easily (little or no complication that might need to be hidden inside some library module).

I don't see a convenient way to separate the CSV from the item data: so little opportunity there for separate modules, beyond the built-in FILE* fopen module (family of functions) already built-in to the C run-time library.

So as I said earlier I see little opportunity for refactoring.

Is it good practice to send variables into a function as pointers and manipulate them within the function body?

I don't think the code is doing that anywhere?

But yes, that is normal practice. For example, instead of ...

item *einlesensortiert(item *list) { ... }

... it would also be idomatic to have ...

void einlesensortiert(item** list) { ... }

... where the caller passes a pointer to the list pointer, and the function uses that the change the contents of the caller's list pointer.

C supports the const keyword, which can be used to specify that the thing which is passed by pointer mustn't be altered by the callee.