C++

Forum

 
Just looking for some feedback and advice on improving my program. Thank you!

NannyCool (13)
Hey, guys this is a finished (but not perfect) piece of coursework that I have had assessed. Just wanted to get some perspective on it really. Any feedback and advice on improvements, would be greatly appriciated. According to my lecturer, though my attempt was good for my level, on the whole it was more complicated than it had to be, ect. I lost most marks on my style (comments, ect). Thank you for taking the time to look at my work. Look forward to hearing from you.

Niranjan

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
 
//Niranjan Karki		-		15/12/08		-		Soft. Dev. Assign.
//A program that takes in a date and calculates the seconds that have passed
#include<iostream>

using namespace std;

int monthchk(bool leap, int mm);	//returns the number of days in a given month
int datecalc(int yy, int mm, int dd, int numleap);	//calculates the number of days passed, return in seconds
int leapcount(int yy, int mm, int dd);	//calculates the number of leap days to add
int timecalc(int hh, int min, int ss);	//converts and returns the given time in seconds

int main()
{
	bool invalid, leap;
	int yy, mm, dd, hh, min, ss, daylimit, numleap, dateresult, timeresult;

	cout<<"Hello, this program reads in a time and date, then calculates\n"
				<<"the number of seconds that have elapsed since 2000/1/1 00:00:00.\n";

	do //takes date, checks date is within correct range
	{
		cout<<"Please enter a date. (YYYY MM DD): ";
		cin>> yy >>mm >>dd;

		leap = ( ( ( yy % 4 == 0 ) && ( yy % 100 != 0 ) ) || ( yy % 400 == 0 ) ); //checks for leap year

		daylimit = monthchk(leap, mm);//returns and sets the max day in a month

		invalid = ( ( !( (yy >= 2000) && (mm <= 12) && (dd <= daylimit) ) ) ||  (mm < 1) || (dd < 1) ) ;

		if ( !(yy >= 2000) )	//error messages
		{
			cout<<"Year should be atleast 2000." <<endl;
		}
		else if ( !(mm <= 12) || ( mm < 1) )
		{
			cout<<"Month should be between 1 and 12." <<endl;
		}
		else if ( !(dd <= daylimit) || (dd < 1) )
		{
			cout<<"Days should be between 1 and " <<daylimit <<"." <<endl;
		}
	}
	while (invalid);

	do //takes time, checks time is within correct range
	{
		cout<<"Please enter a time. (hh:mm:ss): ";
		cin>>hh >>min >>ss;

		invalid = ( ( !( (hh < 24) && (min < 60) && (ss < 60) ) ) || (hh <= -1) || (min <= -1) || (ss <= -1) );

		if ( !(hh < 24) || (hh <= -1) )	//error messages
		{
			cout<<"Hours should be between 0 and 24" <<endl;
		}
		else if ( !(min < 60) || (min <= -1) )
		{
			cout<<"Minutes should be between 0 and 59" <<endl;
		}
		else if ( !(ss < 60) || (ss <= -1) )
		{
			cout<<"Seconds should be between 0 and 59" <<endl;
		}
	}
	while (invalid);

	numleap = leapcount(yy, mm, dd);	//calculates the number of leap days to add
	dateresult = datecalc(yy, mm, dd, numleap);	//calculates the number of days passed, return in seconds
	timeresult = timecalc(hh, min, ss);	//converts and returns given time to seconds

	if ( (dateresult + timeresult) > 1)	//corrects grammer, for 0, 1 or many seconds
	{
		cout<<"Between and 2000/1/1 00:00:00 and " <<yy <<"/" <<mm <<"/" <<dd <<" " <<hh <<":" <<min <<":" <<ss <<" "
				<<dateresult + timeresult <<" seconds have \nelapsed." <<endl; //displays result in seconds
	}
	else if ( (dateresult + timeresult) == 1)
	{
		cout<<"Between and 2000/1/1 00:00:00 and " <<yy <<"/" <<mm <<"/" <<dd <<" " <<hh <<":" <<min <<":" <<ss <<" "
				<<dateresult + timeresult <<" second has elapsed." <<endl; //displays result in seconds
	}
	else
	{
		cout<<"Between and 2000/1/1 00:00:00 and " <<yy <<"/" <<mm <<"/" <<dd <<" " <<hh <<":" <<min <<":" <<ss <<" "
				<<"no time has elapsed." <<endl; //displays result in seconds
	}

	return 0;
}

int monthchk(bool leap, int mm) //returns the number of days in a given month
{
	const int numofdays[13] = { 29, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; //Month 0 for leap feb with 29 days

	if ( (leap) && (mm == 2) )
	{
		return numofdays[0];
	}
	else
	{
		return numofdays[mm];
	}
}

int datecalc(int yy, int mm, int dd, int numleap)	//calculates the number of days passed, return in seconds
{
	const int numofdays[13] = { 29, 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 }; //Month 0 for leap feb with 29 days
	int dayinmonth = 0;
	int dayyears = (yy - 2000) * 365; //calculates the number of years that have passed then converts into days

	for(int month = 1; month < mm; month++)
	{
		dayinmonth = dayinmonth + numofdays[month];
	}

	return (dayinmonth + dayyears + (dd - 1) + numleap) * 86400;
}

int leapcount(int yy, int mm, int dd)	//calculates the number of leap days to add
{
	int currentyear = 2000;
	int leapcounter = 0;
	bool leap;

	do
	{
		if ( currentyear != yy)
		{
			leap = ( ( ( currentyear % 4 == 0 ) && ( currentyear % 100 != 0 ) ) || ( currentyear % 400 == 0 ) ); //checks for leap year

			if (leap)
			{
				leapcounter++;
			}
			currentyear++;
		}
	}
	while (currentyear < yy);

	leap = ( ( ( currentyear % 4 == 0 ) && ( currentyear % 100 != 0 ) ) || ( currentyear % 400 == 0 ) ); //checks for leap year

	if ( (currentyear == yy) && (leap) && (mm > 2) )
	{
		leapcounter++;
	}

	return leapcounter;
}

int timecalc(int hh, int min, int ss)	//converts and returns given time to seconds
{
	int hourtosec = hh * 60 * 60;
	int mintosec = min * 60;

	return hourtosec + mintosec + ss;
}
anders43 (125)
main() is a bit long, you shouldn't have functions with so many lines and variables it makes it difficult to read.

the way it is calculated is a bit cumbersome, you should instead rely more on the built-in functions for time/date instead - always get to know your libraries, whether it is boost, c-runtime library or STL.

it is good to have a comment at the start of each function telling what it is supposed to do, explaining what it returns and what it expects as input.

use assert() in your code to check for expected/unexpected values (programming errors not for user input)

always initialize your variables and declare them where they are used and not at top of the function.

the way u validate input is not good, try entering an invalid value and you will see (infinite loop). instead use getline to read the input, then parse the content and length of the input.

optionally display current date and time as default input

 
 
if ( !(hh < 24) || (hh <= -1) )


seems less readable than

 
 
if ( h < 0 || h > 23 ) ...



btw what is up with all these !(expressions) ?

e.g.

 
 
if ( !(yy >= 2000) )


it is much harder to read.

just 2c

NannyCool (13)
thanks, for the reply.

As to the !(expressions), come to think of it, when I wrote the program, I just went a little "not" crazy. I don't actually remember why, just at the time seemed like the thing to do. Although I see your point, I will try and incorperate that.

if ( yy < 2000)
{
//error messages
}

would be better than

if ( !(yy >= 2000) )
{
//error messages
}

got ya. Thank alot for the advice Anders.
Topic archived. No new replies allowed.