D
dfighter
Flash said:dfighter wrote:
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)...
#define RESET 1
#define SEARCH 0
If, as I suspect, these are two possible values of a single item an enum
would make sense.
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.
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 != '\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.
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.
strncpy(fieldnames,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);
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 = NULL;
switch(type){
case CHAR: v = malloc(sizeof(char)); break;
case INT: v = malloc(sizeof(int)); break;
case DOUBLE: v = 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