"Warp" <war### [at] tagpovrayorg> wrote in message
news:4017878f@news.povray.org...
> Dan P <dan### [at] yahoocom> wrote:
> > PS: Please don't judge my coding skills on that posting -- I wrote that
as a
> > quick one-off to get the job done and I thought it might illustrate the
> > concept.
>
>   IMHO quick coding is not an excuse to break every possible good coding
> principle in the market... ;)
>
> > I know you should never use global variables and should comment
> > code
>
>   Global variables were just one of the many problems in your code,
> and comments aren't necessary if the code is otherwise very clear and
> easy to read.

(Bracing my ego) Okay, I'll bring the red scarf out and wave it to and fro.
How can I make my code better?

Post a reply to this message

"Dan P" <dan### [at] yahoocom> wrote in message
news:401851cb@news.povray.org...
> "Tom Galvin" <tom### [at] imporg> wrote in message
> news:Xns947E38A2533E6tomatimporg@203.29.75.35...
> > "Dan P" <dan### [at] yahoocom> wrote in
> > news:40170c7c$1@news.povray.org:
> >
> > >  an Open Source replacement for SPatch/HamaPatch
> > >
> >
> > http://jPatch.sourceforge.net

Definitely not a bad start! I am shocked at the speed and the user interface
is more pleasant than HamaPatch (and more professional than a lot of
"professional" software I see as well). Personally, I'm going to go in the
direction of exploiting a very specific technology for the heavy-duty side
at the expense of what JPatch excels at:
cross-platformabili..illi...ility... (is that a word?) It's great to know
that JPatch is coming along so good and I can see already that it will
replace HamaPatch very nicely, thank-you-very-much.

I look forward to seeing more!!!

Post a reply to this message

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:

> unsigned *buffer = NULL;
> char *page = NULL;
> unsigned units = 0;
> int frames = 0;
> int width;
> int height;
> unsigned x;
> unsigned y;
> unsigned x2;
> unsigned y2;
> unsigned w;
> unsigned h;

  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.
  So perhaps something like this:

struct BlurData
{
    unsigned* buffer;
    char* page;
    unsigned units;
    int frames, width, height;
    unsigned x, y, x2, y2, w, h;
};

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

