Advice / Evaluation

[elderK]

Hey people,

Was just wondering if anyone could advise me on how I can improve the code in the posted archive.

krdx = my radix tree implementation.
kavl = my avl tree implementation.
kdt = a little data table implementation, basically a bad vector implementation.
inject = a little tool to inject a single file into a generated FAT12 image, with some bootloader image.

Inject is probably the ugliest of them all. I'm not sure how to clean it up, if anyone has a clue I'd be
greatful. AVL, Radix variable names are very, very short.

Basically, I've been experimenting with various things (Naming conventions, Indentation styles,
C Dialect (c89 vs c99)).

SO! Basically, ANY feedback is appreciated. Suggestions on how I can improve any of it, so that I may
grow as a programmer would be AWESOME.

~K

PS: And, should anyone find use for any of this code, feel free to use it. I'd like credit for my code, though.

[Solar]

All in all very structured and clean code, with style being applied consistently. Bravo!


More comments! State what your functions do in your headers, what the parameters are, what is returned, and how errors are handled. Add one-line summaries of what your code blocks do, to allow quicker scanning of your code. krdx_class_t can be EXTERN, INTERN, MARKED, or JUNK - what is "marked", what is "junk"? Yes, I could read your code to find out, or I could be given a comment that makes reading your code easier. kmmwpr.h is a good example.

File and variable names are a bit too terse for my taste, but perhaps you're aiming for C89 compatibility re identifier length... if not, I feel that expressiveness is better than terseness. Virtually no compiler today is so limited in identifier length.

Makefile convention is to write variables in uppercase, i.e. CFLAGS instead of CFLAGS. Note that many variables (like RM, CC etc.) are already preconfigured by make.

Function declarations are not required to state "extern" explicitly.

Always put { } around if / else / while blocks, even if they're one-liners. This will save you some headaches with wrong nestings and blocks that get additional lines later on.

Personally I prefer spaces added after ( and before ) as code seems a bit too cramped otherwise, but that's a matter of taste.

Use const wherever (!) possible. It helps the compiler, and it helps the reader.

Even -Wall -Wextra -pedantic don't catch everything. Check the GCC documentation, there's lots of more. I use "-Wshadow -Wpointer-arith -Wcast-align -Wwrite-strings -Wmissing-prototypes -Wmissing-declarations -Wredundant-decls -Wnested-externs -Winline -Wno-long-long -Wconversion -Wstrict-prototypes" in addition.

[JamesM]

Hi elderK,

I only have time to review one of those tools, really, so I'll pick inject as you said it needed the most work.

inj.h
Looks good - maybe a few comments though, to describe the pre/post conditions of functions, along with what the arguments are.

inj_dump.c
Indentation is particularly good - I'm a little worried however by an argument called "const char *Out" - if it's an 'out' parameter surely it's going to be modified and shouldn't be constant? - Oh, it's a filename. That needs a comment.

malloc(0x200), fread(Buffer, 0x200, 1, Generic) - The 0x200 is a little magic. It's always good to have a #define for these kind of things. Firstly it makes the constant modifiable. Secondly it makes it more descriptive. Thirdly whenever you do constant arithmetic with it it makes that more readable too (consider "0x100" vs "INITIAL_CHUNK_LENGTH / sizeof(uint16_t)".

The goto's are, uh, well I don't like them . Not only that but notice this piece of code:

Code: Select all

if (!(Buffer = malloc(0x200)))
    goto Error;

Error:
    free(Buffer);
    fclose(Generic);
    return 0;

If the buffer failed to allocate, that free is going to do Horrible Things. Sorry - I can just never ever see a reason for using goto. I'm a bit old-school that way.

inj_inject.c, inj_main.c
Same as inj_dump.c, really. goto's and magic numbers all over the place, without comments. Apart from that the code looks awesome.

Cheers!

James

[Solar]

So much for code review, now I'm trying to compile the stuff.

It's .PHONY:, note the uppercase. As a result, "clean" is not recognized as phony target. ("make all && touch clean && make clean" gives "make: `clean' is up to date.")

inj_dump.c expects <inj.h> (i.e., system include), but the current directory is not included in the search path by the Makefile. This would result in /usr/include/inj.h (or whatever) being preferred over the include in the working directory; I'd rate this as bug. (Add -I. to CFLAGS, or use "inj.h" outright.)

Adding my warning flags from last post adds a few warnings, but surprisingly few. Good style!

Makefiles in adt;0/... are named in lowercase; canon is to call them "Makefile" (uppercase "M"). It's also not consistent with the one in inject;0.


Bottom line, I have seen far worse. I'd give this a B+.


Edit: YUCK! I missed goto's? Sorry but that's an instant B-, there...

[DeletedAccount]

Hi ,

Pretty good , and appears to be portable as well , well done ,I picked avl , coz i have done it lots of times .

1) More comments required , at least a one liner before each function and a comment at the start of each file describing purpose , date etc

2) Although a matter of taste ,

