Check assistant's password

Question

I have two more methods the same as this except that they query a different table. Is there another way to know if the result of the query is null? The hasher.CompareStringToHash() method is not allowed to have a null value.

If it's not null, it proceeds to an else condition, and inside that it checks if the encrypted password and password in the textfield are the same. If it's the same it will go to another else statement.

Is there another way to shorten this code? Keep in mind that I should not have a null value in the hasher.CompareStringToHash(). Or, if possible, restrict calling that method if the result is null and lessen the if statements. How can I make this code cleaner and more efficient?

public void CheckAssistantsPassword()
{
    DbClassesDataContext myDb = new DbClassesDataContext(dbPath);

    var password = (from userAccounts in myDb.Assistants
                    where userAccounts.Ass_UserName== txtUserName.Text
                    select userAccounts.Ass_Password).FirstOrDefault();

    var hasher = new Hasher() { SaltSize = 16 };

    if (password == null)
    {
        MessageBox.Show("Invalid Account");
    }
    else
    {
        bool isOkay = hasher.CompareStringToHash(txtPassword.Text,password);
        if (isOkay)
        {
            MessageBox.Show("You May Now Login");
        }
        else
        {
            MessageBox.Show("INVALID PASSWORD");              
        }
    }   
}

Answer 1

I believe you're looking for String.IsNullOrEmpty.

