Password Validation Class

Question

I created a class designed to validate passwords based on user defined criteria. Each PasswordValidator object objects a minimum and maximum password length, a character blacklist, as well as customizable Uppercase, Lowercase, and digit requirements. I am well aware that I could have used regex but I wanted to avoid using it for extra practice with classes. Here is an example object I created as well as the class.

var passwordValidator = new PasswordValidator
            {
                MinLength = 6,
                MaxLength = 13,
                UpperRequirement = 1,
                LowerRequirement = 1,
                DigitRequirment = 1,
                CharBlacklist = new char[] { 'T', '&' }

            };

            while (true)
            {
                var input = Console.ReadLine();
                if (input == "exit")
                {
                    break;
                }
                else
                {
                    Console.WriteLine(passwordValidator.CheckPassword(input));
                }
            }

    class PasswordValidator
    {
        
        public uint MinLength { get; init; }
        public uint MaxLength { get; init; }
        public uint UpperRequirement { get; init; }

        public uint LowerRequirement { get; init; }

        public uint DigitRequirment { get; init; }

        public char[]? CharBlacklist { get; init; }

        public bool CheckPassword(string password)
        {
            bool hasBlacklist = CharBlacklist != null;
            bool hasMaxLength = MaxLength != 0;

            // Length Checking
            if (password.Length < MinLength || (hasMaxLength && password.Length > MaxLength))
            {
                return false;
            }

            uint upperCount = 0;
            uint lowerCount = 0;
            uint digitCount = 0;

            foreach (char passwordChar in password)
            {
                if (hasBlacklist && !AllowedChar(passwordChar))
                {
                    return false;
                }
                if (char.IsUpper(passwordChar))
                {
                    upperCount++;
                }
                else if (char.IsLower(passwordChar))
                {
                    lowerCount++;
                }
                else if (char.IsDigit(passwordChar))
                {
                    digitCount++;
                }

            }

            if (upperCount < UpperRequirement || lowerCount < LowerRequirement || digitCount < DigitRequirment)
            {
                return false;
            }
            else
            {
                return true;
            }
        }
        private bool AllowedChar(char passwordChar)
        {
            foreach (var blacklistChar in CharBlacklist)
            {
                if (passwordChar == blacklistChar)
                {
                    return false;
                }
            }
            return true;
        }
    }

All types of feedback are appreciated but here are some questions I have about how I could improve my code.

1. My C# knowledge is fairly limited. The language I know the best is Java (0.5 years). Are there any language specific style issues you can see in my code? Did I get carried away with C# language features?
2. I chose not to create any constructors for my class in favor of object initializer syntax. I did this because All of the properties in the class are meant to be optional. Did I make the right move?
3. One design issue I see is that MinLength could be greater than MaxLength. How would I fix this?
4. My CheckPassword method is quite lengthy and contains a lot of if else statements. It also has two special cases where there is no MaxLength and where there is a Character Blacklist. What improvements (if any) should I make to that method.

Answer 1

You've mixed Requirement and Validator into one role. a Validator, should only read the requirements, and validate upon it, but in your class, you're setting the requirement from within the validator. So, what you need is to separate both Requirement and Validator given each one of them its own class. This would give more flexibility and better maintainability over them.

so, you should end-up having something like this :

public interface IPasswordRequirement { ... }

public class PasswordValidator
{
    private readonly IPasswordRequirement _requirement;
    // this constructor ensures this instance won't be initialized without providing a requirement.
    public PasswordValidator(IPasswordRequirement requirement)
    {
        _requirement = requirement;
    }
    
    public bool Validate(string password)
    {
        // validate the password using _requirement instance.
    }
}

Now, in the example above I've used an interface as baseline contract to validate upon. This would give you the ability to define different requirements such as (StrongPasswordRequirement, DefaultPasswordRequirement ..etc.), and can give you also the ability to expand the validator to accept multiple requirements at once (for instance, you can define three requirements, and validate the password against them, to see if the password matches any of them.).

CheckPassword should be renamed to ValidatePasswordRequirement, because CheckPassword by itself will give many meanings, such as checking the password hash, checking the password expiry, ...etc. All will go under password validity. So, it would be a good idea to tell which part of validation the method will process.

I chose not to create any constructors for my class in favor of object initializer syntax. I did this because All of the properties in the class are meant to be optional. Did I make the right move?

It's fine, as long as they're optional. You only need to consider the properties default values in your work and wither you need to use the system default value or use custom one. Also, if you're working with different .NET versions, you should consider a backward compatibility approach, where you would favor a default constructor over init only sitters.

One design issue I see is that MinLength could be greater than MaxLength. How would I fix this?

just validate them before initializing something like this :