> int main (int argc, char **argv)
> {
      BlurData data = { NULL, NULL, 0, 0 };

>  int i = 1;
>
>  x = atoi(argv[i++]);
>  y = atoi(argv[i++]);
>  x2 = atoi(argv[i++]);
>  y2 = atoi(argv[i++]);

  Here you don't check if the number of command-line parameters is correct.
That is what 'argc' (argument count) is for.
  If the user didn't give the correct amount of parameters the program
should print a syntax reminder. Instead, your program misbehaves and
prints nothing helpful. It will probably cause a segmentation fault.

  Also, you don't check that the user gave correct parameters, and even
if the parameters are valid, if they make sense.
  atoi() will return 0 if the parameter is not a number. If you want to
go the easy way you could just define that if the user gives invalid
parameters it's the same as he had given a 0. However, you should at
least check that not all the four parameters are 0.

  Using 'i' to index fixed positions in 'argv' seems useless, but that
isn't so bad. It makes it easier to change the number of command-line
parameters.

  So you could perhaps do something like this:

if(argc < 6) /* we need at least 4 coordinates and one file */
{
    fprintf(stderr, "Syntax: %s <x1> <y1> <x2> <y2> <ppm files>\n", argv[0]);
    return EXIT_FAILURE;
}

data.x = atoi(argv[i++]);
data.y = atoi(argv[i++]);
data.x2 = atoi(argv[i++]);
data.y2 = atoi(argv[i++]);

if((data.x==0 && data.y==0 && data.x2==0 && data.y2==0) ||
   data.x2 < data.x || data.y2 < data.y)
{
    fprintf(stderr, "Invalid coordinate parameters.\n");
    return EXIT_FAILURE;
}

data.w = data.x2 - data.x;
data.h = data.y2 - data.y;

  The two last checks in the previous 'if' were so that w and h wouldn't get
screwed up.

>  while (argv[i] != NULL)

  You should use 'argc' to see where do the parameters end. I don't think
you can do that according to the C standard. 'argv[argc]' and beyond can
probably be anything, giving you trash (and if you are lucky causing a
segmentation fault).
  So it should be: while(i < argc)
  We can also do it this way:

for(i = 5; i < argc; ++i)
{
    load(argv[i], &data);
    ++data.frames;
}

  I think that it would be better if load() increased 'data.frames', but
that's a minor detail.

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

>  free((void *) buffer);
>  free((void *) page);

  Why are you casting the the pointers to void* before freeing them?

  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.

  If we were using C++, we could enhance our struct (which in fact should
be a class with the variables in its private part, but that's another
story) with a destructor, like this:

struct BlurData
{
    (The data here)

    ~BlurData
    {
        if(buffer) free(buffer);
        if(page) free(page);
    }
};

  Then the arrays will be automatically freed when 'data' goes out of
scope (so it doesn't matter where the program is terminated).

  Naturally if we were using C++ we wouldn't make it a struct at all,
but a fully-functional class which in itself makes the blur and handles
practically everything. (main() would simply create an instance of this
class and give it the file names.)

>  return 0;

  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.

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

>  fflush(0);

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

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

>  char s_version[1024];
>  char s_width[1024];
>  char s_height[1024];
>  char s_depth[1024];

  This should be something which immediately turns on every alarm in your
head.
  Static buffer overflow is the oldest and most common exploit of all time,
and it still is nowadays.

  Why? Because of the attitude of programmers: "This is a small program
it doesn't really matter". They don't get accustomed to writing secure
code, they don't know how to do it, and then when they write code to
a program which will become something important...

  Believe or not, people are still making this mistake, in the year 2004,
in crucially important programs which can be used to exploit systems.
It all starts with all those "small and unimportant" programs...

  Please don't teach yourself bad habits. Get rid of them from the very
start.

  Besides, you are reserving 4 kilobytes of memory for something which
probably needs a few bytes. It doesn't matter here, but it can start
mattering when the same thing is done millions of times...

  In C++ there's a safe way of parsing words from a stream by using
the std::getline() function. In C it requires more work to get a secure
dynamic string working.
  However, you don't even need dynamic strings in this case. For example,
you are storing some version information in 's_version' but you don't use
it anywhere. Also, you are storing numbers in char arrays for no apparent
reason.

  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.
  As for the version, you can simply "read it away" from the input file.
You don't need to store it anywhere.

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

  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.

  So my suggestion is:

stream = fopen(path, "rb");
if(stream == NULL)
{
    fprintf(stderr, "Couldn't open ");
    perror(path);
    return;
}

  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.

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

-- 
#macro M(A,N,D,L)plane{-z,-9pigment{mandel L*9translate N color_map{[0rgb x]
[1rgb 9]}scale<D,D*3D>*1e3}rotate y*A*8}#end M(-3<1.206434.28623>70,7)M(
-1<.7438.1795>1,20)M(1<.77595.13699>30,20)M(3<.75923.07145>80,99)// - Warp -

Post a reply to this message

"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

Dan P <dan### [at] yahoocom> wrote:
> >   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!!!

  It's way too common that people don't care about the return value of
