<= Part 4 | Series Home | Part 6 =>
In our last post, we worked on allowing our user to enter new accounts - basically making our AddAccountForm usable. We were able to do that - we have a working form which our user can enter customers in to and have it populate the grid.
We have two issues to address right now. The first is that some of the information on the form is required - Particularly Name and Phone Number. In addition, the Phone Number field should be in the format of a phone number - both for entry and display purposes. The second issue that we have is that our Grid which is showing the users is not displaying the Name and Phone Number as the first fields:
Note that ZipCode and Address are the first two fields - and ZipCode is spelled just like that - it should say "Zip Code". Let's finish out the validation on the AddAccountForm and see if we can tackle the display issues.
First things first. We open up our solution and start by running all of our tests. 36/36 green - good to go!
Ok, so we talk to our customer about how he wants validation to work. When someone clicks Save on the AddAccountForm and hasn't filled out Name or Phone Number, then they should highlight Red and the form should not be saved. We ask what constitutes valid input - is a space Ok? He says that the Name field should contain some characters in it, and that the Phone Number field should be of the format (nnn) nnn-nnnn. We ask about extensions, and agree that they can go in that field. So the allowable characters for Phone Number are [0-9]()- and x. In addition, the max length for the phone number should be 25 characters. If they try to type in more than that, the UI should block them, truncating anything past 25.
Validating the Name field seems like a good starting spot to get us back into the swing. So, on to our first test. We know that we want to make sure that empty names can't make it into the data store - but where should that live? Recall that we have two ways to create an Account - via a constructor, or via the CreateAccount factory method. This means that we'd have to put the logic possibly in two places. One option around this would be to make the constructor private, and only allow Accounts to be created via Factory methods. We could then move validation logic into the property setters for Name and PhoneNumber. This seems like a good approach, so let's see what happens.
We go over to our Account.cs class and change the constructor to be private. We then lean on the compiler to see what breaks. Turns out it is only in one place - our Accounts.cs where we create the AccountStore, which is a fake right now anyway. In our test code, it breaks a lot of the tests - but those should be easy to change to a Factory method call. We go back and mark the constructor as public, because we have some more work to do, and we'll make these changes in small steps, changing the constructor back and forth and leaning on the compiler to tell us when we can leave it there for good.
However, what we need now is a test which drives out the factory method for taking in a name and phone number and returning an Account object. Over in our AccountTests class we see we have two tests - TwoAccountsWithMatchingNameAndPhoneAreEqual which uses the constructor, and FactoryMethodCreatesAccountWithCorrectInformation which uses, amazingly, the factory method. Looks like we can add a new test here:
[TestMethod]
public void TwoAccountsFromFactoryWithMatchingNameAndPhoneAreEqual()
{
Account account1 = Account.CreateAccount("Test User", "813-555-1234");
Account account2 = Account.CreateAccount("Test User", "813-555-1234");
Assert.AreEqual(account1, account2);
}
This doesn't compile because we need the factory method. So we create it:
public static Account CreateAccount(string name, string phoneNumber)
{
return new Account(name, phoneNumber);
}
Easy enough - we just delegate to the constructor for now. Even though we know we will be removing the constructor, we use it here. Now, let's mark our constructor as private again and see what we need to change.
The first thing is our dummy AccountStore we are setting up in Accounts. We change that to use the Factory Method and compile to run the tests. However, this reveals a host of other compile errors in our tests. So we change our constructor back to public, recompile, and rerun our tests. 37/37 passed.
We begin repeating this process - changing the constructor, compiling, picking a test to fix, changing the constructor back, and rerunning the tests. This insures that nothing is broken along the way. You could do a global search and replace for "new Account", but I prefer going through the code. Typically I changed a test class' worth of tests at a time, then ran the tests.
One interesting thing was in our AccountTests.cs. The test TwoAccountsWithMatchingNameAndPhoneAreEqual, when it was changed, matched line for line the TwoAccountsFromFactoryWithMatchingNameAndPhoneAreEqual test we added earlier. So we deleted it. Don't be afraid to delete tests that aren't useful anymore.
With everything compiling with the private constructor, and all our tests green, we can now tackle validating the property set. The first thing we need to do is force callers to not call the constructor but to set the properties explicitly. This is just a matter of creating an empty private constructor, which should only break the factory methods. For example, one of our factory methods now looks like:
public static Account CreateAccount(string name, string phoneNumber)
{
Account a = new Account();
a.AccountName = name;
a.PhoneNumber = phoneNumber;
return a;
}
With that change made, all of our tests now pass, and we are ready to setup the validation logic. Our first test should look like:
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithNameAsEmptyStringThrowsArgumentException()
{
Account a = Account.CreateAccount(String.Empty, "813-555-1212");
}
We run our tests, and it fails that the exception was not thrown. Let's fix that by checking the value in the AccountName setter.
if (name == String.Empty)
{
throw new ArgumentException("Account Name is required");
}
And our test...is still failing. Huh. We are calling CreateAccount in the test, which looks like:
public static Account CreateAccount(string name, string phoneNumber)
{
Account a = new Account();
a.AccountName = name;
a.PhoneNumber = phoneNumber;
return a; }
Ah. Stupid me. I'm looking at the value of name and I should be looking at value - meaning the value I'm setting. Name is actually null at this point. So changing the code to check value instead of name passes the test. That brings up another case we may need to think about, but before we go down any paths, let's make sure we didn't break anything. And...we did! AccountReadyForCreationFiredOnAddAccountClick is failing that AccountName is required.
What is happening is that we are testing that clicking on the button fires the right event. However, when we create the dummy form, we don't have any data on it. So when the click event tries to create an account, we pass an empty string to the factory method. It's a matter of changing the test from:
public void AccountReadyForCreationFiredOnAddAccountClick()
{
using (AddAccountForm addAccountForm = new AddAccountForm())
{
bool accountReadyForCreationFired = false;
addAccountForm.AccountReadyForCreation += delegate { accountReadyForCreationFired = true; };
addAccountForm.Show();
addAccountForm.AddAccount.PerformClick();
Assert.IsTrue(accountReadyForCreationFired);
}
}
To this:
public void AccountReadyForCreationFiredOnAddAccountClick()
{
using (AddAccountForm addAccountForm = new AddAccountForm())
{
addAccountForm.AccountName = "Test User";
addAccountForm.PhoneNumber = "813-555-1212";
bool accountReadyForCreationFired = false;
addAccountForm.AccountReadyForCreation += delegate { accountReadyForCreationFired = true; };
addAccountForm.Show();
addAccountForm.AddAccount.PerformClick();
Assert.IsTrue(accountReadyForCreationFired);
}
}
This passes our failing test, and now we have 37/37 green. Back to our validation concern. When we were testing the wrong variable, it wasn't throwing an exception on null. So let's fix that:
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithNullNameThrowsArgumentException()
{
Account a = Account.CreateAccount(null, "813-555-1212");
}
Which fails. So we'll make it pass by adding null to our check. Now we have a green test. We have one more check. What if we do the following:
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithNameOnlySpacesThrowsArgumentException()
{
Account a = Account.CreateAccount(" ", "813-555-1212");
}
It fails. So we need to trim the string, and we should probably check the length instead of comparing to String.Empty since that's really the preferred way of checking for empty strings:
if (value == null || value.Trim().Length == 0)
{
throw new ArgumentException("Account Name is required");
}
This passes our test, and we are at 39/39 green. Looking at the code (the "Refactoring" stage of red-green-refactor) we see that the conditions for validation may be a smell - especially because we'll likely have similar logic for PhoneNumber. But rather than assume, let's drive out that issue with tests.
I also want to take a moment here - this kind of validation testing would be ideal for FitNesse or FIT tests. That way our customer could run through all kinds of edge cases to test the behavior. However, since the cases our customer described from above were fairly straightforward, we'll leave them as unit tests for now.
Ok, on to the phone number. Remember that the format should be (nnn) nnn-nnnn plus a possible extension. However, all of our tests have been using "nnn-nnn-nnnn". Not cool.
However, that's Ok. Since Phone Number turns out to be a somewhat complex set of rules, it deserves its own class. We'll just simply hand the input off to the Phone Number class to validate and return the correctly formatted string. We can then add a method for parsing strings. First, let's capture null and empty string. The tests end up looking like:
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithPhoneAsEmptyStringThrowsArgumentException()
{
Account a = Account.CreateAccount("Test User", "");
} [TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithNullPhoneThrowsArgumentException()
{
Account a = Account.CreateAccount("Test User", null);
}
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithPhoneOnlySpacesThrowsArgumentException()
{
Account a = Account.CreateAccount("Test User", " ");
}
And we end up with similar logic:
if (value == null || value.Trim().Length == 0)
{
throw new ArgumentException("Phone Number is required");
}
Note that we didn't jump to a Phone Number object. Our tests haven't asked for it yet. In fact, we have two approaches we can take. The first is that we could let the user enter whatever they want - nnn-nnn-nnnn or (nnn) nnn-nnnn or even nnnnnnnnnn - and make sure that it gets displayed as (nnn) nnn-nnnn. For now, let's require it to be at least in the format (nnn) nnn-nnnn, and then we'll add a feature later for parsing them.
However, where that logic should exist is likely in the PhoneNumber class. So we should not be creating an account with two strings - we should be using a string and a PhoneNumber, that way the caller can figure out what is best. But, we've got a different story we are trying to finish up here, so let's get validation working, and then come back to this.
Right now, we've got the requirement that AccountName not be empty or null, and that PhoneNumber not be empty or null. Now, let's get the test in to validate that the phone number is in the right format:
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithBadFormattedPhoneThrowsArgumentException()
{
Account a = Account.CreateAccount("Test User", "813-555-1212");
}
Yes, we are starting with the test which breaks all of our code so far that has been using that format. We run the test, and it fails because the exception was not thrown. So let's fix that by using RegularExpressions. We add a reference to the System.Text.RegularExpressions namespace, and end up with this:
public string PhoneNumber
{
get { return phoneNumber; }
set
{
Regex regex = new Regex(@"\([0-9]{3}\) [0-9]{3}-[0-9]{4}");
if (value == null || value.Trim().Length == 0 || !regex.IsMatch(value))
{
throw new ArgumentException("Phone Number is required");
}
phoneNumber = value;
}
}
Which passes our test. However, does it let our valid phone number through?
[TestMethod]
public void CreatingAccountWithCorrectlyFormattedPhoneDoesNotThrowException()
{
Account a = Account.CreateAccount("Test User", "(813) 555-1212");
}
And, the test passes. Just to make sure:
[TestMethod]
[ExpectedException(typeof(ArgumentException))]
public void CreatingAccountWithGarbageFormattedThrowsArgumentException()
{
Account a = Account.CreateAccount("Test User", "(gh!) 1TT-**GW");
}
Which also passes. Now, let's run all of our tests. We have a spectacular failure - 10/45 passing. It looks like all of our code has been using the nnn-nnn-nnnn format. After a quick call to our customer, he says that he would likely enter it both ways, and to allow that way of inputting. We ask him how it should be displayed, and agreed that it should display as (nnn) nnn-nnnn no matter how the user entered it. So, let's write a test:
[TestMethod]
public void CreatingAccountAsnnn_nnn_nnnnWorks()
{
Account a = Account.CreateAccount("Test User", "813-555-1212");
}
This test fails, so let's fix it. We get it to pass by modifying our PhoneNumber property, which is quickly growing unwieldy:
public string PhoneNumber
{
get { return phoneNumber; }
set
{
//(813) 555-1212
Regex allowedRegex1 = new Regex(@"\([0-9]{3}\) [0-9]{3}-[0-9]{4}");
//813-555-1212
Regex allowedRegex2 = new Regex(@"[0-9]{3}-[0-9]{3}-[0-9]{4}");
if (value == null || value.Trim().Length == 0
|| (!allowedRegex1.IsMatch(value) && !allowedRegex2.IsMatch(value)))
{
throw new ArgumentException("Phone Number is required");
}
phoneNumber = value;
}
}
We want to get in there and clean that up, but there's one more thing we need to do. We should accept it as either format, but it should be stored as the former. So...
[TestMethod]
public void CreatingAccountWithnnn_nnn_nnnnPhoneStoresInProperFormat()
{
Account a = Account.CreateAccount("Test User", "813-555-1212");
Assert.AreEqual("(813) 555-1212", a.PhoneNumber);
}
Which fails. Let's get that passing:
public string PhoneNumber
{
get { return phoneNumber; }
set
{
//(813) 555-1212
Regex allowedRegex1 = new Regex(@"\([0-9]{3}\) [0-9]{3}-[0-9]{4}");
//813-555-1212
Regex allowedRegex2 = new Regex(@"[0-9]{3}-[0-9]{3}-[0-9]{4}");
if (value == null || value.Trim().Length == 0
|| (!allowedRegex1.IsMatch(value) && !allowedRegex2.IsMatch(value)))
{
throw new ArgumentException("Phone Number is required");
}
if (allowedRegex2.IsMatch(value))
{
string p1 = value.Substring(0, 3);
string p2 = value.Substring(4);
phoneNumber = String.Format("({0}) {1}", p1, p2);
}
else
{
phoneNumber = value;
}
}
}
Yes, this is nasty. Before we can fix it, let's run all of the tests. We've got some failures - 43/47 passing. The first test is SearchingByFullPhoneWhenMultipleAccountsReturnsOnlyMatchingAccount which searches for "813-555-1212". This search is performed in the FindAccounts method in the Accounts class, which delegates to the MatchesNameOrPhone method, which just matches the full string to the search criteria. It looks like we either need to change that on the fly, or have two representations of the phone number. Two of the other failing tests are doing the same thing - searching for the "nnn-nnn-nnnn", and the last is our test which says that entering the phone number as "nnn-nnn-nnnn" should fail. We remove that test (since it isn't true anymore) and focus on the other failing tests.
We can get the tests passing by modifying the find method like:
private bool MatchesNameOrPhone(Account account)
{
Regex alternateFormat = new Regex("[0-9]{3}-[0-9]{3}-[0-9]{4}");
string phoneNumber = currentSearchCriteria;
if (alternateFormat.IsMatch(currentSearchCriteria))
{
string p1 = currentSearchCriteria.Substring(0, 3);
string p2 = currentSearchCriteria.Substring(4);
phoneNumber = String.Format("({0}) {1}", p1, p2);
}
return account.AccountName.Contains(currentSearchCriteria)
|| account.PhoneNumber.Contains(phoneNumber);
}
Now that we have our tests passing, let's extract out a class for PhoneNumber and move some of this logic there. To do that, the first thing we'll do is modify the tests around validating the phone number to the PhoneNumber (and PhoneNumberTests) class. For example, CreatingAccountWithPhoneAsEmptyStringThrowsArgumentException becomes CreatingPhoneNumberWithPhoneAsEmptyStringThrowsArgumentException and gets moved to the PhoneNumberTests class. Note that we haven't removed the original test, we are just getting them ported over. We move the logic from the PhoneNumber property to the constructor, which gets all of the tests to pass except the display test. The original looked like:
[TestMethod]
public void CreatingAccountWithnnn_nnn_nnnnPhoneStoresInProperFormat()
{
Account a = Account.CreateAccount("Test User", "813-555-1212");
Assert.AreEqual("(813) 555-1212", a.PhoneNumber);
}
We modified it to:
[TestMethod]
public void CreatingPhoneNumberWithnnn_nnn_nnnnPhoneStoresInProperFormat()
{
PhoneNumber pn = new PhoneNumber("813-555-1212");
Assert.AreEqual("(813) 555-1212", pn.ToString());
}
We add the ToString method to just return phoneNumber, and add the local variable phoneNumber to store the result in. This passes our tests - our business logic is now working in PhoneNumber. Now we have to get everything ported over to use this. The first thing we can do is use it internally in Account.cs without having to have callers modify themselves yet. We modify the phoneNumber local variable in Account.cs to be of type PhoneNumber. We then change the PhoneNumber string property to:
public string PhoneNumber
{
get { return phoneNumber.ToString(); }
set
{
phoneNumber = new PhoneNumber(value);
}
}
Oooh. That looks much better. But do all of our tests still pass? And...they do. 53/53 green. The last step is to change CreateAccount to use PhoneNumber instead of a string. Or do we need to? In fact, let's go revisit the search issue from before. Remember that we had to duplicate the logic of the RegEx to get the searches for full phone numbers to work. The problem is that we only can have it one way or the other. If we want PhoneNumber to return the object, we have to modify the callers. If we don't, then we have to construct a phone number object, and then ask for it's alternate format. Since having the PhoneNumber as a string makes life easier for the UI, let's just construct a new PhoneNumber object in the search method.
But first, we need to add a new method to return the AlternateFormat. We could add a method to specify the format, but we'll just leave it as ToString and AlternateFormat. The test looks like:
[TestMethod]
public void AlternateFormatReturnsCorrectNumberFromnnn_nnn_nnnn()
{
PhoneNumber pn = new PhoneNumber("813-555-1212");
Assert.AreEqual("813-555-1212", pn.AlternateFormat);
}
And to pass it:
public string AlternateFormat
{
get
{
string p1 = phoneNumber.Substring(1, 3);
string p2 = phoneNumber.Substring(6);
return String.Format("{0}-{1}", p1, p2);
}
}
We also add a test to make sure it works using the other format. Now we can tackle search. With the new method, our search looks like:
private bool MatchesNameOrPhone(Account account)
{
PhoneNumber pn = new PhoneNumber(account.PhoneNumber); return account.AccountName.Contains(currentSearchCriteria)
|| pn.ToString().Contains(currentSearchCriteria)
|| pn.AlternateFormat.Contains(currentSearchCriteria);
}
Not too shabby. And all of our tests pass - 55/55. We can finally tackle the UI portion of this - which we'll do next time.
