Boost CRC example program file

Question

I'm currently looking at this Boost::CRC example code which I have also inserted below.

I always try to look for suggestions for improving my own coding style when I encounter well-written and well-formatted code.

This code definitely looks like good code, but two things puzzle me about this:

• is it good practice to copy #defined constants to local const variables before using them? Why would this be a good idea?

• is it actually acceptable to just omit the parenthesis around a function body if the body is a try-catch-block?

//  Boost CRC example program file  ------------------------------------------//

//  Copyright 2003 Daryle Walker.  Use, modification, and distribution are
//  subject to the Boost Software License, Version 1.0.  (See accompanying file
//  LICENSE_1_0.txt or a copy at <http://www.boost.org/LICENSE_1_0.txt>.)

//  See <http://www.boost.org/libs/crc/> for the library's home page.

//  Revision History
//  17 Jun 2003  Initial version (Daryle Walker)

#include <boost/crc.hpp>  // for boost::crc_32_type

#include <cstdlib>    // for EXIT_SUCCESS, EXIT_FAILURE
#include <exception>  // for std::exception
#include <fstream>    // for std::ifstream
#include <ios>        // for std::ios_base, etc.
#include <iostream>   // for std::cerr, std::cout
#include <ostream>    // for std::endl


// Redefine this to change to processing buffer size
#ifndef PRIVATE_BUFFER_SIZE
#define PRIVATE_BUFFER_SIZE  1024
#endif

// Global objects
std::streamsize const  buffer_size = PRIVATE_BUFFER_SIZE;


// Main program
int
main
(
    int           argc,
    char const *  argv[]
)
try
{
    boost::crc_32_type  result;

    for ( int i = 1 ; i < argc ; ++i )
    {
        std::ifstream  ifs( argv[i], std::ios_base::binary );

        if ( ifs )
        {
            do
            {
                char  buffer[ buffer_size ];

                ifs.read( buffer, buffer_size );
                result.process_bytes( buffer, ifs.gcount() );
            } while ( ifs );
        }
        else
        {
            std::cerr << "Failed to open file '" << argv[i] << "'."
             << std::endl;
        }
    }

    std::cout << std::hex << std::uppercase << result.checksum() << std::endl;
    return EXIT_SUCCESS;
}
catch ( std::exception &e )
{
    std::cerr << "Found an exception with '" << e.what() << "'." << std::endl;
    return EXIT_FAILURE;
}
catch ( ... )
{
    std::cerr << "Found an unknown exception." << std::endl;
    return EXIT_FAILURE;
}

Answer 1

With the manifest constant being assigned to a const variable, you now have two ways of referring to the same value. In principle, this is superfluous, and should therefore be avoided. In practice, with manifest constants always being globally visible in C++, this is a rather moot point, but it can still be used for making a decision in lack of any other point. So, if there are any other considerations not immediately evident in the code, such as the need to be able to redefine the manifest constant AND at the same time the need to have the constant strongly typed, then by all means, it should be kept, and preferably documented.

The try statement used as a function body looks mighty weird, which means that the average C++ programmer looking at it will go "WTF?". At the same time it does not accomplish any mighty feat that will make one add "--oh, I see why it is done that way, cool!". It is okay for a coding technique to have a certain WTF factor to it, as long as it accomplishes something neat, the neatness of which is proportional to its WTFness, so as to justify it. All that the try-statement-used-as-a-function-body accomplishes is to spare us from having to type an additional --but expected-- pair of curly brackets. Therefore, it should definitely be avoided.

Answer 2

is it good practice to copy #defined constants to local const variables before using them? Why would this be a good idea?

It's not good or bad, but it's definitely weird. In general I prefer constant values to #defines because they have an explicit type that can be checked at compile time. The only benefit (and I hesitate to call it that) of this code is that you could define PRIVATE_BUFFER_SIZE in another header file and it would override the definition here.

is it actually acceptable to just omit the parenthesis around a function body if the body is a try-catch-block?

It might compile, but it's a terrible practice. Don't do it! ;)

Answer 3

I'd invert the condition and use a continue if there is an error with the stream:

for (int i = 1; i < argc; ++i) {
    std::ifstream  ifs(argv[i], std::ios_base::binary);
    if (!ifs) {
        std::cerr << "Failed to open file '" << argv[i] << "'." << std::endl;
        continue; // or return EXIT_FAILURE;
    }

    do {
        char buffer[buffer_size];

        ifs.read(buffer, buffer_size);
        result.process_bytes(buffer, ifs.gcount());
    } while (ifs);
}

http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html

I'm not a C++ master, but maybe the streams should be closed somewhere.

(I know that this isn't an answer to your questions but maybe you find it useful.)