Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I created a Time class. Now I want to modify the code to perform input validation. Hour should be between 0-24, minutes and seconds between 0-59. If class need improvements, please offer suggestions.

Time.h Header File

#ifndef TIME_H
#define TIME_H

class Time
{
    private :
        int hour;
        int minute;
        int second;
    public :
        Time(const int h = 0, const int m  = 0, const int s = 0);    //with default value
        void setTime(const int h, const int m, const int s);        //  setter function
        void print() const;       // Print a description of object in " hh:mm:ss"
        bool equals(Time) const;     //compare two time object
};

#endif

Time.cpp implementation file

#include <iostream>
#include "Time.h"

Time :: Time(const int h, const int m, const int s) : hour(h), minute (m), second(s)
{}

void Time :: setTime(const int h, const int m, const int s) 
{
     hour = h;
     minute = m;
     second = s;     
}       

void Time :: print() const
{
    (hour < 10) ? std::cout << 0 << hour : std::cout << hour;
    std::cout << ":" ;
    (minute < 10) ? std::cout << 0 << minute : std::cout << minute ;    
    std::cout << ":" ;
    (second < 10) ? std::cout << 0 << second : std::cout << second ;
    std::cout << "\n" ; 
}

bool Time :: equals(Time otherTime) const
{
    if(hour == otherTime.hour && minute == otherTime.minute && second == otherTime.second)
        return true;
    else
        return false;
}

main.cpp

#include <iostream>
#include "Time.h"

int main()
{
   Time t1(10, 50, 59);
   t1.print();   // 10:50:59
   Time t2;
   t2.print(); // 06:39:09
   t2.setTime(6, 39, 9);
   t2.print();  // 06:39:09

   if(t1.equals(t2))
        std::cout << "Two objects are equal\n";
    else
        std::cout << "Two objects are not equal\n"; 

   return 0;
}

modified code of setTime for input validation (need suggestions)

void Time :: setTime(const int h, const int m, const int s) 
{
    if(h>=0 && h <=23)
        hour = h;
    else
        hour = 0;

    if(m>=0 && m <=59)
        minute = m;
    else
        minute = 0;

    if(s>=0 && s <=59)
        second = s;
    else
        second = 0;    
}       
share|improve this question
add comment

2 Answers 2

I don't like the internal storage type of (Hour/Min/Seconds).

private :
    int hour;
    int minute;
    int second;

This makes it hard to do comparison move forward backwards etc. I personally just store the number of seconds since midnight.

 private:
    int secondsSinceMidnight;

The converse to this is that usage may make this inefficient. If your main usage is just printing rather than modifying it may be worth keeping it as Hours/Minutes/Seconds (but I would measure before jumping to that conclusion as it is unlikely the main slowness in printing).

I don't like the default time being midnight.

    Time(const int h = 0, const int m  = 0, const int s = 0);    //with default value

If I construct a time object with no explicit value I usually expect it to be the current time. So personally I may default minutes and seconds to zero but I would expect at least the hours to be set. I would then provide a default constructor that sets the object to the current time:

    Time();  // Current time.
    explicit Time(const int h, const int m  = 0, const int s = 0);

Don't like Getters/Setters (they break encapsulation). Also I don't really see the use of this setter over the constructor.

    Time  x(5, 6, 7);

    // Now at a latter point I want to reset `x`;
    x = Time(8, 9, 10);

    // Performs equivalent action to:
    x.setTime(8, 9, 10);

When you provide a print method it is usally a good idea to also pass as an argument the stream you want to print on. This can default to std::cout but at least provide it.

    void print(std::ostream& str = std::cout) const;       // Print a description of object in " hh:mm:ss"

This also lets you define the standard output operator for the class very easily.

    std::ostream& operator<<(std::ostream& str, Time const& data)
    {
        data.print(str);
        return str;
    }

Now you can print using the method. Or the more more normal C++ technique.

    Time  x(5,7,8);
    x.print();              // defaults to std::cout
    x.print(std::cout);     // Or explicitly specify a stream.
    std::cout << x;         // Or just stream to the output.

Fine to have a named function (but pass parameter as const reference).

    bool equals(Time const& other) const;     //compare two time object

But why not define the equality operator as well. It makes the code more natural to read.

    bool operator==(Time const& lhs, Time const& rhs)
    {
         return lhs.equals(rhs);
    }

You don't need to manually put a zero into each field.

       (hour < 10) ? std::cout << 0 << hour : std::cout << hour;

There is a standard manipulator that does that automatically.

   std::cout << std::setw(2) << std::setfill('0') << h << ":"
             << std::setw(2) << std::setfill('0') << m << ":"
             << std::setw(2) << std::setfill('0') << s << "\n";
share|improve this answer
add comment

Validation

What should Time::Time do with out-of-range values? There are four options:

  1. Allow out-of-range values. They doesn't usually cause problems, so why go out of your way to prevent them?

  2. Make an equivalent time with in-range values. For example, Time(3, 77, 99)Time(4, 18, 39). This is what mktime does, and it's very useful, because it lets you do arithmetic on parts of times without worrying about overflow.

  3. Throw an exception. This is easy to understand, and ensures you won't ignore the bug, but it can force callers to do their own validation to avoid the exception.

  4. Silently make a different time instead. This is a bad idea. It's never useful, and it can hide bugs.

If this is a toy class to demonstrate validation, throwing an exception is fine. Any time class intended for practical use, however, should accept out-of-range values and convert them to equivalent in-range ones.

Beware: the seconds component can be 60 or 61 at the end of a day, because of leap seconds. 23:59:61 is a valid time, and is not necessarily equivalent to 00:00:01 – it depends on the date and the leap second schedule. Time is harder than it looks.

Style

As a general rule, complex objects like times should be passed by reference, not by value, to avoid unexpected copying.

The standard names for print and equals are operator<< and operator==.

The if (...) return true; else return false; in Time::equals is redundant.

These three changes produce the following equality operator:

bool Time::operator==(const Time &other) const {
    return hour == other.hour
        && minute == other.minute
        && second == other.second;
}

The conditional operator has lower precedence than comparison operators, so the parentheses in (hour < 10) ? ... are not needed. (C++ generally has convenient precedence.)

The conditionals in setTime (which do the wrong thing — see under Validation above):

if(m>=0 && m <=59)
    minute = m;
else
    minute = 0;

...could be written more simply as minute = (m >= 0 && m <= 59 ? m : 0);. (The parentheses are redundant but helpful to humans, who can't be expected to remember that = and ?: have the same precedence but are right-associative.)

share|improve this answer
    
The behaviour on validation failure depends on the purpose of the code. A database, for example, is expected to check for validity and consistency, but once the data is accepted, regurgitate it faithfully. For example, MySQL is often ridiculed for its lax validation of dates, coupled with other unexpected consequences of such leniency. –  200_success May 20 at 14:41
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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