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.

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

  It's quite unclear where is this 'sort()' function coming from. (And in
fact I'm perplexed that this even compiles! There's no #include <algorithm>
anywhere to find, and even if there was, there's no 'using' anywhere to
find, and the only other 'sort' I can find is a private member variable
of CL_Args.).
  You have either somehow managed to find a bug in the gcc libraries or
to make such an obfuscated piece of code that I can't figure it out... :)
  But anyways, since you are not using 'using' (which is something which
should never be used anyways, so you have a very good principle!), then if
you are using the standard sort() function it's a good idea to use 'std::sort'
for clarity. (The fact that it compiles without the include and the prefix
still perplexes me.)

  You don't need to explicitly close a std::ofstream in the destructor of
your class. std::ofstream will close itself automatically when it is
destroyed. This is not Java. ;)

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

  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.

  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.

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

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


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

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