Criticism of the C code, a young programmer!

valsorym · May 5, 2012

Hi guys,

Introduction (to make it clear why I am asking this question).
Exactly one year ago, I thought to throw OS Windows and switch to UNIX OS. A whole summer of 2011 I studied the various UNIX like operating systems, and stopped on FreeBSD OS.

Even last fall, I do not know how to create / read / watch, even plain text files. Through this forum, I somehow mastered this OS, and now, after seven months, I can freely work on FreeBSD.

When I worked on the Windows OS, I was fond of programming, but I used C++ and IDE Borland Builder - bad Windows way!

Now I want to continue to study programming, but only ANSI C (c89, c99, c11). Now I do not use the IDE. My tools: devel/geany or editors/vim, graphics/dia, devel/doxygen and BSD make. That's all what I learned!

In order to begin a detailed study of ANSI C, I decided to write a small program coding of text data. She has a very simple algorithm, but it's enough to make that first step.

My question.
Here's the source code of my program, could you, as the more experienced programmers look at it and give me what some suggestions / comments / criticism / cry.

I am very interested to know: I'm moving in the right direction?
I am interested in your criticism!

Details for the compilation:
Directory structure:

Code:

 ~/yep/
      |__ bin/
      |__ doc/
      |__ src/
      |__ ..
      |__ .

Sources:
C code, my program.
~/yep/src/yep.c (View here!)

Doxyfile
~/yep/src/Doxyfile (View here!)

Makefile
~/yep/src/Makefile

Code:

# Project:          yep
# Date created:     April 22, 2012
# Whom:             valsorym <valsorym.e@gamil.com>

# binary
EXECUTABLE = $(.CURDIR)/../bin/yep

# compiler
CC = cc
CFLAGS = -Wall -ansi -std=c99 
LDFLAGS = -O2

# targets
all: $(EXECUTABLE)
$(EXECUTABLE): yep.o
	$(CC) $(CFLAGS) -o $@ $> $(LDFLAGS) 

yep.o: yep.c
	$(CC) $(CFLAGS) -c $>

.PHONY: all clean run install

clean:
	rm -f *.out *.o *.core

run:
	$(EXECUTABLE)

install:
	cp $(EXECUTABLE) ./bin/

In advance, Thank you all!

plamaiziere · May 5, 2012

doorways said:
Hi guys,
My question.
Here's the source code of my program, could you, as the more experienced programmers look at it and give me what some suggestions / comments / criticism / cry.

I am very interested to know: I'm moving in the right direction?

Of course you are moving in the right direction. You are doing something, go ahead!

Well I've looked very quickly the C code.

First, I'm happy to see that you use spaces. It looks like spaces are very expensive when I look the code made by some beginners at work (but may be I'm too old and I need new glasses...).

Be consistent with spaces, I don't see why "int i=0" has no space but "i < 0" has some?

There are few things I dislike:
In main(), the "if else if else if else if" where you test options is quite hard to read and understand, this is a typical case where you can use "continue / return / goto" to make the code more readable.

There are few useless cast (avoid cast!) :

Code:

hookword = (hookword ? (char*)hookword : (char*)DEFAULT_HOOKWORD);

hookword is a char *, and DEFAULT_HOOKWORD too...

Also the construction above is not very readable, why you want to say is "if hookword is null, put DEFAULT_HOOKWORD". I don't see the point to use the "?" operator here. I prefer

Code:

if (hookword == null)
    hookword = DEFAULT_HOOKWORD;

which is exactly why you want and easily readable.

To finish, I've noticed some "if (i++ < XXX)", I'm not sure if this is and undefined behavior or not (when the post incrementation is done?). I'm not a good C programmer and I'm too lazy to check this, so I would avoid this (doing C is like carrying some TNT, if you are not cautious it will explode IMO).

VoilÃ , this is highly subjective of course. Perfect Code is a bit like the Holly Grail.

you will find some advise for C style in the "man style" manual page of FreeBSD and all the FreeBSD's source code to see how it is made. It's good C and there are a lot of things to learn from.

Regards.

valsorym · May 6, 2012

plamaiziere said:
Of course you are moving in the right direction. You are doing something, go ahead!

Thank you, it's nice to hear.

plamaiziere said:
First, I'm happy to see that you use spaces. It looks like spaces are very expensive when I look the code made by some beginners at work (but may be I'm too old and I need new glasses...).

Yes, I too prefer the four space instead of a Tab symbol (8 positions).

plamaiziere said:
Be consistent with spaces, I don't see why "int i=0" has no space but "i < 0" has some?

Oh, it's simple.
When I do the initialization I do not use spaces:

Code:

int a=21;
int b=10, *c=(int)&a;

When I use "set" (=) I'm using blanks.

Code:

int a, b, *c;
a = 21;
b = 10;
c = (int)&a;

When I use boolean expression I also use one spaces.

Code:

if (a > b)
     *c = b;

The same way

Code:

for (int i=0, /* Initialize - there is no space */
        i < 100, /* Boolean expression - there is space */
        i++) {... ... ...}

Would you recommend me to always use the space?