private uint _minLength;
private uint _maxLength;

public uint MinLength 
{
    get => _minLength;
    init
    {
        if(_maxLength > 0 && value >= _maxLength)   
        throw new ArgumentOutOfRangeException(nameof(MinLength));
        
        _minLength = value;
    }
}

public uint MaxLength 
{
    get => _maxLength;
    init
    {
        if(value <= _minLength)
            throw new ArgumentOutOfRangeException(nameof(MaxLength));
        
        _maxLength = value;
    }
}

this way you would avoid overlapping between the two properties.

although, AllowedChar can be replaced with CharBlacklist.Contains(character)

your loop can be done like this :

foreach (char passwordChar in password)
{
    if (hasBlacklist && CharBlacklist.Contains(passwordChar))
    {
        return false;
    }
    else if (upperCount < UpperRequirement && char.IsUpper(passwordChar))
    {
        upperCount++;
    }
    else if (lowerCount < LowerRequirement && char.IsLower(passwordChar))
    {
        lowerCount++;
    }
    else if (digitCount < DigitRequirment && char.IsDigit(passwordChar))
    {
        digitCount++;
    }
}

this way, the upper, lower, and digit will only hit if their counter is less than their requirements, whenever any of them fulfil the requirement, it will be skipped in the upcoming iterations.

here is an example of the above points :

public interface IPasswordRequirement
{
     int MinimumLength { get; }
     int MaximumLength { get; }
     int NumberOfUpperCase { get; }
     int NumberOfLowerCase { get; }
     int NumberOfDigits { get; }
     char[]? BlacklistCharacters { get; }
}


/// this would be used as system default.
// then you can define another custom requirement
// that the consumer can customize, and use the default instance 
// if the consumer did not provide any custom requirement.
public class DefaultPasswordRequirement : IPasswordRequirement
{
    private static readonly _defaultBlacklistCharacters = new { 'T', '&' }
    public int MinimumLength { get; } = 6;
    public int MaximumLength { get; } = 13;
    public int NumberOfUpperCase { get; } = 1;
    public int NumberOfLowerCase { get; } = 1;
    public int NumberOfDigits { get; } = 1;
    public char[]? BlacklistCharacters { get; } = _defaultBlacklistCharacters;
}

public class PasswordRequirementValidator
{
    // create the default instance, and reuse it.
    private static readonly IPasswordRequirement _defaultPasswordRequirement = new DefaultPasswordRequirement();
    
    private readonly IPasswordRequirement _requirement;
    
    // when using the default constructor, it will apply the default requirement;
    public PasswordRequirementValidator() : this(_defaultPasswordRequirement) { }
    
    // use a custom requirement instead
    public PasswordRequirementValidator(IPasswordRequirement requirement)
    {
        if(password == null)
            throw new ArguementNullException(nameof(requirement));
        
        _requirement = requirement;
    }
    
    public bool Validate(string password)
    {
        if(string.IsNullOrWhiteSpace(password))
            return false;
        
        if(password.Length < _requirement.MinimumLength || password.Length > _requirement.MaximumLength)
            return false;
        
        
            uint upperCount = 0;
            uint lowerCount = 0;
            uint digitCount = 0;

            foreach (char passwordChar in password)
            {
                if (hasBlacklist && CharBlacklist.Contains(passwordChar))
                {
                    return false;
                }
                else if (upperCount < NumberOfUpperCase && char.IsUpper(passwordChar))
                {
                    upperCount++;
                }
                else if (lowerCount < NumberOfLowerCase && char.IsLower(passwordChar))
                {
                    lowerCount++;
                }
                else if (digitCount < NumberOfDigits && char.IsDigit(passwordChar))
                {
                    digitCount++;
                }
            }
            
        return upperCount >= NumberOfUpperCase && lowerCount >= NumberOfLowerCase && digitCount >= NumberOfDigits;
    }
}

also, you could see a way of replacing the bool to something more descriptive that would give more meaningful results.

Answer 2

Reviewed in no particular order, based on what I observed first.

---

The password configuration properties could do with a bit of clearer naming. This is a minor thing because the names you picked are definitely not bad, they're just not quite as descriptive as they could've been.

Coupled with this, I would be inclined to wrap all the configuration options in some kind of configuration object DTO, so it's easier to pass the combined configuration around.

Also note:

• I don't understand why you used a nullable array. I've removed this and instead opted for giving the blacklist an empty array as a default value. This ensures that you don't actually have to write any code to check if there is blacklist validation or not.
• Weirdly enough, you then didn't use nullability for MaxLength, which is an optional validation rule. Not that nullability is the best solution here, but it's where I would've expected you to explicitly make use of it if you already used it for the blacklist.
  • Instead, it's a more elegant solution to provide a default value of uint.MaxValue. Just like with the blacklist, this means that you can just write your validation logic without needing to double-check if this validation rule was set (as a given password will obviously have less than uint.MaxValue characters.

