HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Mutual conversion operators

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
conversionmutualoperators

Problem


  • What is more readable? value / 60 / 60, value / 3600, or Hours{static_cast(*this).value / 60}? Should I use constants, i.e. SECONDS_IN_HOURS.



  • Should I use the explicit keyword? 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 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.



  • 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

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) :

#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.