if and else/if statements not being read

[maddimarrone]

Hi everyone -- I am currently trying to write a program that uses a function to display a greeting based on a time the user inputs in military time. So far I have a series of if and else-if statements. However, only my first if statement is being read and printed in the code's output. I am wondering if anyone would be able to explain to me why the rest of my code isn't being read.

Here is my code with my comments:

#include <iostream>
#include <string>
using namespace std;

string time2greeting(int time) // add the parameter here // computes the greeting at a certain time
{
// add logic here to return
string Greeting;
int timeOfgreeting;

// Good Morning from 400 to before noon.
if ((timeOfgreeting >= 400) || (timeOfgreeting <= 1159))
cout << "Good Morning, ";

// Good Afternoon from Noon to before 5PM
else if ((timeOfgreeting >= 1200) || (timeOfgreeting <= 1659))
cout << "Good Afternoon, ";

// Good Evening from 5PM to 11PM
else if ((timeOfgreeting >= 1700) || (timeOfgreeting < 2300))
cout << "Good Evening, ";

// What are you doing up, between 11PM and 4AM
else if ((timeOfgreeting >= 2300) || (timeOfgreeting <= 400))
cout << "What are you doing up, ";

//For illegal values, say:
else
cout << "That is an illegal time, ";

return (Greeting);
}

int main()
{
string name, greeting;
int time;

cout << "What is your name? ";
cin >> name;
cout << "What time is it? (0-2400)";
cin >> time;

// call and print time2greeting
greeting = time2greeting(time);
cout << greeting << " " << name << endl;

return(0);
}

[jlb]

First thing is that you're using the wrong logic. Because you're using logical OR if the first "test" evaluates to true the second "test" is not evaluated. I suggest you try to redo your logic to use only one "test" for the time frame. If you properly sequence the logic you only need one of the "tests".

Second thing is that you're using an uninitialized value in your tests. Perhaps you should be using the function parameter?

[maddimarrone]

So do you think I should use the logical AND instead? I'm not entirely sure what you mean. Also, should my variable in my tests then be "time" not "timeOfgreeting"?

[maddimarrone]

In case anyone runs into the same issue...I need to use the AND not OR except for the 11pm-4am window which is more complicated. That needs to be coded as:

else if ((time >= 2300 && time <= 2400) || (time >= 0 && time <= 400))

[Handy Andy]

Hello maddimarrone,

PLEASE ALWAYS USE CODE TAGS (the <> formatting button), to the right of this box, when posting code.

Along with the proper indenting it makes it easier to read your code and also easier to respond to your post.

http://www.cplusplus.com/articles/jEywvCM9/
http://www.cplusplus.com/articles/z13hAqkS/

Hint: You can edit your post, highlight your code and press the <> formatting button.

You can use the preview button at the bottom to see how it looks.

I found the second link to be the most help.

As jlb pointed out.

else if ((timeOfgreeting >= 1700) || (timeOfgreeting < 2300)) This is an || (OR) condition. Which means that only one side has to be true. When checking a range like this the && (AND) both sides have to be true. So else if ((timeOfgreeting >= 1700) && (timeOfgreeting < 2300)) And say that the time is 1800 it is both greater than 1700 and less than 2300.

Also, should my variable in my tests then be "time" not "timeOfgreeting"?

Looking at your function it takes one parameter (time) then you define int timeOfgreeting; which is uninitialized and contains a garbage value, whatever is in the memory location set aside for "timeOfgreeting" and tries to make an "int" out of this. This does not work as you may get a value of (-858993460) which could be different on your computer.

What you could have done is int timeOfgreeting = time;. This makes use of the parameter of the function. Or better is not define "timeOfgreeting" and just use "time". Most times it is better to use what you have and not do something you do not need.

else if ((time >= 2300 && time <= 2400) || (time >= 0 && time <= 400)). This does work, but all you really need is else if (time >= 2300 || time <= 400). Here the || (OR) works because of the two different times do not make a good range to check.

Based on what jlb said I came op with this, which may still be improved on, but works with the tests I did.

string time2greeting(int time) // add the parameter here // computes the greeting at a certain time
{
	// add logic here to return
	string greeting;
	//int timeOfgreeting = time;

	// What are you doing up, between 11PM and 4AM
	if (time > 2259 || time < 400)
		greeting = "What are you doing up, ";

	// Good Evening from 5PM to 11PM
	else if (time > 1659)
		greeting = "Good Evening, ";

	// Good Afternoon from Noon to before 5PM
	else if (time > 1159)
		greeting = "Good Afternoon, ";

	else if (time > 359)
		greeting = "Good Morning, ";

	//For illegal values, say:
	else
		greeting = "That is an illegal time, ";

	return greeting;
}

Line 5 I left to show you what you should have done, but this variable is not needed.

Next notice that the if/else if statements are in reverse order. Sometimes you have to do it this way to make it work out especially when only having one condition to check.

Next problem is that you defined "greetings", but never gave it a value before you returned the string. The "cout" statements should not be in the function, but back in "main" where you need it, so setting "greatings" to a different value makes more sense here.

Although you are free to name a variable any way that you want it is generally accepted that regular variables start with a lower case letter. Classes and structs start with a capital letter and variables defined as a constant use all capital letters. This is not a written rule, but what most people follow along with using "camelCase" with a name that is two or more words or using the underscore (_) to denote a space where a space is not allowed.

If you are still watching this it may help.

Andy