Feedback for Time constructor

[techjohnny]

Another assignment, would like feedback. It works so far, but maybe somebody could break it?

main.cpp

 //
#include <time.h>
#include <iostream>
#include <string>
#include "time.h"
#include <sstream>
#include <string>

using namespace std;

void testoutputformats(Time &t);

int main()
{
	Time alarmTime(22,50,30);
	
	string alarmString;
	
	alarmString = alarmTime.get24HourFormat();

	cout << alarmString << endl;

	alarmString = alarmTime.get12HourFormat();

	cout << alarmString << endl;

	alarmTime.incrementSeconds(45);
	testoutputformats(alarmTime);

	alarmTime.incrementSeconds(200);
	
	alarmTime.incrementMinutes(63);
	
	alarmTime.incrementHours(24);
	
	alarmTime.incrementHours(139);
	testoutputformats(alarmTime);

	alarmTime = Time(6, 30, 0);
	alarmTime = Time(18, 30, 0);
	
    return 0;
}

void testoutputformats(Time &t)
{
	string tmpString;
	tmpString = t.get24HourFormat();
	cout << tmpString << endl;
	tmpString = t.get12HourFormat();
	cout << tmpString << endl;

}

time.h

#include <time.h>
#include <string>
#include <iomanip>
#pragma once
using namespace std;

class Time
{
public:
	Time();
	Time(int hr, int min, int sec);

	string get24HourFormat() const;
	// return 24-hour format (military time).
	string get12HourFormat() const;
	// return 12-hour format (AM/PM)

	int getSeconds() const;
	// return seconds in the time (0-60).

	void incrementSeconds(int n);
	// increment seconds (0-59), if more than 60 increment the minutes too.
	void incrementHours(int h);
	// increment hours (0-23)
	void incrementMinutes(int m);
	// increment minutes (0-59)

private:
	int hr;
	int min;
	int sec;

	bool isValid(int hr, int min, int sec) const
	{
		return ((hr >= 0 && hr <= 23) && (min >= 0 && min <= 59) && (sec >= 0 && sec <= 59));
	}
	// Run a check for valid input.

};

time.cpp

#define _CRT_SECURE_NO_WARNINGS

#include <iostream>
#include <string>
#include <time.h>
#include "time.h"
#include <iomanip>
#include <sstream>

using namespace std;

Time::Time()
{
	
	time_t now;
	struct tm tstruct;

	time(&now);
	
	tstruct = *localtime(&now);

	hr = tstruct.tm_hour;
	min = tstruct.tm_min;
	sec = tstruct.tm_sec;

}
Time::Time(int hours, int minutes, int seconds) 
{
	
	if (isValid(hours, minutes, seconds))
	{
		hr = hours;
		min = minutes;
		sec = seconds;
	}
	else { 
		hr = 0;
		min = 0;
		sec = 0;

		cout << "Please enter a valid time" << endl;
	}
	
}

string Time::get24HourFormat() const
{
	ostringstream oss;
	oss << setfill('0');
	oss << setw(2) << hr << ":" << setw(2) << min << ":" << setw(2) << sec;

	string result = oss.str();

	return result;
}

string Time::get12HourFormat() const
{
	if (hr > 12)
	{
		ostringstream oss;
		oss << setfill('0');
		oss << setw(2) << hr -12 << ":" << setw(2) << min << ":" << setw(2) << sec << " PM";

		string result = oss.str();
		return result;
	}
	else if (hr == 00)
	{
		ostringstream oss;
		oss << setfill('0');
		oss << setw(2) <<  "12" << ":" << setw(2) << min << ":" << setw(2) << sec << " AM";

		string result = oss.str();
		return result;
	}
		
	else
	{
		ostringstream oss;
		oss << setfill('0');
		oss << setw(2) << hr << ":" << setw(2) << min << ":" << setw(2) << sec << " AM";

		string result = oss.str();
		return result;
	}
}


