patterncppMinor
Validating C++ Time class objects
Viewed 0 times
objectsclasstimevalidating
Problem
I created a
Time.h Header File
Time.cpp implementation file
main.cpp
modified code of
```
void Time :: setTime(const int h, const int m, const int s)
{
if(h>=0 && h =0 && m =0 && s <=
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
};
#endifTime.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).
This makes it hard to do comparison move forward backwards etc. I personally just store the number of seconds since midnight.
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.
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:
Don't like Getters/Setters (they break encapsulation). Also I don't really see the use of this setter over the constructor.
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.
This also lets you define the standard output operator for the class very easily.
Now you can print using the method. Or the more more normal C++ technique.
Fine to have a named function (but pass parameter as const reference).
But why not define the equality operator as well. It makes the code more natural to read.
You don't need to manually put a zero into each field.
There is a standard manipulator that does that automatically.
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 valueIf 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 objectBut 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 valueTime(); // 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.