// test for null or empty string
if (!String.IsNullOrEmpty(password) {
}

// -- OR --

// If the hasher call just can't accept null values, you can specify
// an empty string to fall back on like so:
(...LINQ...).FirstOrDefault(String.Empty);

I would also probably refactor this a bit:

public void CheckAssistantsPassword()
{
  using (DbClassesDataContext myDb = new DbClassesDataContext(dbPath))
  {
    var password = (from userAccounts in myDb.Assistants
                    where userAccounts.Ass_UserName == txtUserName.Text
                    select userAccounts.Ass_Password).FirstOrDefault(String.Empty);

    var hasher = new Hasher { SaltSize = 16 };
    if (hasher.CompareStringToHash(txtPassword.Text, password))
    {
      // Success
    }
    else
    {
      // Invalid password
    }
  }
}

Optionally you can place error checking on your context making sure you can establish a connection. Also, I recommend placing a using() block around your context to free it up after you're done with it.

Answer 2

Your code looks logically OK to me. I have refactored it below which may or may not help.

private Hasher _hasher;

public Ctor(Hasher hasher) { _hasher = hasher; }

public bool IsPasswordValid(string pwd, Func<context, string> getHash, DbClassesDataContext context) {
    var hash = getHash(context);

    return hash == null ? false : _hasher.CompareStringToHash(pwd, hash);
}

• Password parameterised to improve testability and reduce responsibility of method
• Returns bool to reduce responsibility of method (another type can show the message box or whatever).
• Accepts query to remove need for the two other similar methods querying other tables and improve testability.
• Use of var to improve terseness (as requested), and readability.
• Injection of context to improve testability, remember to use using to ensure dispose is called.
• Magic number extraction to const for salt size to keep DRY.
• I modified the code directly in S/O, so high chance of typos!
• Method renamed to follow .NET convention surrounding methods returning booleans.
• Hasher injected into type ctor (ideally by IOC container) for improved testability.

Answer 3

I guess it's a matter of taste, but I would write your method like that :

public void CheckAssistantsPassword() {

    DbClassesDataContext myDb = new DbClassesDataContext(dbPath);
    var matches = (from userAccounts in myDb.Assistants
                    where userAccounts.Ass_UserName== txtUserName.Text
                    select userAccounts.Ass_Password)

    if (matches.Count() > 0)
    {
        var hasher = new Hasher() { SaltSize = 16 };
        var password = matches(0);

        if (hasher.CompareStringToHash(txtPassword.Text,password))
            MessageBox.Show("You May Now Login");
        else
            MessageBox.Show("Invalid Account");
    }
    else {
       MessageBox.Show("INVALID PASSWORD");
    }   
}

Then, as mentionned above, there's always the String.IsNullOrEmpty or String.IsNullOrWhitespace if you want to make sure your string is not made up of only blanks.

Answer 4

Ever since LINQ came out I've been talking about how the from ... in ... select syntax is really awful. It looks friendly at first but it completely hides what is really going on which is really useful to understand. Here is the much shorter way of writing it with the extension methods off of IQueryable along with a few other adjustements.

public void CheckAssistantsPassword() {

    DbClassesDataContext myDb = new DbClassesDataContext(dbPath);
    var hasher = new Hasher() { SaltSize = 16 };

    var userAccount = myDb.Assistants.FirstOrDefault(x=>x.Ass_UserName== txtUserName.Text);

    if(userAccount == null || userAccount.password == null) {
        MessageBox.Show("Invalid Account");
        return;
    }
    if(hasher.CompareStringToHash(txtPassword.Text,password) ;
      MessageBox.Show("You May Now Login");
    else
      MessageBox.Show("INVALID PASSWORD");              
}

A few notes,

• The my* naming convention is very visual-basic, you will never see it in C#
• I would probably move both the hasher and the db to a field on the class rather than creating them here.
• MessageBox.Show() is super-icky and hard to work with
• It seems like you are doing this directly in a codebehind form. That is generally going to become VERY hard to maintain. You might want to look into the Model-View-Presenter pattern but the basic idea is to put only your 'ui' logic in the code behind and any stuff like this into other classes. At the very least you do not want to have the same code that references ui elements (such as txtUserName) ALSO referencing database connections. It becomes a mess very quickly.

As for similar methods, as you start to understand the above format you can start to understand lambdas and expressions, you don't have to understand them very deeply, but you can learn that they are simply another syntax for creating certain types of objects. Therefore you could refactor to this (using a refactoring plugin like CodeRush/Refactor Pro or Resharper will help immensely):

public void CheckAssistantsPassword() {
  CheckSomeonesPassword(this.myDb.Assistants, x=>x.Ass_UserName== txtUserName.Text, x=>x.password);
}
public void CheckAssistantsPassword() {
  CheckBestFreindsPassword(this.myDb.Friends, x=>x.FriendType == "Best", x=>x.password);
}

private void CheckSomeonesPassword<ENTITY_TYPE>(IQueryable<ENTITY_TYPE> people, Expression<Func<ENTITY_TYPE, bool>> filter, Func<ENTITY_TYPE, string> getPassword) 
  where ENTITY_TYPE : class {
    var person = people.FirstOrDefault(filter);

    if(person == null || getPassword(person)) {
        MessageBox.Show("Invalid Account");
        return;
    }
    if(this.hasher.CompareStringToHash(txtPassword.Text,getPassword(password)) ;
      MessageBox.Show("You May Now Login");
    else
      MessageBox.Show("INVALID PASSWORD");              
}

** The above code is untested but it should be enough to get you on the right track **

Feel free to ask questions in the comments area

Answer 5

I know there are already many responses to this question, but here are a few other tweaks:

1 - Refactor the method to return a boolean result.. the method shouldn't really care about how you report errors, it should care about returning a simple result... true, the password's match, false, they don't:

public bool ValidatePassword(string username, string password)
{
  if (string.IsNullOrEmpty(username)) throw new ArgumentException("Username is required.", "username");
  if (string.IsNullOrEmpty(password)) throw new ArgumentException("Password is required.", "password");

  using (var context = CreateDataContext())
  {
    string hash = GetPasswordHash(context, username);
    var hasher = new Hasher { SaltSize = 16 };

    return hasher.CompareStringToHash(password, hash);
  }
}

2 - Throw an appropriate exception that represents the exceptional state. 3 - Separate out how you are creating your DbClassesDataContext into its own method, so should this need to be changed, it is changed in one place:

public static DbClassesDataContext CreateDataContext()
{
  return new DbClassesDataContext(_dbPath);
}

4 - User compiled queries where possible. If you know you might be performing a query multiple times, there is little sense in having the query provider generate the sql each time, you might as well take advantage of the CompiledQuery type:

private static readonly Func<DbClassesDataContext, string, IQueryable<string>> GetPasswordHash
  = CompiledQuery.Compile((DbClassesDataContext context, string username) => 
      context.Assistants.Where(a => a.Ass_UserName == username).Select(a => a.Ass_Password));

Other comments:

5 - Rename your data context type to something more applicable. I'm sure your app isn't called DbClasses? The naming of a type should be relevant, in terms of data contexts, you might want to name it after the database name. 6 - You're not storing the plain text password in the database are you? Only ever store the hash.

Importantly the changes above allow you to logically separate out the concerns of the original method, now your validate function should be more streamlined:

public void CheckAssistantsPassword()
{
  try 
  {
    if (!ValidatePassword(txtUserName.Text, txtPassword.Text))
      MessageBox.Show("INVALID PASSWORD");
  } 
  catch (ArgumentException ex) 
  {
    MessageBox.Show(ex.Message);
  }
}

Answer 6

Maybe try separating out your logic, from the display & control flow: for example:

public enum LoginState
{
    None,
    InvalidAccount,
    Valid,
    InavlidPassword
}

public LoginState CheckPassword(string password, string userPassword)
{
    if(string.IsNullOrEmpty(password))
        return LoginState.InavlidPassword;
    else
    {
        var hasher = new Hasher() { SaltSize = 16 };
        return hasher.CompareStringToHash(password, userPassword) ? LoginState.Valid : LoginState.InvalidAccount;
    }
}

And then using it like so:

var loginState = CheckPassword(txtPassword.Text, password);

switch (loginState)
{
    case InvalidAccount:
        MessageBox.Show("INVALID PASSWORD"); 
        break;
    case .... etc
}

Answer 7

I'd probably do it something like this:

public void CheckAssistantsPassword()
{
     DbClassesDataContext myDb = new DbClassesDataContext(dbPath);
     var password = (from userAccounts in myDb.Assistants
                     where userAccounts.Ass_UserName== txtUserName.Text
                     select userAccounts.Ass_Password).FirstOrDefault();

     var hasher = new Hasher() { SaltSize = 16 };
     if (hasher.CompareStringToHash(txtPassword.Text, password ?? String.Empty);
         MessageBox.Show("You May Now Login");
     else
         MessageBox.Show("INVALID PASSWORD");              
 }

The ?? operator returns the value on the left side if it is not null. Otherwise, it returns the value on the right side.