Review my time class

[Piotr_13]

I hope it's right place for it, if no - sorry, please tell me where I should place it.

I wrote simple time class, could you review it, tell me what I did good, what wrong, and how I can fix it?

I excluded things like include guards, namespaces, include directives...

header file:

 class time {
            private:
                unsigned miliseconds, seconds, minutes, hours;
            public:
                // default constructor
                time() : miliseconds(0), seconds(0), minutes(0), hours(0) {}

                // constructor which takes 4 arguments and sets class' variables
                explicit time(const unsigned, const unsigned = 0, const unsigned = 0, const unsigned = 0);

                // sets variables to given values
                void Set(const unsigned hrs = 0, const unsigned mins = 0, const unsigned secs = 0, const unsigned milisecs = 0) { *this = time(hrs, mins, secs, milisecs); }

                // adds hours
                void AddHour(const unsigned hrs) { hours += hrs; }

                // adds minutes
                void AddMin(const unsigned mins) { minutes += mins; hours += minutes / 60; minutes %= 60; }

                // adds seconds
                void AddSec(const unsigned secs) { *this += time(0, 0, secs); }

                // adds milisecs
                void AddMsec(const unsigned milisecs) { *this += time(0, 0, 0, milisecs); }

                // subtracts hours
                void SubHour(const unsigned hrs) { *this -= time(hrs); }

                // subtracts minutes
                void SubMin(const unsigned mins) { *this -= time(0, mins); }

                // subtracts seconds
                void SubSec(const unsigned secs) { *this -= time(0, 0, secs); }

                // subtracts milisecs
                void SubMsec(const unsigned milisecs) { *this -= time(0, 0, 0, milisecs); }

                // returns miliseconds
                unsigned GetMseconds() const { return miliseconds; }

                // returns seconds
                unsigned GetSeconds() const { return seconds; }

                // returns minutes
                unsigned GetMinutes() const { return minutes; }

                // returns hours
                unsigned GetHours() const { return hours; }

                // returns time in miliseconds
                unsigned ToMsecs() const { return hours * 3600000 + minutes * 60000 + seconds * 1000 + miliseconds; }

                // compares if left time is bigger than right time
                bool operator>(const time& comp) const { return this->ToMsecs() > comp.ToMsecs(); }

                // compares if left time is smaller  than right time
                bool operator<(const time& comp) const { return this->ToMsecs() < comp.ToMsecs(); }

                // compares if left time is equal to right time
                bool operator==(const time& comp) const { return this->ToMsecs() == comp.ToMsecs(); }

                // compares if left time is not equal to right time
                bool operator!=(const time& comp) const { return this->ToMsecs() != comp.ToMsecs(); }

                // compares if left time is bigger or equal to right time
                bool operator>=(const time& comp) const { return this->ToMsecs() >= comp.ToMsecs(); }

                // compares if left time is smaller or equal to right time
                bool operator<=(const time& comp) const { return this->ToMsecs() <= comp.ToMsecs(); }

                // multiplies time by a value
                time operator*(const double multi) const { return time(0, 0, 0, this->ToMsecs() * multi + 0.5); }

                // divides time by a value
                time operator/(const double div) const { return time(0, 0, 0, this->ToMsecs() / div + 0.5); }

                // adds times
                time operator+(const time& add) const { return time(hours + add.hours, minutes + add.minutes, seconds + add.seconds, miliseconds + add.miliseconds); }

                // subtracts times
                time operator-(const time&) const;

                // multiplies time by a value
                time& operator*=(const double multi) { return *this = *this * multi; }

                // divides time by a value
                time& operator/=(const double div) { return *this = *this / div; }

                // adds times
                time& operator+=(const time& add) { return *this = *this + add; }

                // subtracts times
                time& operator-=(const time& sub) { return *this = *this - sub; }

                // multiplies value by a time
                friend time operator*(const double multi, const time& timeMulti) { return timeMulti * multi; }
            };

cpp file:

 time::time(const unsigned hrs, const unsigned mins, const unsigned secs, const unsigned milisecs) {
        miliseconds = milisecs % 1000;
        seconds = secs + milisecs / 1000;
        minutes = mins + seconds / 60;
        hours = hrs + minutes / 60;
        minutes %= 60;
        seconds %= 60;
    }

time time::operator-(const time& sub) const{
    if (*this <= sub)
        return time(0);
    return time(hours - sub.hours, minutes - sub.minutes, seconds - sub.seconds, miliseconds - sub.miliseconds);
}

[rubberman]

First, since I have implemented robust time classes several times in the past, I can say that you only want seconds, and microseconds as member variables. Forget the hours and other cruft. Next, always name your variables in your header functions, including your constructors. Your "explicit" constructor does not do so, and the users of this class may not have access to the source file implementation to see what each means. After all, you may have meant for hours to be the first parameter, or not... You can create a constructor that takes the time as hours/seconds/milliseconds, but inside the constructor you want to convert them to seconds and microseconds (or milliseconds if you prefer - it depends upon the accuracy of the clock as your application may need). You also want to be able to adjust for GMT (grenwich mean time drift). IE, if the input is in local time, do you want the time class to use GMT so you can easily adjust between time zones? So, you can have another member variable for the offset from GMT. Just remember, that some countries are not offset by a fixed number of hours, but sometimes by 1/2 hours (such as India). So, you might want that to be a floating point value, or a two part value of hours and minutes offset from GMT (don't forget the sign).

[Piotr_13]

Thank you for your reply. I saw some STL functions declarations, they have named variables, but I didn't think why. Now I know. I spotted an error in subtracting times. I gonna fix it. I don't want GMT variable, I didn't wrote it to store local time, but time of things like delay between parts of code or world record of tour de france(that's why there are hours and minutes). I might just make one class for hrs/mins/secs and second one for secs/milisecs. Do you think it'd be a better idea?