- In the constructor or CL_Args you are assigning "" to several member
strings. This is unnecessary. Strings naturally have proper constructors
which initialize them to empty strings. There's no need to do that yourself.
  Besides, the better way to empty a string is "outfile.erase();".

  - You have some bad type of code repetition in your command-line parsing
function. It's nothing catastrophic because the amount of command-line
switches is pretty small, but it's still not a good idea to learn bad
habits like this. It also is a potential problem if you want to expand
your program later (with more command-line options etc).
  You also use a rather inefficient way of parsing the command line, even
though it isn't anything catastrophic either because of its usual small
size (it's highly unlikely that someone will give the program a command
line which is several megabytes big). However, you shouldn't get accustomed
to bad habits here either.

  Bad repetition:
  * You have hard-coded command-line option name strings in more than one
    place.
  * You parse each one of them with a repeating similar group of code lines
    (when you find yourself doing this it should automatically make you
    wonder if you couldn't use a loop to do it more cleanly with fewer
    lines).

  You also parse the command-line arguments inefficiently: For each possible
option type you traverse the entire command line again and again.

  Possible ways of doing it in a cleaner and more efficient way are endless
but I'm not going to start pondering on this now. However, one key principle
which applies to this (as well as many other things) is abstraction.

  - A more correct (and probably efficient) way of checking whether a string
is empty or not is "name.empty()".

  - What is the ';' in this line doing? arg::ArgsError::~ArgsError(){;}

  - If a class has a constructor taking a parameter, then the class has
no default constructor (one which takes no parameters). It's not necessary
to explicitly declare one private.
  (Besides, if you disable a constructor or operator by declaring it private,
it's important that you don't implement it. If you implement it, the class
itself could use it by mistake, something you wanted to avoid in the first
place.)

  - You have a potential bug in your code: In the copy-constructor of
DataBuffer you copy the contents of the buffer given as parameter using
strcpy() (without the 'std::', by the way; probably another bug in gcc).
  strcpy() assumes the two char arrays are 0-terminated. Is this really
the case?
  The C++-way of copying things is to use std::copy() (from <algorithm>).
(If you are worrying about its performance, you probably don't need to.
Most C++ library implementations probably have a specialization for
arrays of internal types which call 'memcpy()', which is quite fast.)

  - I have written quite some programs reading TGA files myself, and
there's a rather easy (and obvious?) way of reading its header more
easily than byte by byte:
  Just use a 18-bytes long array of chars and read 18 bytes from the
input to it with one stream function call. Then you can read the bytes
you are interested in from the proper offsets in this array. (Since I'm
usually only interested in a few of them, this saves quite a lot of work.)
  So a typical code which I write to read a TGA file would look like this
(in this case I'm reading a paletted 256-color TGA):

    unsigned char header[18];
    std::fread(header, 1, 18, infile);
    if(header[2]!=1 || header[16]!=8)
    {
        std::cerr << "Only uncompressed indexed 8-bit targas, thanks."
                  << std::endl;
        return 1;
    }

    xSize = header[12]+header[13]*256;
    ySize = header[14]+header[15]*256;

    // Skip user data and palette:
    std::fseek(infile, header[0]+(header[5]+header[6]*256)*header[7]/8,
               SEEK_CUR);


-- 
#macro N(D)#if(D>99)cylinder{M()#local D=div(D,104);M().5,2pigment{rgb M()}}
N(D)#end#end#macro M()<mod(D,13)-6mod(div(D,13)8)-3,10>#end blob{
N(11117333955)N(4254934330)N(3900569407)N(7382340)N(3358)N(970)}//  - Warp -

Post a reply to this message