I have an if stmt which checks to see if a char value (upper or Lower) is equal to a value input by end user. My understanding was if you are using a char value in a conditional stmt you need to put single quotes around the char value. If that is correct than I could use some help figuring out why my if/else stmt is not working. It is ignoring the if stmt I think. Thanks for any help.

{
        //Variable declaration
    int AccountNumber; 
    char ServiceType; 
    int RegServiceMinutes = 0;
    int PremDayMinutes = 0;
    int PremNightMinutes = 0;
    double rAmount;
    double pAmount;
//	double AmountDue;
    cout << fixed << showpoint;                     
    cout << setprecision(2);                      

    cout << "This program calculates and prints a cellular phone bill" << endl;
   cout << endl;

    cout << "Enter account number: " ;  
    cin >> AccountNumber;                           
    cout << endl;
    cout << "Enter Service type: " 
        << "R or r (Regular Service) "
        << "P or p (Premium Service)  ";  
    cin >>  ServiceType;  
// cout << endl;

	if (ServiceType == 'r' || 'R')
	{ 
	cout << "Enter number of minutes used for Regular Service:  ";
	cin  >> RegServiceMinutes;
	}
	else
	{
	cout << "Enter number of day minutes used for Premium Service:  ";
	cin  >> PremDayMinutes;
	cout << endl;
	cout << "Enter number of night minutes used for Premium Service:  ";
	cin  >> PremNightMinutes;
	cout << endl;

Dani AI

Generated

The reason your condition always takes the first branch is short-circuit truthiness. In C++, the literal 'R' is a nonzero integer value once promoted; in a boolean context nonzero means true. So the expression is effectively (ServiceType == 'r') || true, which is always true. You were not ignoring the if; you were guaranteeing it. is right that you need two comparisons, but you can also avoid duplication by normalizing the case of the input once.

A simple, readable pattern is to fold the character to one case and compare once. Remember to cast to unsigned char before calling the cctype functions to avoid undefined behavior on platforms where char is signed.

#include <cctype>

char type = static_cast<char>(std::tolower(static_cast<unsigned char>(ServiceType)));

if (type == 'r') {
    // regular
} else if (type == 'p') {
    // premium
} else {
    // handle invalid input
}

If you prefer a switch, this keeps all valid options together and gives you a clean default path:

switch (std::toupper(static_cast<unsigned char>(ServiceType))) {
    case 'R': /* regular */ break;
    case 'P': /* premium */ break;
    default:  /* prompt again or error */ break;
}

Two small robustness tips inspired by the thread:

  • Accept exactly one letter and discard the rest of the line so stray characters do not bleed into later reads of numbers.
    std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
  • Provide a default branch to catch anything other than R/r or P/p and reprompt, which makes the behavior clearer than falling through to a wrong billing path.

This keeps the code readable (a point raised) while making the intent unambiguous and the input handling resilient.

Recommended Answers

All 9 Replies

You can not compare more than 1 value in a particular comparison statement. You use the logical operators to combine multiple comparisons to produce a net result.

not legal:
if (aValue == 1 || 2)

if (1 < aValue < 4)


legal:
if (aValue == 1 || aValue == 2)

if (1 < aValue && aValue < 4)


better legal:
if (1 == aValue || 2 == aValue)

Just reading trough this, but why is if(1 == aValue || 2 == aValue) better legal than the other (just legal) one?

Just reading trough this, but why is if(1 == aValue || 2 == aValue) better legal than the other (just legal) one?

It reduces the likelihood of accidentally using an assignment statement (a = 1) instead of an equality statement (a == 1). If you do attempt an assignment, the compiler will flag it as a syntax error because you are trying to modify a constant value.

start quote:

if (ServiceType == 'r' || 'R')
{ 
cout << "Enter number of minutes used for Regular Service:  ";
cin  >> RegServiceMinutes;
}
else
{
cout << "Enter number of day minutes used for Premium Service:  ";
cin  >> PremDayMinutes;
cout << endl;
cout << "Enter number of night minutes used for Premium Service:  ";
cin  >> PremNightMinutes;
cout << endl;

end quote.

ur missing a closing bracket "}" to end the else part of the statement

> It reduces the likelihood of accidentally using an assignment statement (a = 1) instead of an equality statement (a == 1)
Or it increases the likelyhood of making a mistake when you have (var1 == var2), and no amount of rearranging the code will save you.

Plus, a lot of people find such code to be highly unreadable.

Further, operand swapping zealots also extend this to the relational operators as well (<, > etc), where there is absolutely no value in doing so. I've seen people introduce BUGS into working code because they made a mess of it.

Most modern compilers will diagnose use of = in an if statement right off the bat, without having to mess with the code at all. And this includes the important edge case where rearranging fails.

http://c-faq.com/style/revtest.html
It's from a time long ago when compilers only had error messages, and blindly generated code so long as it was syntactically correct.
If you're still using such a compiler, consider upgrading.

you original if

if (ServiceType == 'r' || 'R')

should be changed to this

if (ServiceType == 'r' || ServiceType == 'R')

Thank you for all the information.

> It reduces the likelihood of accidentally using an assignment statement (a = 1) instead of an equality statement (a == 1)
Or it increases the likelyhood of making a mistake when you have (var1 == var2), and no amount of rearranging the code will save you.

Plus, a lot of people find such code to be highly unreadable.

Further, operand swapping zealots also extend this to the relational operators as well (<, > etc), where there is absolutely no value in doing so. I've seen people introduce BUGS into working code because they made a mess of it.

Most modern compilers will diagnose use of = in an if statement right off the bat, without having to mess with the code at all. And this includes the important edge case where rearranging fails.

http://c-faq.com/style/revtest.html
It's from a time long ago when compilers only had error messages, and blindly generated code so long as it was syntactically correct.
If you're still using such a compiler, consider upgrading.

I can see what you're saying. I used to have issues with that, then I saw the suggestion (here, on DaniWeb actually), so I ran with it. As my previous posts, and your link, demonstrate, it's really only useful on equality with a literal. There really is no point in using it in any other situation because it won't have any effect.

What compiler are you using that flags the operator error? As far as I know, I have a fairly modern compiler, but I don't think I've ever seen it reported before.

> What compiler are you using that flags the operator error? As far as I know, I have a fairly modern compiler

$ cat foo.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char* argv[]) {
  if ( argc = 1 ) {
    printf( "Mmm, interesting...\n" );
  }
  return 0;
}

$ gcc -Wall foo.c
foo.c: In function ‘main’:
foo.c:6: warning: suggest brackets around assignment used as truth value

$ cl /W4 /nologo foo.c
foo.c
foo.c(5) : warning C4100: 'argv' : unreferenced formal parameter
c:\temp\foo.c(6) : warning C4706: assignment within conditional expression

gcc (Ubuntu 4.3.3-5ubuntu4) 4.3.3
Visual Studio is 2008 vintage.

Be a part of the DaniWeb community

We're a friendly, industry-focused community of developers, IT pros, digital marketers, and technology enthusiasts meeting, networking, learning, and sharing knowledge.