povray <nos### [at] nospamit> wrote:
> comments are very appreciated!!

  Some programmatical issues:

- You could increase portability by avoiding things like: system("pause");
(Besides, pausing after a syntax error is not customary in CLI apps. It
interferes with automatic processing, eg. when using makefiles.)

- There's a potential out-of-boundaries access in the GetOption() function:
You index param[0] and param[1] without checking that the string is long
enough for that.

- fprintf( pfile, data[i].c_str()); is not safe, if the string in question
happens to have any % symbols. (Unlikely, but can happen.) The proper way
of doing it is: fprintf( pfile, "%s", data[i].c_str());
  (You could also have used an fwrite() instead. Or C++ streams.)

- It would be more informative if you used perror() (or strerror()) when
opening a file fails. That way the user would get the exact reason for the
failure.

- In the GetExtention() function (which should really be name GetExtension)
does not check if finding the '.' character succeeded. If it fails, then
std::string::npos will be returned, which you assign to an int (rather
than size_t), which you then increment and given as parameter to
std::string::erase. The result is undefined (and might result in a crash).
You should use size_t rather than int, and check if std::string::npos was
returned.
  Likewise for SetExtention().

- argv[0] is in no way guaranteed to contain the path to the executable.
In fact, in most situations it won't contain it.


  I'm not going to nitpick about performance issues because they are
not urgently relevant (but might be an issue if you are converting huge
files).

-- 
                                                          - Warp

Post a reply to this message