the program they are making (specially if it's a command-line program).
  Often they return EXIT_SUCCESS and EXIT_FAILURE because they have been
taught to do so, but they don't really understand why they should do so.
  In the worst case they don't care what the program returns and they
for example *always* return 0 regardless of the cause of the program
termination, or what is a lot worse, always return a value different
from 0.

  It is important that a program returns EXIT_FAILURE when it could not
do what it was supposed to do (eg. bad command-line parameters, couldn't
open an input file, etc). It's also important to return EXIT_SUCCESS when
the program was able to correctly perform its task and everything went fine.

  One could ask "why is it so important? If I run the program from the
command line, what does it matter what it returns? I will see from its
printout if it went well or wrong."

  However, programs are not always run from the command line. One common
usage of command-line programs where the correct return value is crucial
is when running it from a Makefile.
  The make program will *stop* if the program returns an error code
(usually a value different from 0). Thus if the program returns an error
code needlessly it will make the program unusable in a Makefile.
  It's also important for the program to return an error code when an
error happens: If it always returns success, make will not know to
stop and will think the program was successful and will continue (thus
potentially causing all kinds of weird results, the reason of which can
be very hard to find).

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

  flushing a stream is necessary when you need to ensure that the output
is really written to its destination at that point (streams are usually
buffered and things may not be written immediately to the destination).
  If it's enough for the output to be written when the program ends then
there's no need to flush explicitly because the compiler will generate
the flushing call at the end of the program anyways (the C standard
actually requires this, AFAIK).

  One example of a useful use of fflush() is when you are printing some
progress information with printf() (using "\r" to go to the beginning
of the line after each update): fflush(stdout) will ensure that the
progress information is actually printed to standard output at that point.

  fflush() shouldn't be called too often because it makes the program slow.
For example calling fflush() after each output character is madness unless
there's a really good reason for you to do so... :)

  (By the way, stderr is usually unbuffered, which means that it's in
practice flushed after each character. This means that stderr should not
be used for printing megabytes of data. :) )

  And by the way, fflush() takes a FILE pointer, not a file descriptor.
When you give it a 0 you are actually giving it a NULL pointer.

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

  Which 10-years old system are you using where ints are 16-bit? ;)

-- 
plane{-x+y,-1pigment{bozo color_map{[0rgb x][1rgb x+y]}turbulence 1}}
sphere{0,2pigment{rgbt 1}interior{media{emission 1density{spherical
density_map{[0rgb 0][.5rgb<1,.5>][1rgb 1]}turbulence.9}}}scale
<1,1,3>hollow}text{ttf"timrom""Warp".1,0translate<-1,-.1,2>}//  - Warp -

Post a reply to this message

"Darren" <dne### [at] sanrrcom> wrote in message
news:401a7b78$1@news.povray.org...
> Warp wrote:
> >   If it's enough for the output to be written when the program ends then
> > there's no need to flush explicitly because the compiler will generate
> > the flushing call at the end of the program anyways (the C standard
> > actually requires this, AFAIK).

Ah yes, but remember: we might take this code and wrap it in a class later,
so having that fflush there might help us avoid future bugs. Also, even
though the C standard requires this, experience has shown me that not
everybody keeps to the standard when they write their C compilers (see
Visual Studio). To me, flushing the buffer is kindof like closing a file. I
don't have to at the end, but it's good practice and it really doesn't hurt
anything if the stream isn't buffered.

Also, the reason I output using STDOUT is because the netpbm library works
that way. Remember, I chained this program together in a pipe to create PNG
files from PPM files. I don't think there is even a way to use netpbm
without using STDOUT, although I might be wrong (happens a lot).

Post a reply to this message

Dan P <dan### [at] yahoocom> wrote:
> Ah yes, but remember: we might take this code and wrap it in a class later,
> so having that fflush there might help us avoid future bugs.

  If you are going to enhance the program to something which may be used
somewhere where flushing can be important, you can add it then. :)

> Also, even
> though the C standard requires this, experience has shown me that not
> everybody keeps to the standard when they write their C compilers (see
> Visual Studio).

  Can you imagine a compiler which does not output all data written to
a file if you don't fflush() explicitly? Can you imagine how many buggy
programs that would create? Almost every single program would misbehave
if this was the case.
  I can't imagine that. Believe me: Compilers do generate code which flushes
the output.

> To me, flushing the buffer is kindof like closing a file. I
> don't have to at the end, but it's good practice and it really doesn't hurt
> anything if the stream isn't buffered.

  I see no reason. The stream *will* be flushed, period. There's no doubt
about that.
  If you use fflush() too much you are just potentially creating inefficient
code.

  There are *tons* of things which one really *should* do but which no-one
does because of laziness or whatever (for example, have you ever checked
that your printf() call really succeeded?). However, I don't think calling
fflush() is one of them.

  fflush() is important and handy when you really have to make sure that
