working phone number validation code

[jpanther]

Thanks everyone for helping me find new and better ways of writing my code. I finally got a working code that validates phone numbers in the following format: ###-###-####. At first, I tried to use a function that validates and re-prompts if wrong, but that didn't work out so well. So I used a bool function to validate and a void with a reference parameter to re-prompt if wrong. Here is my working code. Thanks.

  //***************************************************************************
bool phoneValidation (string& phone)
{
	
	//variables
	bool invalidFormat = false;
	char dash = '-';
	
	if (phone.length() != 12)			//check for length
		invalidFormat = true;
		
    for( int count = 0 ; count < phone.length() ; ++count )		//loop to check each individual character
    {
    	if (!isdigit(phone[count]))								//if character is not a digit, go in loop
    	{
    		invalidFormat = true;
    		if (count==3 && phone[count] != dash || count==7 && phone[count] !=dash)	//if the 4th and 8th character is not a dash, go in loop
		    	invalidFormat = true;
		   	else
		    	invalidFormat = false;
		}
	}

	return invalidFormat;
}
//*************************************************************************
void phoneReprompt(bool& phoneStat, string& telephoneNum)
{
	//variables
	string phone;
	bool newValidation;
	
	while(phoneStat == true)			// if true than re-prompt question
	{
		cout << endl;
		cout << "ERROR! Invalid format (format: ###-###-####). Re-enter phone number: " << endl;
		cin  >> telephoneNum;
		
		phoneStat = phoneValidation (telephoneNum);		// call this function to validate again
	}
	
	return;
}

Here is where its being called from:

//*************************************************************
void pickingOptions(char selection, ifstream& inRentalData, apartmentInfo addApartment[], int& cnt)
{
	// variable
	int num;
    string phoneNumber;
    string newPhoneNumber;
    string numDelete;
    int locatedPhoneNum;
    float rentAmount;
    float status;
    char yesNo;
    bool inserted = false;
    bool deletedPhoneNum = false;
    bool phoneStatus;
	
	switch (selection)
	{
		case 'S':
			// Display all the apartment information on file
			displayRental(addApartment, cnt);
			break;
		case 'A':
			//loop to keep adding apartments until user is done
			cout << "---- The following 3 questions are for adding an apartment ----" << endl << endl;
			while(!inserted)
			{
				// phone number prompt and validation
				phonePrompt (PHONE_PROMPT, phoneNumber, cnt, inserted);			//user prompt for phone number
				phoneStatus = phoneValidation (phoneNumber);					//Phone number validation and returns bool value(false=correct / true=incorrect)
				phoneReprompt(phoneStatus, phoneNumber);						//phone number re-prompt. If false than pass through, if true than reprompt
				addApartment[cnt].phoneNum = phoneNumber;						//add the validated phone number to array
				
				// rent amount prompt and validation
				rentAndStatusPrompt (RENT_PROMPT, rentAmount, cnt, inserted);	//user prompt for rent amount
				rentValidation (rentAmount);									//rent amount validation
				addApartment[cnt].rent = rentAmount;							//adds validated rent amount to array
				
				// rental status prompt and validation
				rentAndStatusPrompt (STATUS_PROMPT, status, cnt, inserted);		//user prompt for rental status
				statusValidation (status);										//rental status validation
				addApartment[cnt].rentalStatus = status;						//adds rental status to array
			
				// prompt user to continue or not 
	 			cout << "---- Do you wish to add more apartments to the list (Y/N)? ----" <<endl;
	 			cin >> yesNo;
	 			yesNo = toupper(yesNo);
	 			if (yesNo == 'Y')
	 				inserted = false;
	 			else 
	 				inserted = true;
	 			// add counter to array
	 			cnt++;
	 		}
			break;
		case 'D': 
			displayPhoneNumber(addApartment, cnt);			//display the availble phone numbers
			numDelete = deletePhonePrompt();				//user prompt/read for phone number
			phoneStatus = phoneValidation(numDelete);		//validate Phone number
			phoneReprompt(phoneStatus, numDelete);			//reprompt if wrong, continue if correct
			locatedPhoneNum = findAndDeleteNum(numDelete, addApartment, cnt, deletedPhoneNum);	// find and delete phone number in the array
			displayDeleteMessage(numDelete, locatedPhoneNum, addApartment, cnt);				//display message that file was deleted or messafge that file not found
			break;				
	}
	return;
} 

[ChajusSaib]

You are new to c++ but when you get the time look into regular expressions, it'll make your life a whole lot easier.

[jpanther]

I'm sure there are a lot easier ways of doing what I just did. Eventually, I'll learn about those methods and it will make my life a lot easier. For us newbies that have to write code in this manner, this will be a good start.

Let me change a few lines of code, that I found was incorrect as I was doing more in-depth debugging.

