"Warp" <war### [at] tagpovrayorg> wrote in message
news:415d5857@news.povray.org...
> Ross <rli### [at] everestkcnet> wrote:
> > Source code is in C++. Constructive criticism on the source is welcome.
I
> > probably over did some things and maybe blindly missed others.
>
>   1191 lines of code in 16 files for converting TGA images to DF3
> certainly feels a bit like overdoing things... :)
>   But it isn't that bad, really.

Yes, it is a bit much for a simple concept. My primary goal were threefold.
1) make a tga to df3 converter that wouldn't segfault because wrong or
invalid command line arguments and bad tga input. 2) make a tga to df3
converter that was faster than a previously available implementation. 3)
improve my C++.

I could get old versions of tga2df3 to crash just by running it from a
driectory where the files didn't exist, even if I put the full path to the
files as the command line argument. I wanted to fix that. I also wanted to
improve the speed, even though speed isn't a big deal in this program. In my
tests, i've cut the time about in half (using the "time" utility to test).
#3 is a work in progress as you have seen. I'm comming from a background
where C++ was taught as C with classes. Last year I bought Stroustrups book
and was opened to what C++ can really be. Then I bought one of Scott Meyers
book and started putting some of his tips in my code.

So, your most harsh criticism would be appreciated. if you think it's too
offtopic for here, feel free to respond wherever.

>
>   I do not understand why you need to allocate an instance of arg::CL_Args
> dynamically at the beginning of main().
>   Is there a reason why this won't do?
>
> arg::CL_Args args(argc, argv);
>
>   Unnecessarily allocating it dynamically is only a source of problems and
> has no benefits.

Yes, i realize this, tried to change it once, and something went wrong. It
was a while ago. It definately is a plan to do as you said. Your comments on
this topic of C++ in other threads was guiding me actually. I think i have a
comment where I declare arg::CL_Args that I should stop using dynamic
allocation here.

>   And in fact, you *have* one such problem in your code: There are several
> exit points in the program where the instance is not freed.

damn. thought I caught them all. that'll show me.

> If you had
> used a local instance there wouldn't be such problem. You also (quite
> inconsistently) free it in one catch block but not in another.
>
>   In main.cpp you have this line:
>
> if (args->Sort()){ sort(file_q.begin(), file_q.end()); }

yes, that should be the sort from <algorithm>. I know I am not #include-ing
it anywhere. It isn't intentionally obfuscated. i'll clean that up for sure.
So the compiler should really give a warning or not compile then? I should
really be including <algorithm> and saying "std::sort(...)". gcc glitch? i'd
blame myself before gcc though.

>
>   It's customary to initialize variables in the initialization list of the
> constructor whenever possible (in the case of integral variables I suppose
> there isn't any great advantage in doing so, but it doesn't hurt either).

i've only recently started doing this. I think I do it in a few places. I
understand the benefits and I am trying to migrate towards that. What do you
think about, i forget the proper name, exception statements on functions.
for instance saying "this function can throw exceptions of this type". like
"int foo() throws bla" or whatever. I started using it, then commented them
out. I wasn't sure if you have a heirarchy of exception classes if you would
have it throw the derived-from class, or the derived class. did that make
sense?

>
>   It's good programming practice to declare member functions const when
> they do not change the state of the object (this makes it possible to
> call these methods when having a const reference or whatever). You have
> declared some of non-modifying methods const, but not all of them.

List initialization lists, this is a part of the language I am trying to put
into my default usage. I've only used it in some cases.

>
>   Using namespaces is a good idea, but usually namespaces are used to
> distinguish bigger wholes than just one class. Having a namespace called
> 'df3' containing just one class called 'DF3' sounds a bit like overdoing
> things. That's not really what namespaces are useful for.

yeah, I wasn't quite sure what to do. it *felt* like overkill, but I didn't
know the best way to go about it. Instead of dropping namespaces altogether,
I did what I did. suggestions?