the output is flushed immediately, but using it needlessly is a waste
of code. :)

> Also, the reason I output using STDOUT is because

  Did someone put that decision into question?

-- 
plane{-x+y,-1pigment{bozo color_map{[0rgb x][1rgb x+y]}turbulence 1}}
sphere{0,2pigment{rgbt 1}interior{media{emission 1density{spherical
density_map{[0rgb 0][.5rgb<1,.5>][1rgb 1]}turbulence.9}}}scale
<1,1,3>hollow}text{ttf"timrom""Warp".1,0translate<-1,-.1,2>}//  - Warp -

Post a reply to this message

In article <40170c7c$1@news.povray.org>,
 "Dan P" <dan### [at] yahoocom> wrote:

> * Don't give me no guff about DirectX... I don't see SGI working it's
> tail-off to make OpenGL better, but Microsoft is clamoring to do so because
> of their gaming franchise, and Microsoft seems to be turning a new leaf
> lately... along with the rest of the industry. .NET is wonderful thing.

...
I don't know where to start. Argh...
*beats Daniel repeatedly over the head with a clue-bat...*

Look into OpenGL 2.0. OpenGL is being actively developed, and the 
stability of its API is an advantage. And both DirectX and .NET are 
completely, utterly useless to anyone not on a recent version of Windows.

-- 
Christopher James Huff <cja### [at] earthlinknet>
http://home.earthlink.net/~cjameshuff/
POV-Ray TAG: <chr### [at] tagpovrayorg>
http://tag.povray.org/

Post a reply to this message

"Warp" <war### [at] tagpovrayorg> wrote in message
news:401b8003@news.povray.org...
>> > Also, the reason I output using STDOUT is because
>
>   Did someone put that decision into question?

I thought somebody said pushing megabytes of data through STDOUT was a bad
idea, but I might have misread.

Post a reply to this message

"Christopher James Huff" <cja### [at] earthlinknet> wrote in message
news:cjameshuff-5D04D9.14013731012004@news.povray.org...
> In article <40170c7c$1@news.povray.org>,
>  "Dan P" <dan### [at] yahoocom> wrote:
>
> > * Don't give me no guff about DirectX... I don't see SGI working it's
> > tail-off to make OpenGL better, but Microsoft is clamoring to do so
because
> > of their gaming franchise, and Microsoft seems to be turning a new leaf
> > lately... along with the rest of the industry. .NET is wonderful thing.
> ...
> I don't know where to start. Argh...
> *beats Daniel repeatedly over the head with a clue-bat...*
>
> Look into OpenGL 2.0. OpenGL is being actively developed, and the
> stability of its API is an advantage. And both DirectX and .NET are
> completely, utterly useless to anyone not on a recent version of Windows.

Okay, I'm admittedly at fault here because I used an antiquated phrase to
start that footnote and I can see where some people might not understand
what "Don't give me not guff" might mean. What that means is that,
undoubtedly, some people are going to feel very strongly about this
perception and I know why and it is just a matter of opinion. Kind of like
predicting the stock market -- lots of people have different opinions on
where it is going to go and the future will tell. I happen to think that
DirectX is the future, not OpenGL, based on two things: 1. Microsoft is
realizing that their gaming business very important and they tend to do well
on things they find important (think IDEs), and 2. I have been in SGI and I
have reason not to have confidence in that company based on personal,
anecdotal experience.

Also, my target audience is people with the latest version of Windows. I
have decided to go in a different direction than JPatch, which is why I'm
thrilled that JPatch exists. I'm looking to exploit a specific technology,
yet provide the source openly. It's like the fight between Linux Torvalds
and Andy Tanenbaum; they both have completely different opinons on how
someone should build an operating system, but they are both valid and based
on personal opinion. Linus just happened to build his into Linux. See page:
http://cscserver.cc.edu/jtowell/AAATeaching/csc317/Linus%20vs_%20Tanenbaum.htm

I'm not mad, I just have an opinion and I realize it will differ and that
people feel very strongly about it.

Post a reply to this message