"Warp" <war### [at] tagpovrayorg> wrote in message
news:40190021@news.povray.org...
> Dan P <dan### [at] yahoocom> wrote:
> > How can I make my code better?
>
>   That depends on whether you definitely want to stick to C or if you
would
> like to do it the right way in C++... :)
>   With C++ many things can be done a lot more nicely and safely (eg.
minimize
> the risk of memory leaks).
>   However, let's make this the C-way for once. C is quite cumbersome in
many
> ways, but it may be sometimes good to know how things can be done better
> there (you never know when you will be forced to use C in some project).
> C offers some tools for better modularity, even though not many.
>
>   So let's see your code:

This is GREAT stuff you're pointing out!!
Every C/C++ programmer should sit down and read the previous message in
full!

>   I haven't looked yet if all those variables are needed, but anyways,
> you should encapsulate them inside a struct. Besides not contamining
> the global namespace it makes it easier to enhance your program in
> the future. For example, if you want for some reason to keep many images
> on memory at the same time it will be much easier having them in struct
> instances.

That's a great point! I had never thought of that.

>   Then in the main() function you can create an instance of this struct
> which can be used in the functions it calls:

<stuff about parameters>

That's one of those things that I didn't put in because I was in a hurry :-)
Definitely good advice for any production piece of software, though.

>   Naturally blur() has to get the data as parameter now, so the call
> would become: blur(&data);

Always a good thing to pass things around. Otherwise, it's just hard to use
it again. Good advice!

> >  free((void *) buffer);
> >  free((void *) page);
>
>   Why are you casting the the pointers to void* before freeing them?

I saw that in a book and wondered the same thing when I first saw it. It
just stuck in my head as one of those, "Well, I'm sure the author knew a
reason... maybe it's for some compatibility I don't know of?" kind of
things.

>   And by the way, this is one of the bad things about C. In C++ you could
> make the struct to automatically free those arrays when it goes out of
> scope.
>   Doing it in the C-way (which you are pretty much forced to do, usually)
> not only breaks modularity badly, but is also dangerous because it
> increases the danger of memory leaks.

(Raising a glass to that!)

<class stuff>

Definitely. I do C for little things, but always C++ for anything "real".

>   Usually you should return EXIT_SUCCESS from main() if you want to be
> sure of portability.
>   EXIT_SUCCESS is *usually* 0, but the standard does not specify that.

I did not know that! I'll remember that!!!

> > void blur()
> > {
> >  unsigned i;
> >  unsigned xn;
> >  unsigned yn;
> >  unsigned r;
> >  unsigned g;
> >  unsigned b;
>
>   You can write that as:
>
> unsigned i, xn, yn, r, g, b;

Well, this one we differ on. I find it easier to conceptualize the code when
everything is on a seperate line. It doesn't affect compile-time, but I
think it makes the code more readable. It's a style-thang.

> >  fflush(0);
>
>   It's 'fflush(stdout);', and it's unneeded anyways.

Good point on using the variable. I've worked on systems where I had trouble
with putchar() without flushing, though.

> > void load (char *path)
>
>   It's a good practice to take that kind of parameter as "const char*
path".
> It not only avoids you accidentally modifying it, but it also avoids all
> kinds of problems, specially in C++.

Hey, great idea!

<stuff about buffer-overrun exploits>

Normally, I do worry about that for any production code (I always use
strncpy, or things like that, for example). That is a great point about
vulernabilities.

>   In C there's a great function for parsing files with a fixed content:
> fscanf(). Learn to use it. You don't need temporary buffers for reading
> integers with it.

Cool

>   As for the version, you can simply "read it away" from the input file.
> You don't need to store it anywhere.

It's  just a habit to read things like that for future enhancements. Maybe
not a good one.

> >  if ((stream = fopen(path, "rb")) != NULL)
> >  {
>
>   In my personal opinion it's better to handle the failure first and
> the success then. Now the failure handling is many tens of lines below,
> and it's hard to find.

I've never heard that take on it before, but it is a logical take! I usually
like to list the code in good->error order because it becomes easier to read
what it is supposed to do in succession. I guess you can call that an
"optimistic" coding structure :-)

>   As for the failure printing itself, it's a good habit to print the
*reason*
> why opening failed, not just that it failed.
>
>   There's a nice function in C for doing exactly that: perror().

So true!

>   And I have never understood why people use obfuscated C for no apparent
> reason to open a file. Why it can't be opened and checked in two separate
> lines? It's not like it would be less efficient or anything, but certainly
> easier to read.

Laziness.

>   The fprintf/perror combination is a nice trick I figured out years ago
> to print out a nicely-formatted error message.
>
> >    buffer = (unsigned *) malloc(units * sizeof(int));
>
>   Allocating space for ints but converting it to an unsigned array is
> a bit odd. It usually works, but...
>   Better safe than sorry, just use sizeof(unsigned). It's not that hard.

Oh, you're right -- that was a bug on my part. I used unsigned so I can use
up to 65535 instead of being stuck with half that.

>   By the way, you don't check whether the subsequent images have a proper
> size with regard to the first image. It's a good idea to check that.
> (For instance, if the subsequent images are bigger than the first one
> you will be writing outside your array, causing a segmentation fault.
> If they are smaller, the result will be quite odd.)

Again... laziness :-}

Thank you very much for the helpful critique! You have made an impact on my
coding style already, particularly with the exit success message. I really
appreciate that you took the time to help me out!!!

Post a reply to this message