plamaiziere said:
There are few things I dislike:
In main(), the "if else if else if else if" where you test options is quite hard to read and understand, this is a typical case where you can use "continue / return / goto" to make the code more readable.

I think use the "goto" operator - bad! Say "NO" to "goto" operator

If you reduce the number of "if - else" I came up with just such a solution! What do you think about this?

plamaiziere said:
There are few useless cast (avoid cast!) :

Code:

hookword = (hookword ? (char*)hookword : (char*)DEFAULT_HOOKWORD);

hookword is a char *, and DEFAULT_HOOKWORD too...

Yes, I thought about it. But c89 standard recommended me explicitly convert types. Yes, there is no type conversion - I see it. Thank you, I will take into account your remark!

plamaiziere said:
Also the construction above is not very readable, why you want to say is "if hookword is null, put DEFAULT_HOOKWORD". I don't see the point to use the "?" operator here. I prefer

Code:

if (hookword == null) hookword = DEFAULT_HOOKWORD;

which is exactly why you want and easily readable.

I love the short "if" - it saves space.
But I agree with you! You're right - in this situation is no sages. In this case, I had to write:

Code:

if (!hookword)
    hookword = DEFAULT_HOOKWORD;

Thank you, you're right!

plamaiziere said:
To finish, I've noticed some "if (i++ < XXX)", I'm not sure if this is and undefined behavior or not (when the post incrementation is done?). I'm not a good C programmer and I'm too lazy to check this, so I would avoid this (doing C is like carrying some TNT, if you are not cautious it will explode IMO).

No! This is normal! I distinguish suffix and prefix increment / decrement! It works as me needed ..
Suffix and prefix increment / decrement somewhere described in the Standard c89 - it should always work predictably.

plamaiziere said:
VoilÃ , this is highly subjective of course. Perfect Code is a bit like the Holly Grail.

Thank you! (or is it sarcasm?)

plamaiziere said:
you will find some advise for C style in the "man style" manual page of FreeBSD and all the FreeBSD's source code to see how it is made. It's good C and there are a lot of things to learn from..

Regards.

Yes, thank you. I looked the FreeBSD OS source code .

Thank you for your time! You helped me to find some problems, and think about the style!

graudeejs · May 6, 2012

plamaiziere said:
First, I'm happy to see that you use spaces. It looks like spaces are very expensive when I look the code made by some beginners at work (but may be I'm too old and I need new glasses...).

Be consistent with spaces, I don't see why "int i=0" has no space but "i < 0" has some?

Speaking about code style, I can recommend lint(1) and/or devel/astyle. These are code formaters

I usually prefer astyle. But both have their strengths and weaknesses.

valsorym · May 6, 2012

graudeejs said:
Speaking about code style, I can recommend lint(1) and/or devel/astyle. These are code formaters
I usually prefer astyle. But both have their strengths and weaknesses.

Thank you for interesting programs. When choosing between lint(1) vs devel/astyle. Me more like astyle, too.

But honestly, did not see the point in this program. Code rules are quite simple, and use programs lint(1) and/or devel/astyle it seems to me superfluous.

In terms of design code to me more like the language Python. There is code written by different people always looks the same. It is a pity that such a rule is not entered in the C language.

But in any case, thank you! Me need to think about the code style.

plamaiziere said:
Be consistent with spaces, I don't see why "int i=0" has no space but "i < 0" has some?

Once again, I looked the source code of FreeBSD - where spaces are used everywhere: initialization / assignment (=) / boolean expressions.
Yes, I have something to think about it. Thank you!

valsorym · May 6, 2012

I looked at the style code FreeBSD sources, listened to your advice and looked like running astyle and made the final version of the source code.

P.S. Guys, I know you are bored to look at it. But I'm really interested in your opinion - any recommendations / suggestions / tips ....

throAU · May 7, 2012

Personally (this is not BSD style or whatever):

On spaces vs tabs.... if you want 4 space tabs, set your editor to display tabs as 4 spaces.

Other people working on your code may prefer more/less visual indentation. If you use tabs, they can choose without having to reformat all of your code. All that is stored in the file is a TAB character, how that is expanded out when viewed is totally up to the user viewing the code.

fluca1978 · May 7, 2012

I appreciate the level of comments in your code, that is not usual and is very useful.
Besides what have been already commented about the general coding, I would recommend you to use some kind of versioning, like github or bitbucket or alike, this will make reviewing your code easier and will not require pastebin.

anon12b · May 7, 2012

Some of these points may just be my preferences, but:

You don't need to roll your own argument parsing, use getopt(3)
Your code would have more clarity if you explicitly list conditions, rather than using C's integer true/false semantics.
Similarly with errexit(), see errx(3)
Another clarity point: if you are going to initialise values at the beginning of a scope, it is usually better to put them on separate lines. This is not necessary, but it is a bit of a jumble when some values in the middle of a line are initialised, and the rest are not.
Finding a character in a string, see strchr(3)
Use array notation over *(array + offset). Pointer dereference may look cooler, but there is no point in duplicating a syntactic element like this.
It seems strange that you rely on integer conversion to boolean a lot, then suddenly swap to using your boolean enum.
You do not check the return values of some of your functions that can fail.
Your use of strncat(3) is incorrect in createyep. The count specifies what will be appended, not the total length.
Random files names: mktemp(3), and friends.
It may also be more clear if you perform assignments outside of conditionals; especially in cases where you are negating both the function call, and the result.
You use a lot of unnecessary casts, which make the code less readable.
You can use char in a switch case, which may be more readable than a series of if...else.

