As an exercise, I've written a program implementing run-length encoding.
I would be more that happy to hear your comments, suggestions, etc
Sure thing.
#include <stdio.h>
#include <string.h>
int main(int argc, char *argv[])
{
int cur = 0, i = 0, prev = 0;
FILE *src, *dest;
Richard suggests initializing all your variables
to *some* mnemonic value at declaration time; e.g.,
FILE *src = NULL;
FILE *dest = NULL;
My opinion differs on this matter; in my opinion,
you shouldn't even be initializing 'cur' and 'prev'
until you have figured out whether the program is
going to be processing a file or not, at least.
At any rate, I think we all agree that variable
declarations should be one per line unless there's
a readability issue at stake:
FILE *src = NULL;
FILE *dest = NULL;
int cur;
int prev;
int i = 0;
Finally, I see that you seem to be using the value
of 'i' to represent several different things in this
program (?). Usually, single-character names like
'i' (and 'j' and 'k') are "reserved" for use as loop
counters. When you break with this convention, it
makes your writing more confusing than it really needs
to be. I suggest you remove the declaration of 'i'
and insert declarations of better-named variables
inside the appropriate 'if' blocks [see below].
if(argc == 4)
{
src = fopen(argv[argc-2], "rb");
dest = fopen(argv[argc-1], "wb");
As Richard already said, [argc-2] is just a fancy
way of writing [2] at this point.
More importantly, you're sticking an awful lot of
code inside this if-block. You need to figure out
what the essential "building blocks" of your program
are going to be, and split them out into functions.
In a program like this, I would make one function
int process_rle(FILE *input, FILE *output);
and have 'main' call that function when needed. But
then, I see that this program really has three distinct
functionalities, so let's give it three distinct
processing functions:
int process_comp(FILE *input, FILE *output);
int process_uncomp(FILE *input, FILE *output);
int process_dump(FILE *input, FILE *output);
Try it out!
if(src == NULL)
{
fprintf(stderr, "error: %s can't openned\n", argv[argc-2]);
return -1;
Grammatical and spelling errors in your error messages
are never a good sign.
}
if(dest == NULL)
{
fprintf(stderr, "error: %s can't openned \n", argv[argc-1]);
return -1;
}
if((strcmp(argv[1], "--dump")) == 0)
See my note further down about user-interface issues.
{
while((cur = fgetc(src)) != EOF)
No reason to use 'fgetc' here when 'getc' would work
just as well. The only difference between 'fgetc'
and 'getc' is that 'fgetc' is guaranteed to evaluate
its argument ('src') only once. And that doesn't
matter in this case. So use 'getc'.
fprintf(dest, "%x ", cur);
You're reading single bytes from 'src', but writing
hexadecimal numbers to 'dest'? What exactly is this
program supposed to be doing? Do you have another
program that translates the hex output back into
single bytes, or do you really want this sort of
output?
fclose(src);
fclose(dest);
}
else if((strcmp(argv[1], "--comp")) == 0)
{
for(;fscanf(src, "%x", &cur) == 1; prev = cur, i++)
*This* is where you should have initialized 'i' to zero --
in the initialization section of the 'for' loop that uses
its value! [Also, 'i' should be named 'count' or similarly.]
{
if(prev != cur)
{
if(prev != 0)
What if the file contains '\0' bytes? You never
print those, do you?
fprintf(dest, "%02d %x ", i, prev);
Ye gods, that's confusing. You're going to alternate
between printing decimal and hexadecimal numbers to
the output file? How's the reader supposed to tell which
is which?
i = 0;
}
}
/* dump the unprocessed data */
if(i)
fprintf(dest, "%02d %x ", i, prev);
fclose(src);
fclose(dest);
}
else if((strcmp(argv[1], "--uncomp")) == 0)
{
while(fscanf(src, "%d%x", &i, &cur) == 2)
{
while(i-- > 0)
Here's the second bizarre use of 'i' to mean 'count'.
Better to use a loop syntax that's obviously correct,
like
int i;
for (i=0; i < count; ++i)
fprintf(dest, "%x ", cur);
}
fclose(src);
fclose(dest);
}
else
fprintf(stderr, "error: unknown option %s\n", argv[1]);
....and close the files, too, or are you giving up on
that? Be consistent!
[You don't *have* to fclose(src) and dest if you don't
want to -- the program will take care of that for you
when it exits. But since you did it consistently in every
other case, why not do it here too? Of course, once you've
pulled out the 'process_rle' function(s), you'll have only
one place that needs 'fclose's. Hint hint.]
}
else
printf("usage: hexdump OPTIONS <src-file> <dest-file>\n"
"\nOPTIONS:\n"
"\t--dump \t dump src-file in hex;\n"
"\t--comp \t perform rle compression on src-file;\n"
"\t--uncomp\t decompress the data\n");
Okay. User-interface time.
First off, don't use '\t' characters in output that's supposed
to be human-readable. Use spaces instead; makes the output much
more consistent between different user configurations. Or if
you're really hardcore, just use the 'printf' format specifiers.
Next, note that when you write "OPTIONS", most people are going
to assume that those "OPTIONS" are... well, OPTIONAL. So it may
come as a surprise to them to find out that the program just
shows this same help message over and over if they don't enter
exactly *one* of these OPTIONS as the first parameter.
Next, note that this program doesn't actually compress very
many files. It'll probably expand anything you give it,
*especially* text or images. That's because it reads bytes
and for each byte, writes up to five bytes of text. That's
kind of silly, unless you're just doing that kind of output
for debugging purposes.
Finally, once you've split out those 'process_*' functions,
you will find that your 'main' function is really empty! Put
that empty space to good use by adding a decent parser for your
command line -- never force the user to enter arguments in a
certain order if there's another logical way they could do it.
See
http://www.contrib.andrew.cmu.edu/~ajo/free-software/detab.c
for a simple example of a decent 'main' function.
Good job.
All in all, a better program than the average first effort.
But you need to work on splitting up functions; on variable
naming and encapsulation (declare 'i' close to where it's
used, for example); and on your program concept in general--
where are you trying to take this program? Did you want to
use putc() instead of fprintf(), for example? How about
adding some error-checking on the value of 'i', making sure
it won't overflow? Things like that.
Hope this helped,
-Arthur
