Evolving Existing Code through TDD: A Password Case Study

Yoan Thirion -

In Week 4 of our Summer Craft Book, which Yann Courtel and I created, we focus on Test-Driven Development with the exercise below.

The Initial Challenge

In this exercise, our task was to create a program that validates a password following Test-Driven Development cycles based on the following criteria:

  • At least 8 characters
  • At least one uppercase letter
  • At least one lowercase letter
  • At least one number
  • At least one special character from . * # @ $ % &
  • No other characters allowed

The objective was simple: determine if the password is valid.

The Implementation (Proposed Solution)

Here's the result source code proposed in our book:

namespace Password
{
    public sealed class Password : IEquatable<Password>
    {
        private readonly string _value;
        private Password(string value) => _value = value;

        private static readonly Seq<Func<string, Option<ParsingError>>> Rules =
            create<Func<string, Option<ParsingError>>>(
                input => Match(input, ".{8,}", "Too short"),
                input => Match(input, ".*[A-Z].*", "No capital letter"),
                input => Match(input, ".*[a-z].*", "No lower letter"),
                input => Match(input, ".*[0-9].*", "No number"),
                input => Match(input, ".*[.*#@$%&].*", "No special character"),
                input => Match(input, "^[a-zA-Z0-9.*#@$%&]+$", "Invalid character")
            );

        public static Either<ParsingError, Password> Parse(string input)
            => Rules.Map(f => f(input))
                .FirstOrDefault(r => r.IsSome)
                .ToEither(new Password(input))
                .Swap();

        private static Option<ParsingError> Match(string input, string regex, string reason)
            => !Regex.Match(input, regex).Success
                ? new ParsingError(reason)
                : None;

        ...
    }

    public sealed record ParsingError(string Reason);
}

In this implementation, we applied the Parse Don't Validate principle. This principle suggests converting input data into a validated type as early as possible, rather than leaving it as raw data and validating it at each use.

The "Parse Don't Validate" principle offers several benefits:

  1. Type Safety: Once parsed, we have a guarantee about the state of our data.
  2. Error Handling: Errors are handled explicitly and early.
  3. Simplification of Downstream Code: Code that uses the Password type doesn't need to worry about validation.
  4. Single Responsibility: The parsing logic is centralized in one place.

Let's break down the implementation:

  1. Return type Either<ParsingError, Password>:
    • We use an Either type that can contain either an error (ParsingError) or a valid password (Password).
    • This forces the calling code to explicitly handle both cases, avoiding silent errors.
  2. Private constructor:
    • Password has a private constructor, meaning you can't directly create a Password instance without going through the Parse method.
    • This ensures that every Password instance is valid.
  3. Static Parse method:
    • This method is the single entry point for creating a Password.
    • It applies all validation rules and returns either an error or a valid Password.
  4. Rule application:
    • We apply each rule (Rules.Map(f => f(input))) and stop at the first error encountered (FirstOrDefault(r => r.IsSome)).
    • If no errors are found, we create a new Password.
  5. No IsValid method:
    • Notice the absence of an IsValid method. Once a Password is created, we are certain it is valid.

The advantage of this approach is that the rest of our code can work with Password instances without worrying about their validity. We have "parsed" the input into a valid type right at the beginning, rather than validating it at each use.

Password Evolution

The purpose of our Advent Of Craft initiative is "To create a community of developers eager to learn from each other and improve together."

To facilitate this, we use a Discord server where Jonathan Roelinger (Padnom here) initiated an interesting discussion about evolving the implementation to handle a list of ParsingError instead of a single one.

Instead of continuing discussions through Discord, we decided to code it together during a Software Teaming session with Jonathan and Yann.

Software Teaming in progress...

Using T.D.D to Change Existing Code / Behavior

In this session we agreed that our objective was to extend the parsing method to return a list of parsing errors. This meant our code would evolve to look like this:

public static Either<Seq<ParsingError>, Password> Parse(string input) { ... }

Let's walk through how we used TDD to guide this evolution:

Step 1: 🔴 Start with a Failing Test

Following TDD principles, we began by writing a test that expressed our new requirement:

[Theory]  
[InlineData("P@ssw^rd1", "Invalid character")]
public void Password_Should_Failed_To_Parse(string password, string reason)
	// We prototype a new method in the Password class
    => Password.ParseWithMultipleErrors(password)  
               .Should()  
               // We define what are our expectations for this new method: a list of errors (1 in this case)
               .BeLeft(errors => errors.Should().ContainSingle(e => e.Reason == reason));

This test failed initially, as expected in the TDD red phase. We consider a compilation error as part the red phase:

Step 2: 🟢 Make the Test Pass

We then generate code from usage and copy paste existing code to make it pass as fast as possible:

public static Either<Seq<ParsingError>, Password> ParseWithMultipleErrors(string input)   
=> Rules.Map(f => f(input))
        .Filter(r => r.IsSome)
        .Bind(r => r)  
        .ToSeq();

Step 3: 🔵 Refactor

After getting our test to pass, we considered potential refactorings. This step is crucial in TDD to ensure our code remains clean and maintainable.

What could be improved here according to you? 🤔