I hope some of those help. I only commented on style, rather than examining any algorithms, or techniques especially.

plamaiziere · May 7, 2012

doorways said:
Thank you, it's nice to hear.

Yes, I too prefer the four space instead of a Tab symbol (8 positions).

I prefer tabs myself.

Oh, it's simple.
When I do the initialization I do not use spaces:

Ok but isn't easier to remember only one rule: use space? I think this is more readable too.

I think use the "goto" operator - bad! Say "NO" to "goto" operator

goto is useful to handle errors and, on error, to release the ressources previously allocated. This is widely used for this case (check the FreeBSD kernel: more than 22000 goto!):

Code:

$ fgrep -R 'goto ' /sys/* | wc -l
   22047

If you reduce the number of "if - else" I came up with just such a solution! What do you think about this?

You introduce (if I remember the original code) a structure to simplify the "if else if else ...". This is, in my opinion, useless.

Thank you! (or is it sarcasm?)

This was not at all a sarcasm. I meant that perfect code does not exist. If you are happy with your code, be happy! Code style is something very subjective.

Regards.

valsorym · May 9, 2012

anon12b said:
I hope some of those help. I only commented on style, rather than examining any algorithms, or techniques especially.

Yes, thank you. Your comments are very helpful for me.

valsorym · May 9, 2012

Thank you all, I saw a lot of things out of your criticism. I have something to think about - thank you all very much!

P.S. The final result.

_martin · May 12, 2012

I've noticed you don't use curly brackets to set block for if/while/for when only one statement follows it. Though this is OK it does make code less readable. Brackets set block and it's easier to understand which block belongs where.

But know there are more styles you can use - there's not one good standard (at least indent wise). As you asked on FreeBSD forums, you can check developers handbook. I think K&R C Programming language book has some style recommendations too.

valsorym · May 12, 2012

matoatlantis said:
I've noticed you don't use curly brackets to set block for if/while/for when only one statement follows it. Though this is OK but it does make code less readable. Brackets set block and it's easier to understand which block belongs where.

But know there are more styles you can use - there's not one good standard (at least indent wise). As you asked on FreeBSD forums, you can check developers handbook. I think K&R C Programming language book has some style recommendations too.

Yes, thank you. Now I reading the "FreeBSD Developers' Handbook", section "Style Guidelines" - it says that it is necessary to use the style(9). Good observation, they use curly brackets often. Thanks.

jrm@ · May 12, 2012

Google's C++ style guide is another style resource. It's a superset of what you need since it's for C++ and not C.

valsorym · May 12, 2012

jrm said:
Google's C++ style guide is another style resource. It's a superset of what you need since it's for C++ and not C.

Thanks, but I am not use C++, I use only C. :stud

bigearsbilly · May 21, 2012

Code:

$(EXECUTABLE): yep.o
	$(CC) $(CFLAGS) -o $@ $> $(LDFLAGS) 

yep.o: yep.c
	$(CC) $(CFLAGS) -c $>

Also you don't need to re-define these rules in your makefile. They already exists.
You don't even need a makefile to work in a simple manner:

Code:

$ echo 'int main(void){return 0; }' > 1.c
$ make 1                                 
[B]cc -O2 -pipe   1.c  -o 1[/B]


$ export CFLAGS='-g -Wall'                                            
$ rm 1
$ make 1                  
[B]cc -g -Wall   1.c  -o 1[/B]

expl · May 21, 2012

Code:
Code:

CFLAGS = -Wall -ansi -std=c99

This is wrong. You are contradicting yourself in this line. -ansi means strict C90 standard that is most supported dialect by all compilers. You are overwriting -ansi with -std=c99. And if you leave just -ansi your code will not compile because it does not comply with strict C90 rule set. From quick observation: you use C++ style comments // strict C comment is /* */ nothing else, you declare variables inside loop definition like for (int var=0; ...; ...) this is not strict ANSI C valid. Those are the ones that I noticed by quick overlook.

Your argument parsing is kinda messy chain of ifs and else ifs, should rewrite this for tsarg() to return argument code and then just switch() it, that would prevent unnecessary function calls (function call is waste of CPU cycles).

There is a buffer overflow possibility in errexit() since you use sprintf instead of snprintf.
Similar error in terminal().

Code:

if (terminal("rm -f %s", tempfile))

Have you heard about remove()?

There are to many strlen() calls and you are failing to use "const" declarations on a lot of variables that you use as constants.

Thats what I've noticed with quick look over the code.

valsorym · May 21, 2012

bigearsbilly said:
Also you don't need to re-define these rules in your makefile. They already exists.
You don't even need a makefile to work in a simple manner:

expl said:
Thats what I've noticed with quick look over the code.

Thank you, guys.
It is constructive, your comments useful to me.