>
>   Commenting your code is usually a good idea. It makes things easier to
> understand for others.
>   For example, DF3::writeDF3Header() returns an int, but it's completely
> unclear what the return value means. Looking at its implementation doesn't
> help either: It just returns 1. A short comment in the declaration of
> this function about what the function is returning would clear things up
> a lot.
>
>   On the other hand, comments like this one do not cast vast amounts of
> trust in the program:
> "// for some reason, this exception isn't being unwound properly
>  // to main()'s catch block. so catch it here and exit."
>   That comment means, in other words: "This is a kludge."
>   (Good thing you were honest on adding such a comment, though. That's
much
> better than just leaving it uncommented. It's, however, usually a good
thing
> to resolve the problem instead of just coding a kludge around it.)
>

Commenting is an art. I'll try to improve. As far as that "kludge" comment,
it was weird. Exceptions really weren't being unwound. I don't know if it
was a compiler issue or my fault. Maybe cleaning and fixing other parts will
help make it obvious.


>   I know it's often tempting to use C's shortcuts for some things, but
> unless you are writing obfuscated code it may be a better idea to do it
> the clear way, not the short way. For example, this line of code:
>
> ((r+1) == tga_readcount) ? bytes = tga_lastread : bytes =
image_data.size();
>
> takes a while to be understood. And there's absolutely no reason for doing
> it that way. (And in fact, that's an expression returning a value which is
> not used for anything.)
>   Why not do it the clear way, which can be understood right away (and
also
> probably doesn't cause most compilers to warn about an unused value):
>
> if(r+1 == tga_readcount)
>     bytes = tga_lastread;
> else
>     bytes = image_data.size();
>
>   And even if you wanted to use the ?: shortcut, wouldn't this be better:
>
> byte = ((r+1) == tga_readcount) ? tga_lastread : image_data.size();
>

premature optimization is the root of all evil. i'm guilty here. just trying
to avoid branches.


>
>   You have hard-coded the type of the DF3 voxel: unsigned char.
>   Hard-coding a type is in some cases a bad idea. This is one of such
cases.
>   What if the user would want to convert a 16-bit B/W TGA (or a 48-bit
color
> TGA) to DF3 so that voxels are 16 bits long?

I saw you mention, i think it was you anyway, in another thread that df3's
support 8, 16, and 24(or was it 32?) bit "voxels". Is this a new thing?
Also, is the df3 format a pov-team "invention", or what? I was working off
of about 8 lines of information in the povray 3.5 documentation and for
developing a program based on a file format for the first time, it was a
little vague. i'd like to allow more for more flexibility. that wasn't a
goal for this version. i wouldn't mind implementing it now, however.

> If you wanted to support this
> you would need to make quite a lot of changes to your code.
>   This is because you hard-coded the size of the voxels in your code,
which
> makes it rigid and hard to change.
>   A much better OO design is to use an abstract voxel type which public
> interface does not fix the size of the voxel to a certain small limit
> (it could take brightness values as eg. longs or doubles and internally
> convert them to whatever actual type is used).
>   This way, even if you had fixed the type to unsigned char, you would
> have done so only inside the abstract voxel type and it would be very
> easy to change it to something else (eg 'unsigned short' or 'unsigned
int'),
> or make it dynamic altogether (ie. use the type which best matches the
> input TGA pixel size and/or perhaps some command-line parameter).
>   This is why abstraction is such an important OO concept.
>
>   I'm in a hurry right now, so I only was able to make a couple of
comments
> about the code. Perhaps I'll review more later, if you are interested.
>

I am very interested. I was fearing your comments actually. I agree with
everything you have said with respect to C++ programming in the past
(primarily resource management), and know I can't get it all right the
first, second, or third time. Like I said, feel free to comment more if you
have time. This is the first program i've ever released to public eyes,
thanks for helping.

-ross.

Post a reply to this message