4
\$\begingroup\$
  1. What is more readable? value / 60 / 60, value / 3600, or Hours{static_cast<Minutes>(*this).value / 60}? Should I use constants, i.e. SECONDS_IN_HOURS.
  2. Should I use the explicit keyword? Would it actually prevent stupid mistakes/silent bugs? Or would it become annoying to have to cast everything?
  3. It is easy to "glaze" over the following code and miss mistakes (for example, mixing up multiplication and dividing, using 24 instead of 60 when converting hours to minutes). How should it be refactored? I'm not sure if toSeconds, toMinutes is better than Second::operator Minutes() in terms of clarity.
  4. What happens if I add days, and then weeks, and so on? It would require duplicating code and keeping track of what I have already and haven't already implemented. This is basically an extension to number 3 in terms of refactoring: how should I define them in terms of the other to avoid duplication? 

#include <iostream> struct Seconds; struct Minutes; struct Hours; struct Seconds { int value; operator Minutes() const; operator Hours() const; }; struct Minutes { int value; operator Seconds() const; operator Hours() const; }; struct Hours { int value; operator Minutes() const; operator Seconds() const; }; Seconds::operator Minutes() const { return Minutes{value / 60}; } Seconds::operator Hours() const { return Hours{static_cast<Minutes>(*this).value / 60}; } Minutes::operator Seconds() const { return Seconds{value * 60}; } Minutes::operator Hours() const { return Hours{value / 60}; } Hours::operator Minutes() const { return Minutes{value * 60}; } Hours::operator Seconds() const { return Seconds{static_cast<Minutes>(*this).value * 60}; } int main() { Minutes m{60}; Seconds s{3600}; Hours h{1}; std::cout << (static_cast<Seconds>(m).value == s.value); std::cout << (static_cast<Seconds>(h).value == s.value); std::cout << (static_cast<Minutes>(s).value == m.value); std::cout << (static_cast<Minutes>(h).value == m.value); std::cout << (static_cast<Hours>(s).value == h.value); std::cout << (static_cast<Hours>(m).value == h.value); return 0; }
\$\endgroup\$
2

3 Answers 3

Reset to default
5
\$\begingroup\$

What you done looks good but I am wondering if it is not a bit over-engineered.

Here's what I would have done (which is probably under-engineered) :

#include <iostream> enum TimeUnit { Second = 1, Minute = Second*60, Hour = Minute*60, Day = Hour*24, Week = Day*7 }; class Time { public: Time(int n = 0, TimeUnit unit = Second) : value(n*unit) {}; bool operator==(const Time& other) const { return (value==other.value); } int convert(TimeUnit unit = Second) const { return value/unit; } private: const int value; // Assumed to be in seconds. }; int main() { Time s0; Time h0(0,Hour); std::cout << (s0 == h0); Time s3600(3600); Time s3600bis(3600, Second); Time m60(60, Minute); Time h1(1, Hour); std::cout << (s3600 == s3600bis && s3600bis == m60 && m60 == h1); return 0; }

My point is to just take into account the fact that a time is just a value and a unit. We store the value in the easiest format and we convert it on demand. No conversion is actually required for comparison. Adding a new Unit is straightforward (assuming there is a constant factor to multiply - month and year wouldn't work because they do not always last the same time).

\$\endgroup\$
1
\$\begingroup\$

I think you are right on track because your code has pretty much the same interface as as the Standard Library header <chrono>. Just replace all your classes with #include <chrono>, your static_cast to duration_cast and .value to count() and you are all set:

#include <iostream> #include <chrono> int main() { using namespace std::chrono; minutes m{60}; seconds s{3600}; hours h{1}; std::cout << (duration_cast<seconds>(m).count() == s.count()); std::cout << (duration_cast<seconds>(h).count() == s.count()); std::cout << (duration_cast<minutes>(s).count() == m.count()); std::cout << (duration_cast<minutes>(h).count() == m.count()); std::cout << (duration_cast<hours>(s).count() == h.count()); std::cout << (duration_cast<hours>(m).count() == h.count()); // Bonus std::cout << (s == m) << (m == h); }

Live Example.

The <chrono> implementation is a lot easier than your classes: it consists of a single class template 

template<class Rep, class Period> class duration<Rep, Period = std::ratio<1>>;

The Rep parameter is a signed integer that contains the count(), and the std::ratio is used to convert without loss of precision. You can also define your own types day, week or year this way. As you can so from my code example, you don't have to convert types to prove equality, just use == directly on the objects. duration_cast is only necessary to print stuff in different units, not for computations.

\$\endgroup\$
0
0
\$\begingroup\$

1) For readability, I'd go with explicitly using the single conversion constants instead of lumping them together: 

const int uptimeHours = uptimeMSecs / 1000 / 60 / 60;

Possibly off-topic, but I would probably do away with all these structs and auto-conversions entirely. If you use well named variables to keep track of the units, the conversion constants (60 secs to a minute etc.) are pretty straightforward.

\$\endgroup\$

You must log in to answer this question.