bool phoneValidation (string& phone)
{
	
	//variables
	bool invalidFormat = false;
	char dash = '-';
	
	if (phone.length() != 12)			//check for length
	{
		invalidFormat = true;
		return invalidFormat;
	}
		
    for( int count = 0; count < phone.length(); count++ )			//loop to check each individual character
    {
    	if (!isdigit(phone[count]) && count !=3 && count !=7)		//checks all characters in phone if they are not digits, except count 3 and 7
    	{
    		invalidFormat = true;
    		return invalidFormat;
		}
		if (count==3 && phone[count] != dash || count==7 && phone[count] !=dash)	//if the 4th and 8th character is not a dash, go in loop
		{
			invalidFormat = true;
			return invalidFormat;
		}
		else
		    invalidFormat = false;
	}
	return invalidFormat;
}

I found out that I had to return each error check that turned out to be incorrect. This site is awesome!! The help I got here was absolutely valuable. Thank you very much.

[Kevin C]

I think that is not good enough. For example, the phone number with slashes instead of dashes would still pass as valid. And the prone number with all 12 digits would also be valid.

In short: The code for verifying the dashes is at the wrong place.

Also, at all three places, instead of:
invalidFormat = true;
return invalidFormat;

you can simply write:
return true;

[TheIdeasMan]

Hi,

Just a pedantic, 1 cent worth of advice :+)

Try to avoid double negatives coming from your variable names - rather than invalidFormat = false; , I would rather see validFormat = true;

This situation would be worse if you had several of them, then combined them in a conditional expression.

Hope this helps - even a little bit :+)

[TheIdeasMan]

Hi again,

Some other things:

In your switch statement, have each case call a function, and always provide a default: case to catch invalid input.

With functions, there is a sort of a rule that functions should not be more than 40 LOC - some even go for less. The body of a loop is a good candidate for a function call, or any compound statement (more than 1 LOC in braces)

Having function calls for each case also aids understanding.

This:
while(phoneStat == true)

can be more concisely written:

while(phoneStat)

Good Luck :+)

There is no point in having a return statement at the end of a void function. However, return can be handy to exit such a function early.

[jpanther]

Kevin C, thanks for your feedback. I went and checked it again and the program works correctly. Are you using my last updated phoneValidation function? I realize that I could use return true instead of setting it equal to a variable, but my instructor likes to see us use variables, probably to get us more familiar with passing variables. Thanks for the input Kevin C.

TheIdeasMan, thanks for your input, I totally see where you are coming from, I will try to avoid double negatives. It would make it easier to understand and follow. I added an 'E' to my switch:

		case 'D': 
			displayPhoneNumber(addApartment, cnt);				  //display the availble phone numbers
			numDelete = deletePhonePrompt();				  	  //user prompt/read for phone number
			phoneStatus = phoneValidation(numDelete);			  //validate Phone number
			phoneReprompt(phoneStatus, numDelete);				  //reprompt if wrong, continue if correct
			locatedPhoneNum = findAndDeleteNum(numDelete, addApartment, cnt, deletedPhoneNum);	// find and delete phone number in the array
			displayDeleteMessage(numDelete, locatedPhoneNum, addApartment, cnt);				//display message that file was deleted or messafge that file not found
			break;
		case 'E':
			cout << "Do you want to save to file (Y/N): ";		  	
			cin >> results;
			results = toupper(results);
			if (results == 'N') 
				cout << "Have a nice day. Goodbye!" << endl;
			else
				readToRentals (outRentalData, addApartment, cnt); //if user selection is 'Y' to save data to file
			break;				
	}
	return results;
} 

Are you talking about changing the case 'E' to " default: " instead or using default as a means of error checking if they don't use S,A,D, or E. Like:

		default:
			cout << "ERROR! An invalid selection was inputted." << endl;

Thank you for your input.

[TheIdeasMan]

Hi,

Yes, your last code snippet was what I had in mind.

With the switch cases, I would like see them contain a function call and nothing else. I know that seems to be a pain, but it really does tidy up the code :+)

Another thing about switches, one can set up an enum which holds all the options for the switch. Then turn on a compiler warning for when you don't supply a case for each item in the switch.

More variable names to fix up: is cnt contents or count, or something else ?

results is vague. Why is this variable returned at the end of the function? Doesn't it only apply to the E case ?

readToRentals if that writes to a file, shouldn't it be writeToRentalsFile ?

One other very small thing: If you format your code editor to replace tabs with spaces, it will display better when you post it here. This site expands tabs to the full 8 spaces, thus showing much more indenting than what was probably in the original.

Also, from a style POV, place your comments before the line the apply to, not at the end, that way I don't have to scroll right all the time.

Here is a web site with c++ best practices, and it contains entries relating to lots of different style guides. Note that the Google style guide is considered rather bad, the JSF Air Vehicle one is allegedly quite good. Some of the others might be good too, I haven't looked at all of them. One can Google a particular style guide, if they want to read the whole document.

http://www.codergears.com/QACenter/index.php?qa=questions