Re: [LONG] Re: Request for comment on my tiny learning project: dfighterdb (mytree)

Flash Gordon wrote:
dfighter wrote:

<snip>

Thank you Richard for you observations. Since you told me it's ok to post larger codebases too, I'm posting it now.

In addition to Keith's comments (which I agree with)...

<snip>

#define RESET 1
#define SEARCH 0

If, as I suspect, these are two possible values of a single item an enum would make sense.

<snip>

static char *ui_menu_main_szitem[] = {
"Table",
"Quit",
NULL
};

Adding a const qualifier on this and the other similar arrays I've snipped might help trap errors at compile time.

<snip>

static char *header = "dfighterdb UI";
static char *separator = "-------------";

Again, a const might be useful.

static void ui_trim(char *s){
int i = 0;
for(i = 0; s[i] != '\0'; i++)

You are setting i to 0 on two consecutive lines. Whilst legal, it does look wrong to me and is likely (I think) to make people stop and wonder.

<snip>

static int ui_getmenuchoice(void){
char line[MAXINPUTLINELENGTH];
fgets(line,MAXINPUTLINELENGTH,stdin);

return line[0];

This is rather odd. If you want to throw away all characters after the first on the line it won't work if the line is too long (someone accidentally put something down on the keyboard). It would be better to make sure you have read up to EOF or a newline in my opinion.

}

static int ui_getINT(int *error){
char line[MAXINPUTLINELENGTH];
int value = 0;
fgets(line,MAXINPUTLINELENGTH,stdin);
if(!isdigit(line[0]))
*error = -1;
else value = (int)atof(line);

atof is not a good function to use for several reasons. How will you do error checking with it? Why use a function to decode a floating point number when what you want is an integer value? strtol and checking the result is in range of an int (make value a long) and checking for errors would be better. Even without checking for errors (just range) it would still be better! Also the cast is not needed and does not help.

return value;

}

static double ui_getDOUBLE(int *error){
char line[MAXINPUTLINELENGTH];
double value = 0.0;
fgets(line,MAXINPUTLINELENGTH,stdin);

if(!isdigit(line[0]) && line[0] != '+' && line[0] != '-')
*error = -1;
else value = atof(line);

strtod would be better. You could then check for errors.

<snip>

strncpy(fieldnames[i],name,MAXFIELDNAMELENGTH);

strncpy is rarely the right function for the job. It can leave you with an array of chars which is NOT terminated by a '\0', so if you then treat it as a string, which you do below, it will then go reading off in to the wild blue yonder!

}

for(i = 0; i < numfields; i++){
error = 0;
do{
printf("type of \"%s\":",fieldnames[i]);

<snip>

void ui_tablemenu_query_new(void){
unsigned char numfields = table_numfields();
int i = 0;
void **v = NULL;
unsigned char type = 0;
char *fieldname = NULL;
int error = 0;
char cval = 0;
int ival = 0;
double dval = 0;

v = malloc(numfields*sizeof(void*));

You could use sizeof *v instead of sizeof(void*). It saves having to get the type right or check further up that it is right.

if(v == NULL){
printf("ERROR: not enough memory\n");
return;
}

for(i = 0; i < numfields; i++){
type = table_fieldtype(i);
v[i] = NULL;

switch(type){
case CHAR: v[i] = malloc(sizeof(char)); break;
case INT: v[i] = malloc(sizeof(int)); break;
case DOUBLE: v[i] = malloc(sizeof(double)); break;
}

This is going to be very memory (and probably time) inefficient on most systems. You might want to investigate using a union of char, int & double to avoid this extra block of mallocs. If you also need pointers to strings (which I think you do) then add a char* to the union as well.

<snip>


Well, that's as far as I've read. However, it does give you a few more things to look.
Thank you Flash Gordon, you did raise some good points too. I'm going to consider them and make changes accordingly.
Setting a variable to 0 on to consecutive lines was due to my habit of declaring and initializing the variables at the same time, it may be a bit confusing, but I think that I will not change that part :)

dfighter
.

Relevant Pages