Determining which command line arguments are numeric

Question

I am working on using STL and in general safe C++ practices. Instead of posting large samples of code I am working with a small sample to catch problems before I move onto larger ones.

How do you find out if an istringstream found an input that is not of the desired type? That is, you have a string and you want to verify if it is a float or not.

Using the program below, when I input

[mehoggan@desktop argument_processing]$ ./args 1 2 3 4 2y

I get:

1 2 3 4 2 

which is not what I want, since not all arguments where numeric.

I should of gotten:

ERROR: 2y is not numeric

What improvements would you make to this small program?

#include <vector>
#include <iostream>
#include <algorithm>
#include <sstream>
#include <iterator>

namespace CLP {
    using std::vector;
    using std::istringstream;
    using std::string;
    using std::cerr;
    using std::endl;

    vector<float> all_numeric_arguments( int argc, char *argv[ ] ) {
        vector<float> v_argv;
        vector<string> v_tmp;
        copy( &argv[1],&argv[argc],back_inserter( v_tmp ) );
        for( vector<string>::iterator it=v_tmp.begin( );it!=v_tmp.end( );it++ ) {
            istringstream iss( (*it) );
            float f=0.0;
            if( iss >> f >> std::dec ) {
                v_argv.push_back( f );
            } else {
               cerr << "ERROR: " << (*it) << " is not numeric " << endl;
               return v_argv;
            }
        }
        return v_argv;
    }
}

int main( int argc, char *argv[ ] ) {
    std::vector<float> v = CLP::all_numeric_arguments( argc, argv );
    if( v.size( ) == (unsigned)(argc-1) ) {
        copy( v.begin( ),v.end( ),std::ostream_iterator<float>( std::cout," " ) );
        std::cout << std::endl;
    }
}

Answer 1

In your case, you apparently want to verify that there was not only a successful conversion, but also that the conversion consumed all the input. Given that this is exactly what Boost lexical_cast does, my immediate advice would be to use its existing code to do the job. If you can't do that, I'd still write a similar function, and probably give it the same name, so people who already know about lexical_cast will be able to recognize what you're doing without having to read through the code to do it.

As far as other changes go, rather than using std::copy to copy argv into v_tmp, I'd probably just initialize v_tmp from argv. I'd also note that &x[y] is generally equivalent to x+y, and simplify that part of the code a bit, giving:

std::vector<string> v_tmp(argv+1, argv+argc);

That gives us code that looks like:

template<typename Target, typename Source>
Target lexical_cast(const Source& arg) { 
    // ...
}

std::vector<string> v_tmp(argv+1, argv+argc);
vector<float> v_argv;

for (int i=0; i<v_tmp.size(); i++)
    v_argv.push_back(lexical_cast<float>(v_tmp[i]);

That does leave one real question, however: exactly how you want to signal the fact that your lexical_cast has failed. Boost lexical_cast throws an exception. One possible alternative to that would be to return a NaN instead. I'm not at all sure I'd recommend that, but I can imagine situations where it might be the preferred way to go.

Assuming you go the route of throwing an exception, the code would change to something like this:

for (int i=0; i<v_tmp.size(); i++) {
    try { 
        v_argv.push_back(lexical_cast<float>(v_tmp[i]));
    }
    catch (bad_lexical_cast const &) {
        std::cerr << "Error: " << v_tmp[i] << " is not numeric.\n";
    }
}

That leaves us with the definition of lexical_cast itself. Most of it would quite obviously be essentially identical to the code you had, just adding a check that after we do the conversion, the stream is empty:

template<typename Target, typename Source>
Target lexical_cast(const Source& arg) {      
    istringstream iss(arg);
    Target f = Target(); // generic equivalent to your `0.0` initialization

    iss >> f;
    char c;

    // if the conversion failed, or we can read another character, we 
    // couldn't convert, or there's data left in the stream, which we 
    // consider a failure.
    if (iss.fail() || iss.get(c))
        throw bad_lexical_cast();
    return f;
}

Since that's throwing a bad_lexical_cast, we'll also have to define that type. In this case, we'll keep it simple:

class bad_lexical_cast {};

In reality, you usually want to derive your exception classes (directly or indirectly) from std::exception (unless you've defined a similar hierarchy of your own -- in any case you typically want a common base class for all your exceptions). You probably also want to include an indication of what failed and why in the exception object itself, so the code that catches it doesn't need as much information about what was being done at the time as we used here.

Answer 2

Here are two small points regarding the formatting:

1. In all_numeric_arguments after introducing v_tmp, you should call v_tmp.reserve(argc); so that you won't do to many reallocations. (This is purely an optimization which is probably not important in this example, but since you know the size it is a good idea.)

2. In the for loop, you should use const_iterators to make clear that you won't modify v_tmp. Also (another optimization which is simply a good habit) do not call v_tmp repeatedly.

   So your for loop should read like this:

   for( vector<string>::const_iterator it = v_tmp.begin(),
                                       end = v_tmp.end();
        it != end ; ++it )
   {
       //...
   }
   

   or if you can use C++0x already:

   for (auto it = v_tmp.cbegin(), end = v_tmp.cend(); it != end; ++it) { /* ... */ }