patterncMinor
Read CSV, compute some stuff and prepare data to be sent to GNUPlot
Viewed 0 times
computereadcsvdatasentstuffsomegnuplotandprepare
Problem
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.
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
#include
#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");
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
#include
#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");
Solution
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.
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.
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.
Code Snippets
plot *initList2(plot *list2) {
return (list2 = NULL);
}Context
StackExchange Code Review Q#42433, answer score: 4
Revisions (0)
No revisions yet.