Criticism wanted ( examine my small progs )

[Shadow]

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".

[Prelude]

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

[Shadow]

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.

[Prelude]

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

[Salem]

> 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

[Shadow]

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!

[Prelude]

>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

[Shadow]

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:

#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:

	.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?

[billholm]

Just testing my signature and avatar. Hehehe

[Shadow]

> 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.

[billholm]

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.

[Shadow]

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.

[billholm]

This is another mistake.

[billholm]

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.

[Shadow]

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.*