Re: Request for comment on my tiny learning project: dfighterdb (mytree)
- From: dfighter <dfighter@xxxxxxxxxxxx>
- Date: Wed, 22 Apr 2009 15:53:16 +0200
Barry Schwarz wrote:
On Sun, 19 Apr 2009 03:28:25 +0200, dfighter <dfighter@xxxxxxxxxxxx>Hi Barry!
wrote:
Dear everyone!
dfighterdb (I started it as mytree), is a tiny and simple console based "database" application I'm writing as a learning project.
It's purpose is to teach me some generic dynamic data structures, and general design of "larger" projects (Basically a binary tree and a linear list with 256 generic fields).
It's not complete yet, however it is complete enough to present an insight into my "coding style", to show how well (or badly) I can use the language, how portable the code is, general design of the project, etc.
So I'd like to ask you all if you have the time to review it and comment on it.
zipped source code + win32 binary: http://www.freeweb.hu/dfighter/dfighterdb.zip
In tree.c, you have a instances of code of the form
ptr = NULL;
ptr = malloc(...);
-There is no point in assigning a variable one value and in the very
next statement assigning it a new value.
You have many identifiers that start with a double underscore. These
are reserved for the implementation. You shouldn't use them.
In common.c you have code of the form
case CHAR:{
if(*(char*)val1 > *(char*)val2)
retval = 1;
else if(*(char*)val1 < *(char*)val2)
retval = -1;
}break;
-In most of your code I looked at, your closing braces line up with
the keyword to which the opening brace is attached. This makes it
easy to find the end of a block. Here the closing brace is well
indented beyond the case keyword and much too easy to miss, partly
because of the attached break.
-Furthermore, the break is not part of the if or the else or even the
initial if. It should not be on the same line as the brace and
aligned with the initial if since it is at the same "top" level within
the case.
-Your code uses tabs for indentation. If you ever have to post a
single function to Usenet asking for help, the tabs will play havoc
with your message. It is better(tm) to get used to using consistent
spacing for indenting your code.
Member field of treenode_t uses a dynamically allocated array of
void*. Every time you need to reference the object pointed to by one
of the void*, you cast the pointer to the appropriate type. In
addition to the intended effects, casts also tell the compiler it
should ignore certain problems which is almost always undesirable. I
wonder if a union of three pointer types would be easier on the typing
and reading. Something along the lines of typedef union mypointers{
char* mychar;
int* myint;
double* mydouble;
}mypointer_t;
and in treenode_t
mypointer_t *field
Then in tree_dump the body of your switch would look like
case CHAR: printf("%c ",*root->field[i].mychar); break;
case INT: printf("%d ",*root->field[i].myint); break;
case DOUBLE: printf("%lf ",*root->field[i].mydouble); break;
In ui_mainmenu, you print out a line that does not terminate with a
'\n'. On some systems, this line may not appear in a timely fashion.
You should use fflush(stdout);
to insure that it does.
If you are going to include several functions in a single translation
unit (.c file), consider putting them in alphabetical order to make it
easier to find. I recommend that functions larger than one screen
display should be in their own files.
In ui_mainmenu, you call ui_getmenuchoice to decide what to do. You
then test the returned value against EOF. ui_getmenuchoice cannot
possibly return the value EOF. Furthermore, ui_getmenuchoice does not
test the return from fgets for errors.
Why did you decide to use atof in ui_getINT when atoi seems more
appropriate? The cast is unnecessary. I recommend using the strto_
functions in place of atof simply for the better error detection.
Why does ui_getDOUBLE tolerate negative values while ui_getINT does
not?
I understand why ui_trim strips off trailing blanks but why does it
strip off trailing punctuation?
Why does ui_getSTRING refuse to recognize a string that starts with
something other than a letter?
In ui_tablemenu_create, once ui_getINT sets error to 1, it never gets
reset back to 0. You have the same problem when calling ui_getSTRING.
In ui_tablemenu_query_new, shy do you cast the return from
ui_getmenuchoice to unsigned char only to assign the result to cval
which is of type char? And the same problem with error.
First of all thanks for your review.
I'm aware of the double underscore problem now from earlier posts to this thread.
I don't do the indentation myself. I am currently using M$ Visual Studio 2008's editor to edit source code, so it is done automatically, while it is very convenient as you mentioned the tabs might cause problems. I will see if I can change the editor's behavior, or in worst case scenario I will write a small program to change the tabs to spaces.
The rest of your thoughts are valid and good points as well I will consider them and make changes accordingly.
dfighter
.
- Follow-Ups:
- Re: Request for comment on my tiny learning project: dfighterdb (mytree)
- From: Barry Schwarz
- Re: Request for comment on my tiny learning project: dfighterdb (mytree)
- References:
- Request for comment on my tiny learning project: dfighterdb (mytree)
- From: dfighter
- Re: Request for comment on my tiny learning project: dfighterdb (mytree)
- From: Barry Schwarz
- Request for comment on my tiny learning project: dfighterdb (mytree)
- Prev by Date: Re: when does operator promotion happen? Is it expensive?
- Next by Date: Re: when does operator promotion happen? Is it expensive?
- Previous by thread: Re: Request for comment on my tiny learning project: dfighterdb (mytree)
- Next by thread: Re: Request for comment on my tiny learning project: dfighterdb (mytree)
- Index(es):
Relevant Pages
|