public class PasswordValidationConfiguration
{
    public uint MinimumPasswordLength { get; init; }
    public uint? MaximumPasswordLength { get; init; } = uint.MaxValue;

    public uint MinimumUppercaseCharacters { get; init; }
    public uint MinimumLowerCaseCharacters { get; init; }
    public uint MinimumDigitCharacters{ get; init; }
    public char[] Blacklist { get; init; } = Array.Empty<char>();
}

The reason for this is that configurations like this generally get mapped from a config file which is deserialized, meaning you tend to use a data-only structure to deserialize these settings; which is precisely what you'd use PasswordValidationConfiguration for.

---

CheckPassword has a few issues:

• A more apt name would be Validate. I'm dropping "password" from the method name because that's already obvious from the class name and the string password input parameter.
• It only returns a boolean, it doesn't actually tell you which rules were broken. While this is definitely workable and realistic (a lot of websites don't tell you the precise reason validation failed), I do think that having a character blacklist makes it very relevant to be able to tell the users that they violated this blacklist of arbitrary characters.
  • I'm going to implement validation messages here as it is a valuable feature for general purpose validation logic.
• It's quite a complicated method, and could've been subdivided.
• As mentioned earlier, you could've skipped the "has this validation option been chosen" logic for the blacklist and the max length.
• Iterating over the characters of the password once is indeed more performant than doing it once per validation check. However; this is a negligible difference, and it's generally more difficult to write and make sure that you didn't introduce a bug somewhere. I would opt for more clear methods that may end up iterating a few times. The performance hit is negligible, and this is not a piece of code that you're going to be running millions of times per second.

I'm going to address all of these topics at the same time. This is how I would've structured the code:

public class PasswordValidator
{
    private readonly PasswordValidationConfiguration configuration;

    public PasswordValidator(PasswordValidationConfiguration configuration)
    {
        this.configuration=configuration;
    }

    public (bool isValid, string[] validationErrors) Validate(string password)
    {
        var errors = new List<string>();

        if (password.Length < configuration.MinimumPasswordLength)
            errors.Add($"Password must at least be {configuration.MinimumPasswordLength} characters long");

        if(password.Length > configuration.MaximumPasswordLength)
            errors.Add($"Password cannot contain more than {configuration.MaximumPasswordLength} characters");

        if (password.Count(char.IsDigit) < configuration.MinimumDigitCharacters)
            errors.Add($"Password must contain at least {configuration.MinimumDigitCharacters} digits");

        if (password.Count(char.IsLower) < configuration.MinimumLowerCaseCharacters)
            errors.Add($"Password must contain at least {configuration.MinimumLowerCaseCharacters} lowercase characters");

        if (password.Count(char.IsUpper) < configuration.MinimumUppercaseCharacters)
            errors.Add($"Password must contain at least {configuration.MinimumUppercaseCharacters} uppercase characters");

        var blacklistedCharactersInPassword = configuration.Blacklist.Where(blacklistedChar => password.Contains(blacklistedChar));
        foreach (var blacklistedCharacter in blacklistedCharactersInPassword)
            errors.Add($"Password cannot contain character '{blacklistedCharacter}'");

        return (errors.Any(), errors.ToArray());
    }
}

You've fallen into the if(foo) return true; else return false; trap few time. This can at all times be rewritten as return foo; (or return !foo when you would otherwise return false in the if block).

I considered making submethods for the individual validation logic, but because these were all one-liners and I didn't want to overcomplicate the example, I didn't do it.
If you don't mind the added legwork, the Validate method would be easier to read if you abstracted both the validation logic and the related error message. A short example would be:

private bool IsTooShort(string password)
    => password.Length < configuration.MinimumPasswordLength;

private string TooShortMessage
    => $"Password must be at least {configuration.MinimumPasswordLength} characters long";

// In Validate method:

if(IsTooShort(password))
    errors.Add(TooShortMessage);

While it does add some lines to the class, it enhances the readability of the Validate method body, which can be very helpful when your validation logic becomes less than trivial.

I don't think it's quite necessary in the current case, but for more complex logic this can start making sense.

---

Overall, your code is good. It uses the language constructs in the way they were designed to be used and I could easily read your code's intentions. For a learner (which I assume you are since you were trying to get some practice with classes), this is a job well done.

My feedback here is mostly an insight in how this would've been developed in a professional setting. The changes I made (other than having error messages) did not change how the code worked, they simply refactored it into a shape that's more easy to make alterations to in the future.