Simplifying FOR loops

Question

I have a function that essentially reads values in from a vector of doubles, appends these to a string (while ensuring a space between each and setting their precisions) and returns the end result, minus the final whitespace:

std::string MultiplePrintProperties::GetHpitchString()  
{    
    std::string str;  
    vector< double >::iterator it;    

    for ( it  = Vals.begin();    
          it != Vals.end();  
          it++ )  
    {
        ostringstream s;    

        // Set precision to 3 digits after the decimal point
        // and read into the string 
        boost::format fmt( "%.3f " ); 
        s << fmt % *( it );
        str.append( s.str() );   
    }

    // Remove last white space and return string   
    return str.substr( 0, str.length() - 1 ); 
}

I would like to find out if this code could be simplified in any way. I have recently been investigating the use of for_each and functors in particular but have not been able to figure out how these techniques could improve this particular example.

Answer 1

Since you're actually transforming doubles into strings, and appending these strings to a stringstream, you can use std::transform for that:

// your functor, transforming a double into a string
struct transform_one_double {
   std::string operator()( const double& d ) const {
     boost::format fmt( "%.3f " ); 
     return (fmt % d).str();
   }
};

// iteration code, taking each value and inserting the transformed
// value into the stringstream.
std::transform( vals.begin(), vals.end()
              , std::ostream_iterator<std::string>( s, " ")
              , transform_one_double() );

Answer 2

You can create a class with an overloaded operator() having a reference to std::string as a member. You will declare an object of this class and pass the string into constructor, then use the object as the third parameter to for_each. Overloaded operator() will be invoked for each element and append the text to the referenced string.

Answer 3

Seems like I'm a bit old skool these days. I would have done this:

std::string MultiplePrintProperties::GetHpitchString()  
{    
    std::string str;  
    vector< double >::iterator it;    

    for ( it  = Vals.begin();    
          it != Vals.end();  
          it++ )  
    {
        // Set precision to 3 digits after the decimal point
        // and write it into the string 
        char buf[20];
        snprintf( buf, 20, "%.3f", *it );
        if (str.length() > 0)
            str.append(" ");
        str.append( buf );          
    }

    return str; 
}

Answer 4

as stated above, lots of ways to achive this, but... doesn't this method just beg for having some more parameters and being templatized? suppose you had

template< class tType >
std::string PrintVectorToArray( const std::vector< tType >& V, const char* Seperator );

then you could create

1 2 3

1, 2, 3

1.0 and then 2.0 and then 5.0

for any type convertable to string and with any seperator! I once did it this way, and now find myself using this method a lot.

Answer 5

I'd suggest to use a single stringstream and a single format. Those aren't exactly cheap.

std::string MultiplePrintProperties::GetHpitchString()  
{    
    std::ostringstream s;    
    // Set precision to 3 digits after the decimal point
    static boost::format fmt( "%.3f " ); 

    for ( vector< double >::iterator it  = Vals.begin();    
          it != Vals.end(); it++ )  
    {
        // and read into the string 
        s << fmt % *( it );
    }
    // Remove last white space (if present) and return result
    std::string ret = s.str();
    if (!ret.empty()) ret.resize(ret.size()-1);
    return ret;
}

If I had profiling information proving it was still a bottleneck, I'd consider using a static ostringstream:

static std::ostringstream s;    
...
std::string ret;
std::swap(ret, s.str());
return ret;

Answer 6

The 'fmt' variable should be declared outside your loop, as setting the formatting up every iteration is slow and not needed. also the stringstream is not needed. So the body would become something like this:

  std::string s;
  std::vector<double>::iterator i = vals.begin();

  if (i != vals.end())
{
  boost::format fmt("%.3f");
  s = str(fmt % *i++);

  while (i != vals.end())
    s += ' ' + str(fmt % *i++);
}

Answer 7

I did not find your original code bloated or in desperate need of simplification. I would however move the

boost::format fmt("%.3f");

and

ostringstream s;

out of the loop to ensure they are only initialized once. This would save of a lot of str.append()-ing too. I'm guessing xtofl's std::transform() solution will have this problem (it's easy to fix by initializing it once for the struct though).

If you are looking for other alternatives to

 for (it = begin(); it != end(); ++it) {...}

check out BOOST_FOREACH which would enable you to iterate in the following manner:

std::vector<double> list;
BOOST_FOREACH(double value, list) {
    ...
}