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

Validating C++ Time class objects

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

Problem

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 
#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 
#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 =0 && m =0 && s <=

Solution

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";

Code Snippets

private :
    int hour;
    int minute;
    int second;
private:
    int secondsSinceMidnight;
Time(const int h = 0, const int m  = 0, const int s = 0);    //with default value
Time();  // Current time.
    explicit Time(const int h, const int m  = 0, const int s = 0);
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);

Context

StackExchange Code Review Q#51038, answer score: 5

Revisions (0)

No revisions yet.