3
\$\begingroup\$

I have the below and it gives me the desired output (ie; 1 day 12:05:33) but it looks overly complicated, is there a more straight forward way of doing this with the least amount of string concatenations?

     std::string eta_string ( int64_t s ) {

     time_t seconds ( s );
     tm p;
     gmtime_s ( &p , &seconds );
     std::string str;

     if ( p.tm_yday > 0 ) {
      if ( p.tm_yday == 1 ) {
        str = std::to_string ( p.tm_yday ) += " day ";
      } else {
        str = std::to_string ( p.tm_yday ) += " days ";
      }
      if ( p.tm_hour < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_hour ) += ":";
      } else {
        str = str += std::to_string ( p.tm_hour ) += ":";
      }
      if ( p.tm_min < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_min ) += ":";
      } else {
        str = str += std::to_string ( p.tm_min ) += ":";
      }
      if ( p.tm_sec < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_sec );
      } else {
        str = str += std::to_string ( p.tm_sec );
      }
     }
     else if ( p.tm_hour > 0 ) {
      if ( p.tm_hour < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_hour ) += ":";
       } else {
        str = str += std::to_string ( p.tm_hour ) += ":";
       }
       if ( p.tm_min < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_min ) += ":";
       } else {
        str = str += std::to_string ( p.tm_min ) += ":";
       }
       if ( p.tm_sec < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_sec );
       } else {
        str = str += std::to_string ( p.tm_sec );
       }
     } else if ( p.tm_min > 0 ) {
      str = "00:";
      if ( p.tm_min < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_min ) += ":";
      } else {
        str = str += std::to_string ( p.tm_min ) += ":";
      }
      if ( p.tm_sec < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_sec );
      } else {
        str = str += std::to_string ( p.tm_sec );
      }
     } else {
      str = "00:00:";
      if ( p.tm_sec < 10 ) {
        str = str.append ( "0" ) += std::to_string ( p.tm_sec );
      } else {
        str = str += std::to_string ( p.tm_sec );
      }
    }
    return str;
 }
\$\endgroup\$
  • 1
    \$\begingroup\$ I recommend you to wait with accepting an answer. Accepted answer usually discourages other reviewers to post their answers. Other reviews might be better (I'm not saying they will), may fill some gaps that others didn't find and many more. \$\endgroup\$ – Incomputable Aug 19 '16 at 19:07
1
\$\begingroup\$

Try this. It uses a string-based stream to do formatted output. Let the stream put in leading zeroes. When you're done with your output, the str() method returns (a copy of) the embedded string.

#include <time.h>
#include <sstream>
#include <iomanip>

std::string eta_string ( int64_t s )
{
    time_t seconds ( s );
    tm p;
    gmtime_s ( &p, &seconds );

    std::ostringstream os;

    if ( p.tm_yday > 0 )
        os << p.tm_yday << (p.tm_yday == 1 ? " day " : " days ");

    os << std::setfill('0') << std::setw(2) << p.tm_hour << ':' <<
        std::setfill('0') << std::setw(2) << p.tm_min << ':' <<
        std::setfill('0') << std::setw(2) << p.tm_sec;

    return os.str();
}
\$\endgroup\$
1
\$\begingroup\$

I'll start by warning that I'd use some utility functions/classes I have around to make some things easier. If you look at all their code, this looks big and complex (probably more so than the original code).

One of those is the infix_ostream_iterator I've posted previously. Second is a small type to handle formatting in conjunction with an ostream_iterator (or an infix_ostream_iterator, as the case may be). It looks like this:

#ifndef FMT_H_
#define FMT_H_

namespace {
    // A numeric type that carries formatting information, so all items of a specified 
    // type will be formatted in that way when written to a stream.
    template <int width, int prec, class T = int, int base = 10>
    class fmt {
        T const &i;
    public:
        fmt(T const &i) : i(i) {}

        friend std::ostream &operator<<(std::ostream &os, fmt const &f) {
            return os << std::setfill('0') 
                      << std::setw(width) 
                      << std::setprecision(prec) 
                      << std::setbase(base) 
                      << f.i;
        }
    };
}

#endif

Although header guards (and such) add quite a bit of bulk, the basic idea here is pretty simple: you instantiate an ostream_iterator over it, and in that instantiation you specify the width and precision you want used.

As I said, if we look at all the code for these, they add a lot of bulk. On the other hand, if we look at them as just:

#include "infix_iterator.h"
#include "fmt.h"

They're really pretty simple. Anyway, let's now consider the rest of the problem.

As for the rest of the problem, I don't really approve of using gmtime (or any of its variants) for this task. It's really asking it to do something it was never designed to do. It depends on characteristics that are probably required by POSIX but certainly are not required by either the C or C++ standards (which, at least as I see things means: "they aren't portable").

Therefore, I'd compute the number of days/hours/minutes/seconds on my own, and format the result. That intermediate result doesn't strike me as generally useful in this case, so I'd prefer to keep it reasonably private--but at the same time, I'd strongly prefer to separate that computation from formatting and displaying the result.

To accomplish both those, I'd wrap the formatting up into a small class, with the splitting routine made private (though if you see the splitting as valuable in itself, making it public wouldn't necessarily be terrible).

That means a class definition something on this order:

class format {   
    std::deque<int> split(int seconds);
public:
    format(int seconds);
};

That leaves only the implementation of the two member functions. Split might look something like this:

std::deque<int> format::split(int seconds) {
    static const std::vector<int> factors{ 60, 60, 24 };

    std::deque<int> results;

    for (auto const &f : factors) {
        results.push_front(seconds % f);
        seconds /= f;
    }
    results.push_front(seconds);
    return results;
}

...and operator() could look something like this:

std::string format::operator()(int seconds) {
    static const char *plural[] = { "s ", " " };
    std::ostringstream ret;

    auto data = split(seconds);

    if (data[0])
        ret << data[0] << " day" << plural[data[0] == 1];

    std::copy(data.begin() + 1, data.end(), 
              infix_ostream_iterator<fmt<2,0>>(ret, ":"));
    return ret.str();
}

To test this, we might use code something on this order:

int main() {    
    std::vector<int> tests{ 
        1, 
        59, 
        60, // one minute
        61, // one minute, one second
        120,    // two minutes
        121,    // two minutes, one second
        3600,   // one hour
        3601,   
        3661,   // one hour, one minute, one second
        7201,   // two hours, one second
        25 * 3600 + 60 + 1, // 1 day, 1 hour, 1 minute, 1 second
        2 * 24 * 3600 + 3 * 3600 + 4 * 60 + 5   // two days, 3 hours, 4 minutes, 5 seconds
    };

    // Run all the tests:
    std::transform(tests.begin(), tests.end(),
        std::ostream_iterator<std::string>(std::cout, "\n"),
        format());
}

Note that here I use "test" kind of loosely--it doesn't attempt to automatically verify the results, just display them for you.

There is a fair amount of code involved, it's broken up into small pieces, each of which has a fairly clearly defined purpose, and is fairly easy to test, verify and understand (I think, anyway). A fair amount of that code corresponds to things your original didn't really include. The part that does the same is basically just format::operator(), which really is pretty short and simple.

Personally, I'm a lot less concerned about the total amount of code than I am about the simplicity of the code, at least within some degree of reason, anyway. At some point, sheer size can become its own problem, but we're not really approaching that point here (again, at least in my opinion).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.