Phone number checker, issues w/ conditionals

[TomShoe]

Hello! I'm writing a telephone checker that's meant to look for a very specific US telephone number format. It goes like [phone]. It iterates through each number and symbol to check and make sure each symbol is correct, but when I insert a correct number, the program outputs "NO" then "YES" instead of just "YES". I'm not sure where the error is coming from, but I think it's something to do with checking for the parentheses.

Any help would be appreciated.

  #include <iostream>
#include <string>
#include <iterator>
#include <vector>
#include <cctype>

using namespace std;

int main()
{
	string pNum;
	cout << "Enter a phone number";
	cin >> pNum;
	string::iterator it = pNum.begin();
	int i = 1;

	if (pNum.length() != 13)
	{
		cout << "NO" << endl;
		system("pause");
		return 0;
	}

	while (it != pNum.end())
	{
		switch (i)
		{
		case 1:
		{
			if (*it =! "(")
			{
				cout << "NO" << endl;
				break;
			}
		}
		case 2:
		{
			if (isdigit(*it) || *it == 0)
			{
				cout << "NO" << endl;
				break;
			}

		}
		case 3:
		case 4:
		case 6:
		case 7:
		case 8:
		case 10:
		case 11:
		case 12:
		case 13:
		{
			if (isdigit(*it) == 0)
			{
				cout << "NO" << endl;
				break;
			}

		}
		case 5:
		{
			if (*it =!")")
			{
				cout << "NO" << endl;
				break;
			}
		}
		case 9:
		{
			if (*it =! "-")
			{
				cout << "NO" << endl;
				break;
			}
		}
		}	
		it++;
		i++;
	}
	if (i >= 13)
	{
		cout << "YES" << endl;
	}
	system("pause");
	return 0;
}

[dutch]

The "not equals" symbol is backwards here and a few other places:

if (*it =! "(")

Also, you need to use single quotes for a character, not double quotes. So it should be
And your "break"s should all be outside of the if blocks since you want to break the switch whether or not the if condition is true. So it should be:

		case 1:
			if (*it != '(')
				cout << "NO" << endl;
			break;

Also, you probably want to end the loop when you realize the answer is "NO". So it's probably best to set some kind of flag indicating to exit the loop instead of printing "NO". Then you can print NO or YES after the loop depending on that flag.

And I don't understand what you think *it == 0 is testing.

[jonnin]

one of the neat things about a switch is its fall-through capability. you can factor the cout<<no by letting it trigger any of the nos and fall into them. (Actually, this is about the only use I have for switches, is their ability to lump logic this way ... they are extremely limited for any other use case). You sort of did this with the empty cases but its not quite optimal.

this seems like a lot of code for what you want to do. maybe a regex test or two would make it much more concise? May be able to do it in one check? If not that, then 13 unrolled lines would do it, you have 80 lines to check 13 values.

bool good = (num.length() == 13);
good &= (num[0] == '(');
good &= isdigit(num[1]);
... etc, 14 lines I think?
then cout yes or no based off good's status.

This would not exit early when good became false, but with all those jumps removed, its probably more efficient, because the checks are all cheap (isdigit is likely inlined).

[JLBorges]

Let the data structure the code. As in most cases, the general problem is easier to solve.

#include <iostream>
#include <string>
#include <cctype>

int main()
{
    // char 'd' in the pattern stands for any decimal digit.
    const std::string pattern = "(ddd)ddd-dddd" ;

    std::string phone_number ;
    std::cout << "phone number [" << pattern << "]: " ;
    std::getline( std::cin, phone_number );

    bool valid = phone_number.size() == pattern.size() ;
    for( std::size_t i = 0 ; valid && i <  pattern.size() ; ++i )
    {
        if( pattern[i] == 'd' ) valid = std::isdigit( phone_number[i] ) ;
        else valid = phone_number[i] == pattern[i] ;
    }

    if(valid) std::cout << "YES\n" ;
    else std::cout << "NO\n" ;
}