patterncppMinor
Mutual conversion operators
Viewed 0 times
conversionmutualoperators
Problem
- What is more readable?
value / 60 / 60,value / 3600, orHours{static_cast(*this).value / 60}? Should I use constants, i.e.SECONDS_IN_HOURS.
- Should I use the
explicitkeyword? Would it actually prevent stupid mistakes/silent bugs? Or would it become annoying to have to cast everything?
- It is easy to "glaze" over the following code and miss mistakes (for example, mixing up multiplication and dividing, using
24instead of60when converting hours to minutes). How should it be refactored? I'm not sure iftoSeconds,toMinutesis better thanSecond::operator Minutes()in terms of clarity.
- What happens if I add
days, and thenweeks, 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
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(*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(this).value 60};
}
int main()
{
Minutes m{60};
Seconds s{3600};
Hours h{1};
std::cout (m).value == s.value);
std::cout (h).value == s.value);
std::cout (s).value == m.value);
std::cout (h).value == m.value);
std::cout (s).value == h.value);
std::cout (m).value == h.v
Solution
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) :
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
Here's what I would have done (which is probably under-engineered) :
#include
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).Code Snippets
#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;
}Context
StackExchange Code Review Q#40336, answer score: 5
Revisions (0)
No revisions yet.