void Time::incrementHours(int h)
{
	if (h >= 0)
	{		
		int totalsecs = sec + 3600 * (hr+h) + 60 * (min);
		hr = (totalsecs / 60 / 60) % 24;
		min = (totalsecs / 60) % 60;
		sec = totalsecs % 60;
	}
}

void Time::incrementMinutes(int m)
{
	if (m >= 0)
	{
		int totalsecs = sec + 3600 * hr + 60 * (min+m);
		hr = (totalsecs / 60 / 60) % 24;
		min = (totalsecs / 60) % 60;
		sec = totalsecs % 60;
	}
}
void Time::incrementSeconds(int n)
{
	{
		int totalsecs = n + sec + 3600 * hr + 60 * min;
		hr = (totalsecs / 60/60) % 24;
		min = (totalsecs / 60) % 60;
		sec = totalsecs % 60;
	}
	
}
int Time::getSeconds() const
{
	return sec;
}

[gunnerfunner]

the overloaded ctor:

Time::Time(int hours, int minutes, int seconds) 

if(!isValid) you're still left with a Time object (0,0,0) which you probably don't want. So it's best to delete this ctor altogether and use isValid() to check the incoming data first and only then to pass the data to a setter method

for printing Time objects, since you're already using std::tm, you can also use std::put_time() passing a reference to a std::tm object initialized with your Time object data values:
http://www.cplusplus.com/forum/beginner/208986/#msg983688

Above link also shows how you could use std::duration_cast to overload addition and subtraction operators for Time objects and for the current increment methods in class Time

Incidentally the int's passed to the ctors and the increment methods could be const qualified

Finally, a bugbear of mine: http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice

[techjohnny]

Thank you for the response.

Where do I define the delete function for the constructor? Inside the class?

I was trying to put it into the else {} statement, but that is returning an error.

Instead of using namespace std; I'm using using std::cout

Thanks.

[dhayden]

get12HourFormat isn't right. It looks to me like 12:00:01 through 12:59:59 will be reported as AM instead of PM.

Consider creating a private normalize() function that will normalize the value:

void Time::noirmalize()
{
	int totalsecs = sec + 3600 * hr + 60 * min;
	hr = (totalsecs / 60/60) % 24;
	min = (totalsecs / 60) % 60;
	sec = totalsecs % 60;
}

Then, for example, incrementMinutes() becomes:

void Time::incrementMinutes(int m)
{
	if (m >= 0)
	{
		min += n;
		normalize();
	}
}

Generally it's a good idea to store the data in a format friendly to the computer rather than the human. Convert to a human-friendly format when you have to. Following that principle, I would have stored a single int to store the number of seconds. That makes the incrementXYZ() functions trivial. It also means the check for AM vs PM is trivial and you only have to convert to hours, minutes and seconds when the user wants it.

[gunnerfunner]

Where do I define the delete function for the constructor? Inside the class?

#include <iostream>

struct Time
{
    int hr;
	int min;
	int sec;

	Time(){}
	Time (const int& h, const int& m, const int& s)=delete;
};

int main()
{
    Time t;//OK
    Time u(1,2,3);//error: use of deleted function 'Time::Time(const int&, const int&, const int&)'|
}

[techjohnny]

Dhayden, thank you for the feedback. This is what is working, is there anything wrong with what I have now?


string Time::get12HourFormat()
{
if (hr > 12) {

string display = " PM";
ostringstream oss;
oss << setfill('0');
oss << setw(2) << hr -12 << ":" << setw(2) << min << ":" << setw(2) << sec << " " << display;

string result = oss.str();
return result;
}
else if (hr == 12)
{
string display = "PM";
ostringstream oss;
oss << setfill('0');
oss << setw(2) << "12" << ":" << setw(2) << min << ":" << setw(2) << sec << " " << display;

string result = oss.str();
return result;
}
else
{
string display = "AM";
ostringstream oss;
oss << setfill('0');
oss << setw(2) << hr << ":" << setw(2) << min << ":" << setw(2) << sec << " " << display;

string result = oss.str();
return result;
}

[dhayden]

Yup, that looks good.