1. This site uses cookies. By continuing to use this site, you are agreeing to our use of cookies. Learn More.

Criticism of the C code, a young programmer!

Discussion in 'Userland Programming and Scripting' started by valsorym, May 5, 2012.

  1. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    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!
     
  2. plamaiziere

    plamaiziere New Member

    Messages:
    184
    Thanks Received:
    39
    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 thanks for this.
  3. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    Thank you, it's nice to hear.

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

    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?

    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?

    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!

    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!

    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.

    Thank you! (or is it sarcasm?)

    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!
     
  4. graudeejs

    graudeejs Well-Known Member

    Messages:
    4,594
    Thanks Received:
    632
    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 thanks for this.
  5. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    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.

    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!
     
  6. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    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 ....
     
  7. throAU

    throAU New Member

    Messages:
    912
    Thanks Received:
    134
    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.
     
    valsorym thanks for this.
  8. fluca1978

    fluca1978 New Member

    Messages:
    733
    Thanks Received:
    68
    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.
     
    valsorym thanks for this.
  9. anon12b

    anon12b New Member

    Messages:
    17
    Thanks Received:
    5
    Some of these points may just be my preferences, but:

    1. You don't need to roll your own argument parsing, use getopt(3)
    2. Your code would have more clarity if you explicitly list conditions, rather than using C's integer true/false semantics.
    3. Similarly with errexit(), see errx(3)
    4. 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.
    5. Finding a character in a string, see strchr(3)
    6. Use array notation over *(array + offset). Pointer dereference may look cooler, but there is no point in duplicating a syntactic element like this.
    7. It seems strange that you rely on integer conversion to boolean a lot, then suddenly swap to using your boolean enum.
    8. You do not check the return values of some of your functions that can fail.
    9. Your use of strncat(3) is incorrect in createyep. The count specifies what will be appended, not the total length.
    10. Random files names: mktemp(3), and friends.
    11. 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.
    12. You use a lot of unnecessary casts, which make the code less readable.
    13. 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.
     
    valsorym thanks for this.
  10. plamaiziere

    plamaiziere New Member

    Messages:
    184
    Thanks Received:
    39
    I prefer tabs myself.

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

    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
    


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

    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 thanks for this.
  11. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    Yes, thank you. Your comments are very helpful for me.
     
  12. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    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.
     
  13. matoatlantis

    matoatlantis Member

    Messages:
    533
    Thanks Received:
    75
    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 thanks for this.
  14. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    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.
     
  15. jrm

    jrm Member

    Messages:
    798
    Thanks Received:
    225
    valsorym thanks for this.
  16. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    Thanks, but I am not use C++, I use only C. :stud
     
  17. bigearsbilly

    bigearsbilly New Member

    Messages:
    143
    Thanks Received:
    5
    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                                 
    cc -O2 -pipe   1.c  -o 1
    
    
    $ export CFLAGS='-g -Wall'                                            
    $ rm 1
    $ make 1                  
    cc -g -Wall   1.c  -o 1
    
    
    
     
    valsorym thanks for this.
  18. expl

    expl New Member

    Messages:
    664
    Thanks Received:
    121
    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 thanks for this.
  19. valsorym

    valsorym New Member

    Messages:
    419
    Thanks Received:
    20
    Thank you, guys.
    It is constructive, your comments useful to me.