Step 4: 🔴 Add More Test Cases

We then added a test for the case where a string is successfully parsed as a Password:

[Theory]  
[InlineData("P@ssw0rd")]
public void Success_For_A_Valid_Password_Parsed_With_Multiple_Errors(string password)  
    => Password.ParseWithMultipleErrors(password)  
               .Should()  
               .BeRight(p => p.ToString().Should().Be(password));

Step 5: 🟢 Implement the Passing Case

We implemented the passing case and then refactored to remove redundancy:

public static Either<Seq<ParsingError>, Password> ParseWithMultipleErrors(string input)  
{
	// Filter to retrieve all the parsing errors
    var parsingErrors =  Rules.Map(f => f(input))  
                              .Filter(r => r.IsSome)  
                              .Bind(r => r)  
                              .ToSeq();  
  
    return parsingErrors.IsEmpty
	    // Instantiate a new password if no errors
        ? new Password(input)  
        : parsingErrors;  
}

Step 6: 🔵 Remove redundancy

  • We have redundancies in our implementation (Filter is redundant with Bind calls)
  • The original Parse method could use the new ParseWithMultipleErrors to avoid duplication
public static Either<Seq<ParsingError>, Password> ParseWithMultipleErrors(string input)  
{  
    var parsingErrors =  Rules.Map(f => f(input))  
                              .Bind(r => r)  
                              .ToSeq();  
  
    return parsingErrors.IsEmpty   
? new Password(input)  
        : parsingErrors;  
}

public static Either<ParsingError, Password> Parse(string input) 
	=> ParseWithMultipleErrors(input)
            .MapLeft(errors => errors.Head());

Final Refactoring: 🔵 Removing Accidental Complexity

Accidental Complexity

Something shocked me when I opened the proposed solution... (that I have developed myself a few months ago). I had implemented accidental complexity and redundancy code...

// It can be really complex to understand what is going on here...
private static readonly Seq<Func<string, Option<ParsingError>>> Rules = 
    create<Func<string, Option<ParsingError>>>(  
        // Every line is a duplicvated call to Match
        input => Match(input, ".{8,}", "Too short"),  
        input => Match(input, ".*[A-Z].*", "No capital letter"),  
        input => Match(input, ".*[a-z].*", "No lower letter"),  
        input => Match(input, ".*[0-9].*", "No number"),  
        input => Match(input, ".*[.*#@$%&].*", "No special character"),  
        input => Match(input, "^[a-zA-Z0-9.*#@$%&]+$", "Invalid character")  
    );
    
public static Either<ParsingError, Password> Parse(string input) =>  
    ParseWithMultipleErrors(input).MapLeft(errors => errors.Head());  

// Why do we need to use an Option to do so? 🤔
private static Option<ParsingError> Match(string input, string regex, string reason)  
    => !Regex.Match(input, regex).Success  
        ? new ParsingError(reason)  
        : None;

We can use a Map or a Rule object to achieve the same purpose.

  • Rule Definition: We have defined a Rule class that encapsulates the regex pattern and the reason for the error. This makes it easier to manage and understand the rules.
public sealed record Rule(string Pattern, string Reason)
{
    public bool IsNotMatching(string input) => !IsMatch(input, Pattern);
}
  • Rule Collection: We have created a collection of Rule objects using Seq.create. This eliminates the need for duplicated calls to the Match method and makes the code more concise.
private static readonly Seq<Rule> Rules = Seq.create(
    new Rule("^.{8,}$", TooShort),
    new Rule(".*[A-Z].*", NoCapitalLetter),
    new Rule(".*[a-z].*", NoLowerLetter),
    new Rule(".*[0-9].*", NoNumber),
    new Rule(".*[.*#@$%&].*", NoSpecialCharacter),
    new Rule("^[a-zA-Z0-9.*#@$%&]+$", InvalidCharacter)
);

A few words on Copilot

To refactor the Rules from the previous implementation using Seq<Func<string, Option>> to Seq<Rule> , Github Copilot "helped" us... or not...

At this point, we had not noticed the mistakes made by Copilot, but we immediately received feedback from our tests.

After a few seconds of investigation, we discovered that it has completely messed up the Regex we used...

private static readonly Seq<Rule> Rules = create(
    new Rule(@"^.{8,}$", "Too short"), // "^.{8,}$"
    new Rule(@"[A-Z]", "No capital letter"), // ".*[A-Z].*"
    new Rule(@"[a-z]", "No lower letter"), // ".*[a-z].*"
    new Rule(@"\d", "No number"), // ".*[0-9].*"
    new Rule(@"[^A-Za-z0-9]", "No special character"), // ".*[.*#@$%&].*"
    new Rule(@"^[A-Za-z0-9!@#$%^&*()_+{}\[\]:;""'<,>.?/\|\\`~ -]+$", "Invalid character") // "^[a-zA-Z0-9.*#@$%&]+$"
);
Small advice: Never blindly believe what a GenAi tool generates...

Final Version

Here is the final version of our code which, we think, is more readable and maintainable.

using LanguageExt;  
using static System.Text.RegularExpressions.Regex;  
using static Password.Password.Errors;  
  