I would prefer something like ,
typedef stuct nodeptr* NODEPTR ; , since i am less confused this way
than using nodeptr ** all the time .
3)Although a popular Cism , better avoid statements like
if(p)
{
do_something();
}
and it is better to write
if( p != 0)
{
do_something;
}
I am also guilty of commiting the same mistake

4) I would also prefer more verbose macros and enums

Finally I am a great moron , You may not take my advice seriously

Regards
Saint Luficer

[JamesM]

3)Although a popular Cism , better avoid statements like
if(p)
{
do_something();
}
and it is better to write
if( p != 0)
{
do_something;
}
I am also guilty of commiting the same mistake

Actually, I'd just stick to the advice of "Don't use boolean operators to perform arithmetic operations". So if you're checking for a true/false value (which happens to be encoded as !0, 0), then use if (var). If you're checking whether an arithmetic result came back as zero (which in this case would be abnormal), use if (var != 0).

[DeletedAccount]

Goto's are not really bad , sometimes when there are a lot of nested loops or nested conditionals goto provides an easy way .
(Do not ask about cyclomatic complexity now ) .goto should be used with caution . "GOTO" is not really bad . It depends upon the programmer

Truly Evil
Saint Lucifer

[DeletedAccount]

I would usually have something like

Code: Select all

enum { FALSE , TRUE } ; 

in one of the header files and do something like

Code: Select all

if ( p != FALSE )
{
  do_something();

} 

than if(var){ }
etc , as pointed out earlier .. my opinions doesnt matter much ....

[elderK]

Thanks for the input guys, I was hoping I'd get a reply tonight.

As for the gotos, aye. I agree.
As for INJECT, the source in the archive was a rush translation from a different indentation style.
While the gotos remain mostly identical, I did rush-remove a bunch of them without serious testing.

JamesM, you are perfectly correct - freeing the buffer would result in certain DooM!
I agree with the constants, Magic numbers are evil. So, I'm glad to get that response.

Solar, I too like the space between ( and ). It makes macros look quite painful though, no?

As for Gotos, I have a love-hate relationship with them. Sometimes, used well they can save a lot
of time and duplication. Used badly, it will make code absolutely horrible. I don't consider using
Gotos a horrible thing, if it is done for a decent reason. INJECT in my opinion, is still incomplete and
requires some serious cleaning up, restructuring. If I can find a nice way to handle errors without
GOTOs and without typing duplicating most of the error handling code, Ill abandon Gotos for good.

Solar, again, Thanks about the Makefiles. Ive seen lowercase used in a lot of situations and so, which
I use (M or m) usually rests upon however Im feeling at the time. But, from now on Ill make sure to have
it capitalized. Also, Ill do some closer reading of the GNU Make manual, see precisely which variables
are defined (RM, etc).

Thanks again guys! God willing, Ill have something pretty spiffy to show you all soon.
(At least from a conceptual perspective, that is; not all flashy gui-ish stuff).

~K
PS: Out of curiosity, how do you prefer variable naming? Lately Ive been fluctuating (testing) between
super short names, standard C-style names (bla_t, do_something()), Windowish names (DoSomethingCrazy()),
Long, descriptive names (which I like for the most part, when used Standard-C style).

Main problem is I like things to be consistent within a single component / module. If a few variables have long
names, I feel this insane urge to make ALL variables have long names. Im slowly coming around to the idea of
"use simple names for easily understandable, transient variables". Like, counters in a loop which does something
obvious.

Also, out of curiosity, how do you feel about the definition of multiple variables? Do you split the definitions
across multiple lines as in:

Code: Select all

    uint16_t v1;
    uint16_t v2;
    uint16_t v3;

Do you group them?

Code: Select all

    uint16_t v1, v2, v3;

If declaring pointers, do you group the pointers at a certain side of the definitions?
Also, if you are initializing the variables - do you still allow the definitions (or is it declarations?) on a single line,
should the initializer be small enough (even if a call or macro)?.

Cheers!

[Solar]

Solar, I too like the space between ( and ). It makes macros look quite painful though, no?

Not sure what you mean, here.

But, from now on Ill make sure to have
it [Makefile] capitalized.

Other than being canon, it makes the Makefile show up at the top of a "ls".

Also, Ill do some closer reading of the GNU Make manual, see precisely which variables
are defined (RM, etc).

Chapter 10.3, Implicit Variables.

PS: Out of curiosity, how do you prefer variable naming? Lately Ive been fluctuating (testing) between
super short names, standard C-style names (bla_t, do_something()), Windowish names (DoSomethingCrazy()),
Long, descriptive names (which I like for the most part, when used Standard-C style).

FOO_BAR( ... ) for macros.

foo_bar( ... ) for functions.

fooBar() for methods.

FooBar, in my book, would be a class name.

Oh, and stay away from Hungarian Notation. If you get too far away from a variable's declaration to remember its type, you have a scope problem - and I prefer to read code without requiring a code book to decipher the HN warts.