[H-GEN] C code.

The fuzzy one s335810 at cello.it.uq.edu.au
Mon Oct 27 05:13:52 EST 1997


On Mon, 27 Oct 1997, Andrae Muys wrote:

Muhahahaha.  Let the games... BEGIN!

;Well after that last C post I thought I'd solicit comments on my own
;coding style (constructive please, flames < /dev/random > /dev/null).
;Anyone who's interested can have a look at
;http://www.uq.edu.au/~cmamuys/source_downloads/hash.[ch] constructive
;critism appreciated.

First Pass:

"It is a convention and courtesy to keep to 80 columns per line"
(Official quote for insertion into second year Ada assignments, it must be
good)

Second Pass:

	A little more serious though, I find the code difficult to read
due to bunching on the right hand side.  There is a serious lack of
vertical whitespace, making it difficult to visually isolate algorithm
steps.  It is also difficult to isolate code flow, the steps are much more
chunky then I am used to.  I suppose that could grow on a person, however.
I just hope you never have to go beyond two levels of indentation, or it
will be hell to read

Third pass:  (actual code analysis)

	Nice object encapsulation.

	IntHashErrorStrings is oddly formed.  It is not ordered in any
obvious way.  <shrug>  I'd probably put the exist/nexist into another
array for clarity.  They appear to be dealing with different things than
error/ok.

	Hmm, there's a corresponding enumerated type, there's no reason to
keep them seperate.  It would be better to have them both in the header
file and visable then trying to hide what is essentially a reiteration of
the enumerated type data anyway...

	Oh.  You've got it in the header file as an extern char.  That
comes close to making sense, but the only reason you would do it is to
prevent having to rewrite header files... which you'd probably have to do
for the enumerated type anyway if a change is required.  It's a fine
point, but for readability's sake I would be putting them all together
anyway.  It took me almost a minute to work out those relations over two
files.

	<counfused by uint>  Not defined anyhere must be standard C that I
haven't seen before.

	You've kept the type of the actual has table private by only
renaming the type in the header file.  That's good.  Saves rewriting spec
just because of a change in algorithm.

	Ahh, I should have read the comments in the header file before
devling into the C.  The EXIST and NEXIST are success results... So one
function returns ERROR/NEXIST/EXIST, and another returns ERROR/OK.  I
still don't like it though.  I would seperate them into two types of
return value (both integer naturally).  They are not really related, one
being for key elements, and another determining whether or not a table is
successfully killed off.  I can see that you want to avoid straight
boolean because of the common confusion as to when a return value should
be true, and when false, but this amalgamation is... but then I'm picky.

	Back to the C.
	
	A bit ugly having to alloc twice during init, but no real way
around it... not unless you want to got the dirent way and put an array[1]
at the end, then just malloc what you need <cringe>

	Lots of error handling inline with the code... Should'a' used Ada
:)

	Ohh Ooooh, a coded in constant I can abuse!  Should be a #define
if you value your sanity.  #define DEFAULTSIZE 32.  Where is this default
documented anyway?  I can't seem to find it... I would have written

	if (size > 0)
		table->len = size;
	else
		table->len = DEFAULTSIZE;

	myself.  I know the compiler will work it out, but there's no
reason to make it less readable by humans.  Oh, and I don't like ==.  One
of my phobias coming from numerical maths.  I know and you know that your
code is written using unsigned in, but does the next idiot maintainer know
it?  If that uint was changed for an int your code would fail on negative
sizes with what looks like and out-of-memory error I believe.  Just being
picky again :-).

	Are comments only allowed at the tops of functions <Odd look>?

	<notices HASH_REPORT>  That's a somewhat... interesting
definition.  I can accept the fprintf part (even though it only works
with fixed strings), but to undefine it if HASH_DEBUG is off?  That seems
silly.  You obviously didn't define it if HASH_DEBUG is off, so someone
else must have, and they've probably got a good reason for it..?  Oh, now
I see.  You're only undefining it to define it again with comments around
it.  I hope none of those strings have close comment characters in them
<evil grin>.  I'd personally make it another function.  That way it can
accept non-fixed strings, doesn't have to comment the code out, and
doesn't need a complex definition.  It can include an ifdef HASH_DEBUG
around the code within it, and I would probably give the compiler a pragma
to inline it, just to be sure it didn't cost a performance hit on release
day.

	Hmm, I'd personally have the old value returned from
intHashAddEntry.  You'd avoid a *.  *s are bad.  One star is fine.  Two
you start to think, there's got to be a better way.  Any more and you've
got to hold onto your sanity before it balks and runs screaming into the
night.  The problem with pointers is that in C you never know what they
mean.  Are they a pointer, or an array?  Am I looking at a three
dimensional array, or just a couple of layers of indirection.  That annoys
me.

	See, you return it when you do intHashGetEntry.  My case rests.

	<shrug>  Check for table==NULL in intHashDelEntry.  That's fine,
but I'm in the habit of declaring that kind of thing simple bad data.
Maybe put an ifdef HASHDEBUG around it, but it breeches the preconditions,
and shouldn't be dealt with in the final product at run-time... But that
may just be me.

	ARRRRGGGGHHH!!  I'm hit!!!!!!

	uint intHashDel (IntHashTable **table, void ***data)

	<shakes head to regain balance>  Stay steady now, there -must- be
a reason for it.  It returns ERROR/OK, takes a table which it may modify
(hence **), and ahhh.

	uint intHashDel (IntHashTable **table, void **data[])

	No.  Still one * unaccounted for... ahh ok.  Returns (one *) a
null-terminated array (another *) of void *s.  That's three.  Personally
I'd just leave them to their fate when they destroy the hash table.  Don't
bother returning an array, they'll most likely just free it anyway.  My
definition would be

	uint intHashDel (IntHashTable *table)

	If there's an error, there's no need to tell them twice.

	I would have another function to tell them at any time what the
data of the hash table was.  If they call -that- function, chances are
they want to know.  Oh, and it would just return a pointer to the memory,
not ask them to give me a pointer, and then have me modify it.  Saves an
awful lot of hassle that way.

Conclusion:

	I am dead, aren't I?

You have been enlightened	| The fresh air coming through a window
by the fuzzy one		| reminds me of the outside world.
				| It's something I don't want to
Forsaken I sit in with a radio	| remember right now.
blaring.  When did my fire burn |
so low?				| s335810 at cello.it.uq.edu.au
				|
memory at techie.com (personal)	| It's only now I despise my humanity.

----------------------- HUMBUG General List --------------------------------
echo "unsubscribe general" | mail majordomo at humbug.org.au # To Unsubscribe



More information about the General mailing list