namespace Password  
{  
    public sealed class Password : IEquatable<Password>  
    {        private readonly string _value;  
  
        private Password(string value) => _value = value;  
  
        private static readonly Seq<Rule> Rules = Seq.create(  
            new Rule("^.{8,}$", TooShort),  
            new Rule(".*[A-Z].*", NoCapitalLetter),  
            new Rule(".*[a-z].*", NoLowerLetter),  
            new Rule(".*[0-9].*", NoNumber),  
            new Rule(".*[.*#@$%&].*", NoSpecialCharacter),  
            new Rule("^[a-zA-Z0-9.*#@$%&]+$", InvalidCharacter)  
        );  

        public static Either<Seq<ParsingError>, Password> Parse(string input)  
            => Rules.Filter(rule => rule.IsNotMatching(input))  
                .Map(rule => new ParsingError(rule.Reason))  
                .Let(parsingErrors => ToEither(input, parsingErrors));  
  
        private static Either<Seq<ParsingError>, Password> ToEither(string input, Seq<ParsingError> parsingErrors)  
            => parsingErrors.IsEmpty ? new Password(input) : parsingErrors;  
  
        ...
  
        public static class Errors  
        {  
            public const string TooShort = "Too short";  
            public const string NoCapitalLetter = "No capital letter";  
            public const string NoLowerLetter = "No lower letter";  
            public const string NoNumber = "No number";  
            public const string NoSpecialCharacter = "No special character";  
            public const string InvalidCharacter = "Invalid character";  
        }    
	}  

    public sealed record ParsingError(string Reason);  
    public sealed record Rule(string Pattern, string Reason)  
    {
		public bool IsNotMatching(string input) => !IsMatch(input, Pattern);  
    }
}

// And the associated tests
public class PasswordTests
{
    [Theory]
    [InlineData("P@ssw0rd")]
    [InlineData("Advent0fCraft&")]
    public void Success_For_A_Valid_Password_Parsed_With_Multiple_Errors(string password)
        => Parse(password)
            .Should()
            .BeRight(p => p.ToString().Should().Be(password));

    [Fact]
    public void Value_Equality()
    {
        const string input = "P@ssw0rd";
        var password = Parse(input).ValueUnsafe();
        var other = Parse(input).ValueUnsafe();

        password.Equals(other).Should().BeTrue();
        (password == other).Should().BeTrue();
        (password != other).Should().BeFalse();
    }

    public class FailWhen
    {
        [Theory]
        [InlineData("P@ssw^rd1", InvalidCharacter)]
        [InlineData("aa", TooShort, NoCapitalLetter, NoNumber, NoSpecialCharacter)]
        [InlineData("xxxxxxx", TooShort, NoCapitalLetter, NoNumber, NoSpecialCharacter)]
        [InlineData("adventofcraft", NoCapitalLetter, NoNumber, NoSpecialCharacter)]
        [InlineData("p@ssw0rd", NoCapitalLetter)]
        [InlineData("ADVENTOFCRAFT", NoLowerLetter, NoNumber, NoSpecialCharacter)]
        [InlineData("P@SSW0RD", NoLowerLetter)]
        [InlineData("Adventofcraft", NoNumber, NoSpecialCharacter)]
        [InlineData("P@sswOrd", NoNumber)]
        [InlineData("Adventof09craft", NoSpecialCharacter)]
        [InlineData("PAssw0rd", NoSpecialCharacter)]
        [InlineData("Advent@of9Craft/", InvalidCharacter)]
        [InlineData("", TooShort, NoCapitalLetter, NoLowerLetter, NoNumber, NoSpecialCharacter,
            InvalidCharacter)]
        public void Password_Should_Failed_To_Parse(string password, params string[] reasons)
            => Parse(password)
                .Should()
                .BeLeft(errors => ErrorsAreEquivalent(reasons, errors));

        private static void ErrorsAreEquivalent(string[] reasons, Seq<ParsingError> errors)
            => errors.Should().BeEquivalentTo(reasons.Select(c => new ParsingError(c)));
    }
}

Once we were happy and confident enough with our new implementation, we have progressively moved callers of the original Parse methods to our new implementation and finally strangled (Deleted) it.

With this article we wanted to demonstrate the power of TDD in guiding code evolution. By following the red-green-refactor cycle, we were able to methodically enhance our password validator, making it more robust and flexible while maintaining its integrity.

If you're interested in exploring this kata further, check out our repository: https://github.com/advent-of-craft/password-tdd

GitHub - advent-of-craft/password-tdd
Contribute to advent-of-craft/password-tdd development by creating an account on GitHub.
GitHubadvent-of-craft

Let us guide you step by step using the git log:

And if you enjoy this type of content, join us in the Advent Of Craft adventure:

GitHub - advent-of-craft/2023: Advent of Craft
Advent of Craft. Contribute to advent-of-craft/2023 development by creating an account on GitHub.
GitHubadvent-of-craft
Remember, TDD isn't just for new code – it's a powerful tool for evolving existing codebases too. Keep coding, keep testing, and keep improving!
Another heuristic to end up: TDD or not, never trust a test which has never failed!