Ugh...Address validation. Take some address strings, add to that a city, state, postal code, and country... make sure they are are all look syntatically 'valid' based on some business logic - it's not as easy as we'd hope to be able to handle EVERY possibility. But, no matter WHAT you come up with, I can guarantee that it's guaranteed to be much easier to digest than the block of validation code discovered by Mickey.
According to Mickey, there are a couple of special rules for this address validation that result in some of the WTF-ness. If an address is changed, and it's "close" to existing address, then the user needs to be prompted to confirm the address. And this prompt shouldn't take place unless the address itself has been changed. Wanna bet that the user specification for 'close' included the phrase 'you know what I mean'? That's probably the reason why the code doesn't actually address the idea of 'closeness'...the developers are still waiting for the definition.
Count the WTFs that you see in this code. There will be a quiz at the end of the article.
bool addrChanged =false; if (!this.isInsertMode) { var propCurrent = (from addr in dbContext.Addresses join prp in dbContext.Properties on addr.AddressID equals prp.AddressID where prp.PropertyID == CurrentPropertyID select addr).SingleOrDefault(); if (propCurrent == null) throw new Exception("Property not found."); int iStrNum; if (!int.TryParse(this.txtStreetNo.Text, out iStrNum)) throw new Exception("Street number entered is not a valid integer."); if (propCurrent.StreetNumber != iStrNum) addrChanged = true; if (!string.IsNullOrEmpty(txtStreetName.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.StreetName) && propCurrent.StreetName.ToLower() != txtStreetName.Text.ToLower()) || string.IsNullOrEmpty(propCurrent.StreetName)) { addrChanged = true; return; } } if (!string.IsNullOrEmpty(txtBuildingNo.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.BuildingNumber) && propCurrent.BuildingNumber.ToLower() != txtBuildingNo.Text.ToLower()) || string.IsNullOrEmpty(propCurrent.BuildingNumber)) { addrChanged = true; return; } } if (!string.IsNullOrEmpty(txtTrailerNo.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.TrailerNumber) && propCurrent.TrailerNumber.ToLower() != txtTrailerNo.Text.ToLower()) || string.IsNullOrEmpty(propCurrent.TrailerNumber)) { addrChanged = true; return; } } if (!string.IsNullOrEmpty(drpStreetSuffix.SelectedItem.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.Suffix.Suffix1) && propCurrent.Suffix.Suffix1.ToLower() != drpStreetSuffix.SelectedItem.Text.ToLower()) || string.IsNullOrEmpty(propCurrent.Suffix.Suffix1)) { addrChanged = true; return; } } if (!string.IsNullOrEmpty(txtSuite.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.Suite) && propCurrent.Suite.ToLower() != txtSuite.Text.ToLower()) || string.IsNullOrEmpty(txtSuite.Text)) { addrChanged = true; return; } } if (!string.IsNullOrEmpty(txtCity.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.City) && propCurrent.City.ToLower() != txtCity.Text.ToLower()) || string.IsNullOrEmpty(txtCity.Text)) { addrChanged = true; return; } } if (!string.IsNullOrEmpty(drpStates.SelectedItem.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.State) && propCurrent.State.ToLower() != drpStates.SelectedItem.Text.ToLower()) || string.IsNullOrEmpty(propCurrent.State)) { addrChanged = true; return; } } if (!string.IsNullOrEmpty(txtZipCode.Text) && !addrChanged) { if ((!string.IsNullOrEmpty(propCurrent.ZIPCode) && propCurrent.ZIPCode.ToLower() != txtZipCode.Text.ToLower()) || string.IsNullOrEmpty(txtZipCode.Text)) { addrChanged = true; return; } } } if (addrChanged) { // A modal confirmation dialog is shown and if the user cancels it, the update // is aborted. } if (this.isInsertMode || addrChanged) { // Update the Linq2Sql entity values and submit changes... code removed because there are // already too many WTFs }
So, how many did you find? For those keeping score, the following list is not necessarily comprehensive. What are the odds that more WTFs will be identified in the comment?
1. The inventive re-imagining of if-if logic. Because more curly braces make for better code.
2. The idea that clearing the existing value from a field doesn't result in the address being changed
3. The fact that any change to any one of the address fields results in the update being 'aborted' (by returning out of the method.
4. The fact that validation of the street number only takes place if the isInsertMode is true? No need to validate existing addresses, I guess.
5. Redundant checking for null or empty values. Consider the zip code validation. If the text box is empty, it never gets into the inner block. And once in the inner block, the text box can't be empty so there's no reason to include it in the OR clause.
5. Schizophrenic variable names (I'm thinking of you iStrNum. Are you a number? A String? An integer? Pick a side).
Okay, that last one isn't quite as bad. But would it really have been so hard to call the variable iStreetNumber. Or, just streetNumber (since Sherlock Holmes wouldn't be able to provide his address).
There are more that are possible, but were left off the list because they were easy. Mundane. Beneath the readers of TDWTF. For example, the existence of isInsertMode implies that there is an isUpdateMode counterpart. Otherwise the checks that result in addrChanged being set to true would never happen. The obvious lack of unit testing is another. But feel free to take your own shots. After all, that's what the comments are for.