Be careful with sample code

Found this code somewhere, it doesn't matter where:

1. public override bool ValidateUser(string strName, string strPassword)  
2. {  
3.     //This is the custom function you need to write. It can do anything you want.  
4.     //The code below is just one example.  
5.   
6.     // strName is really an email address in this implementation  
7.   
8.     bool boolReturn = false;  
9.   
10.     // Here is your custom user object connecting to your custom membership database  
11.     MyUserProvider oUserProvider = new MyUserProvider();  
12.     MyUser oUser = oUserProvider.FindUser(strName);  
13.   
14.     if (oUser == null)  
15.         return boolReturn;  
16.   
17.     // Here is your custom validation of the user and password  
18.     boolReturn = oUser.ValidateUsersPasswordForLogon(strPassword);  
19.   
20.     return boolReturn;  
21. }  

The comments are there because it's a sample, not because they should be there in production code, so that's fine. My problem is with the boolReturn variable.

1. bool boolReturn = false;  
2.   
3. // Here is your custom user object connecting to your custom membership database  
4. MyUserProvider oUserProvider = new MyUserProvider();  
5. MyUser oUser = oUserProvider.FindUser(strName);  
6.   
7. if (oUser == null)  
8.     return boolReturn;  

First of all, one rule (Code Complete should really be required reading for programmers) is that you want to keep assignments to a variable as close as possible to the place where the variable is used. The oUserProvider and oUser variables have nothing to do with the boolReturn, so let's switch the order:

1. // Here is your custom user object connecting to your custom membership database  
2. MyUserProvider oUserProvider = new MyUserProvider();  
3. MyUser oUser = oUserProvider.FindUser(strName);  
4.   
5. bool boolReturn = false;  
6. if (oUser == null)  
7.     return boolReturn;  

Ok... is the boolReturn variable used anywhere after the return? Not before it's reassigned. That means that, for all intents and purposes, the scope of the variable boolReturn ends here, and a *new* boolReturn variable is created. (Think about it.)

Let's rewrite the code accordingly (think it through to verify that nothing was changed in the meaning of the code):

1. public override bool ValidateUser(string strName, string strPassword)  
2. {  
3.     //This is the custom function you need to write. It can do anything you want.  
4.     //The code below is just one example.  
5.   
6.     // strName is really an email address in this implementation  
7.   
8.     // Here is your custom user object connecting to your custom membership database  
9.     MyUserProvider oUserProvider = new MyUserProvider();  
10.     MyUser oUser = oUserProvider.FindUser(strName);  
11.   
12.     bool boolReturn = false;  
13.     if (oUser == null)  
14.         return boolReturn;  
15.   
16.     // Here is your custom validation of the user and password  
17.     boolReturn2 = oUser.ValidateUsersPasswordForLogon(strPassword);  
18.   
19.     return boolReturn2;  
20. }  

Progress. This change makes visible another problem: why set a variable if you're only using it once? (Ok, it makes sense for complicated calculations, but it's not the case here.)

1. public override bool ValidateUser(string strName, string strPassword)  
2. {  
3.     //This is the custom function you need to write. It can do anything you want.  
4.     //The code below is just one example.  
5.   
6.     // strName is really an email address in this implementation  
7.   
8.     // Here is your custom user object connecting to your custom membership database  
9.     MyUserProvider oUserProvider = new MyUserProvider();  
10.     MyUser oUser = oUserProvider.FindUser(strName);  
11.   
12.     if (oUser == null)  
13.         return false;  
14.   
15.     // Here is your custom validation of the user and password  
16.     return oUser.ValidateUsersPasswordForLogon(strPassword);  
17. }  

Frankly, I find the ?: operator to be a great help, so my code would tend to look like the next example, but I understand some people get confused by it, so take your pick:

1. public override bool ValidateUser(string strName, string strPassword)  
2. {  
3.     //This is the custom function you need to write. It can do anything you want.  
4.     //The code below is just one example.  
5.   
6.     // strName is really an email address in this implementation  
7.   
8.     // Here is your custom user object connecting to your custom membership database  
9.     MyUserProvider oUserProvider = new MyUserProvider();  
10.     MyUser oUser = oUserProvider.FindUser(strName);  
11.   
12.     return oUser == null  
13.         ? false  
14.         : oUser.ValidateUsersPasswordForLogon(strPassword);  
15. }  

Finally, what's up with the (bad) Hungarian notation? The fact that the name (or email, whatever) is a string or something else is relevant for the compiler, not for the programmer. The good Hungarian notation can be useful, but most people seem to only know about the bad variant. Let's get rid of it altogether:

1. public override bool ValidateUser(string name, string password)  
2. {  
3.     var userProvider = new MyUserProvider();  
4.     var user = userProvider.FindUser(name);  
5.   
6.     return user == null  
7.         ? false  
8.         : user.ValidateUsersPasswordForLogon(password);  
9. }  

To me, this looks much better than the original. Doesn't it?