Criticism wanted ( examine my small progs )

This is a discussion on Criticism wanted ( examine my small progs ) within the C Programming forums, part of the General Programming Boards category; If you would, examine my 3 programs and tell me what areas I should brush up my skills in. 3zips.zip ...

  1. #1
    Unleashed
    Join Date
    Sep 2001
    Posts
    1,765

    Criticism wanted ( examine my small progs )

    If you would, examine my 3 programs and tell me what areas I should brush up my skills in.

    3zips.zip inclues 3 small programs:

    CmdLine.c -
    An example of using command line parameters while defaulting to a usage message.

    Install.c ( this file probably has big problems )-
    A configurable (somewhat) installing / file copying program.

    AutoText.c -
    A program to make text "type itself".
    Attached Files Attached Files
    The world is waiting. I must leave you now.

  2. #2
    Code Goddess Prelude's Avatar
    Join Date
    Sep 2001
    Posts
    9,796
    AutoText.c
    ----------
    An interesting solution, but there are a few problems and inaccurate results. The first problem is that the array anim has 38 characters but you only give it 37 (Don't forget the NUL). Since it isn't to be modified anyway, why not remove any size errors by simply assigning the string statically to a char *?

    [pedantic]
    You have a type mismatch between the literal values and the variables. The literals will be intepreted as int the way they are and you are assigning them to long, to be explicit you want to place an L at the end of the literal, like so:

    towait<10000000L
    [/pedantic]

    I have a problem with the delay loop. Due to the difference in speed of different computers, this loop will take more or less time to execute, resulting in varying output speeds. A better way would be to define a sleep function with the standard time libraries so that the delay is predictable across any platform that uses any normal measure of time.

    The outer loop is redundant and just a waste of space, you can safely remove it and the output will be the same. The only reason I can think of it being there is if you have multiple lines that you want to separate, though that can be done by embedding newlines in the string and the outer loop is unnecessary yet again.


    CmdLine.c
    ----------
    No real problems with this except that you neglect to inform the user of invalid input in the error case. Perhaps mentioning that what they entered was incorrect and here is an example to emulate.


    Install.c
    --------
    Well, this may take a bit.

    >if( ( Tmp = fopen("Install.cfg", "r") ) == 0)
    This is a bit pedantic, but you can't be sure that 0 and NULL can be interchangeable on every implementation. If you are testing for NULL, then use NULL.

    As it is, procedure can return void since you don't return anything useful and even then you ignore it.

    >char FileName[11] = "Install.cfg";
    Remember the nul terminator, this string is 12 characters long.

    >infoFile = fopen(FileName, "r");
    Always test files to see if they were opened successfully. This is a point where errors are very common, so error checking should be equally common.

    >while( fscanf( infoFile, "%s%s%s", Source, Destination, Name ) != EOF )
    This really chaffes with me, it just doesn't feel safe. Perhaps some validation checking and using fgets and sscanf to parse the string instead of fscanf would help a bit. These three strings are vital to the rest of the program running properly, check them over with the proverbial white glove treatment.

    >fclose(infoFile);
    What if fopen returns NULL? The program will choke, this is another reason to check the return value of fopen. You can place the call to fclose in the success case so that if fopen fails bad things don't happen.

    >int FileCopy ( char * a, char * b, char * c )
    Very bad style, try to create variable names that actually mean something so that the reader has a chance of figuring the program out without you being required to comment every other line.

    The actual file copy routine looks like one of mine, so I'll pass it over as fully tested and working properly.

    That's what I found with a quick look over, it compiles and I assume that it runs. I'll leave the stress testing to you though, it may break in certain situations that you didn't prepare for.

    All in all, not bad. Nothing terribly sinful and the programs all apparently work as intended.

    -Prelude
    My best code is written with the delete key.

  3. #3
    Unleashed
    Join Date
    Sep 2001
    Posts
    1,765
    These 3 programs demonstrate some of the things I'm trying to learn right now. CmdLine is the only program I pretty much created myself. All the rest of the programs are sort of meshes of other's code . (Remember, I'm learing these...). Thanks Prelude!

    P.S. That was "exactly" your file copying routine.
    The world is waiting. I must leave you now.

  4. #4
    Code Goddess Prelude's Avatar
    Join Date
    Sep 2001
    Posts
    9,796
    Reading over this again I noticed a slight error:
    >if( ( Tmp = fopen("Install.cfg", "r") ) == 0)
    This is a bit pedantic, but you can't be sure that 0 and NULL can be interchangeable on every implementation. If you are testing for NULL, then use NULL.
    You can't be sure that 0 will be interpreted as a pointer to null, so it's best to use the NULL macro which is usually defined as (void *)0 to explicitly say that the value is a pointer. If you wish to keep the 0 though, you can cast it to (FILE *)0 and the effect would be the same as using NULL. The macro is essentially shorthand for casting zero.

    -Prelude
    My best code is written with the delete key.

  5. #5
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,434
    > You can't be sure that 0 will be interpreted as a pointer to null
    Actually, you can
    http://www.eskimo.com/~scs/C-faq/q5.9.html

    A zero in a pointer context is always cast (by the compiler) into a NULL pointer of the appropriate type

  6. #6
    Unleashed
    Join Date
    Sep 2001
    Posts
    1,765
    Thank you Salem and Prelude for your reviews.
    I have pretty much all of the problems worked out except one, which I am still working on.

    Salem:
    I have been frequently reviewing the information at eskimo.com.
    I have also been reviewing:
    http://acm.uva.es/problemset/gnudoc/libc/Top.html ,
    which contains mainly the same information. - Informative links!
    The world is waiting. I must leave you now.

  7. #7
    Code Goddess Prelude's Avatar
    Join Date
    Sep 2001
    Posts
    9,796
    >A zero in a pointer context is always cast (by the compiler) into
    >a NULL pointer of the appropriate type
    Most of the time, but not always: http://www.eskimo.com/~scs/C-faq/q5.2.html.
    Since Shadow's code is a comparison with a pointer, the conversion is made so in hindsight I really didn't have to be so general in my explanation.

    -Prelude
    My best code is written with the delete key.

  8. #8
    Unleashed
    Join Date
    Sep 2001
    Posts
    1,765
    The first problem is that the array anim has 38 characters but you only give it 37 (Don't forget the NUL).
    Yes Prelude,

    I was searching this site:
    http://gcc.gnu.org/onlinedocs/gcc-3.0.4/gcc_3.html#SEC5
    for an accurate list of command line compiler options for my gcc compiler ( curiousity got me ) and I decided to instruct my compiler to hault the compilation of this code:
    Code:
    #include <stdio.h>
    #include <time.h>
    
    int main(void)
    {
        	int counter;
        	char * autotext = "This is an example of automatic text.";
            for(counter=0;counter<38;counter++)
            {
            	sleep(100);
                    printf("%c",autotext[counter]);
            }
            return 0;
    }
    at the assembly stage. I now have assembly output. It has not yet been converted in to machine code.

    Getting back to your explination of the array and the extra nul character, I found this to be interesting in the assembly output:
    Code:
    	.file	"autotext.c"
    gcc2_compiled.:
    ___gnu_compiled_c:
    	.def	___main;	.scl	2;	.type	32;	.endef
    .text
    	.align 32
    LC0:
    	.ascii "This is an example of automatic text.\0"
    LC1:
    	.ascii "%c\0"
    	.align 4
    .globl _main
    	.def	_main;	.scl	2;	.type	32;	.endef
    /* code continues */
    > .ascii "This is an example of automatic text.\0"
    I see now.
    Why does it assign the extra character?
    The world is waiting. I must leave you now.

  9. #9
    Registered User billholm's Avatar
    Join Date
    Apr 2002
    Posts
    225

    Talking

    Just testing my signature and avatar. Hehehe
    All men are created equal. But some are more equal than others.

    Visit me at http://www.angelfire.com/my/billholm

  10. #10
    Unleashed
    Join Date
    Sep 2001
    Posts
    1,765
    > Just testing my signature and avatar. Hehehe
    It looks fine to me.
    In the future you could create a temporary post in the General Discussions board.
    You could also construct a better way to perform such a test.
    The world is waiting. I must leave you now.

  11. #11
    Registered User billholm's Avatar
    Join Date
    Apr 2002
    Posts
    225

    Lightbulb

    Sure thing! Oh by the way, your install.cprogram just gave me a nice idea I'm gonna make a rabbit program and spike my unruly neighbor who claims to be a hacker hehehe.
    All men are created equal. But some are more equal than others.

    Visit me at http://www.angelfire.com/my/billholm

  12. #12
    Unleashed
    Join Date
    Sep 2001
    Posts
    1,765
    billholm,
    http://www.tuxedo.org/~esr/faqs/smart-questions.html
    Read the information in this link before posting again on this thread. I would ask you to delete your posts, but I won't go that far. I came back here thinking I would have somewhat of an answer. When I arrived, I was grately mistaken. Thank you.
    The world is waiting. I must leave you now.

  13. #13
    Registered User billholm's Avatar
    Join Date
    Apr 2002
    Posts
    225
    This is another mistake.
    All men are created equal. But some are more equal than others.

    Visit me at http://www.angelfire.com/my/billholm

  14. #14
    Registered User billholm's Avatar
    Join Date
    Apr 2002
    Posts
    225
    Oh I think you were hurt by what I said, weren't you? Don't worry bcos I don't hate hackers, bcos I'm a hacker-trainee myself. Hackers have helped me a lot already.

    But my neighbor is different. HE DESERVES TO BE TAUGHT A LESSON IN COMPUTER ETHICS by showing him what the "Golden Rule" means.
    All men are created equal. But some are more equal than others.

    Visit me at http://www.angelfire.com/my/billholm

  15. #15
    Unleashed
    Join Date
    Sep 2001
    Posts
    1,765
    It's late. I have a headache a need sleep.

    > This is another mistake.

    *Mumbles short spurts of jibberish, with a tone of disgust, in the general direction of your 4 glorious words of wisdom.*
    The world is waiting. I must leave you now.

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. a little OT, but, criticism and avice wanted !
    By Shadow in forum C++ Programming
    Replies: 2
    Last Post: 10-01-2001, 08:38 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21