1
\$\begingroup\$

Builder pattern separates object construction from its representation

I have to generate username and password for the Account class. Since this operation is a bit more complex, I decided to wrap it into a Builder pattern because it allows me to create an object step by step using methods.

  • FirstName and LastName are required
  • MiddleName is optional.
  • Faculty number is 6 digits
  • Username equals to [prefix][year of study which is the first two digits of the faculty number][name initials e.g. John Smith Mikaelson -> jsm]
  • Password is simply the faculty number, nothing complicated here

I would like to get a code review on the code and the unit tests because 1) it's another opinion 2) I'm also not sure what the naming convention is when there is no "Act".

Part of the code, i.e. the transliteration was already reviewed by Peter Csala at Transliterate between Cyrillic and Latin scripts

Snippet

public sealed class Account
{
    public string FirstName { get; set; } = string.Empty;
    public string MiddleName { get; set; } = string.Empty;
    public string LastName { get; set; } = string.Empty;
    public string Username { get; set; } = string.Empty;
    public string Password { get; set; } = string.Empty;
}

public sealed class AccountBuilder
{
    private readonly Account _account;

    public AccountBuilder()
    {
        _account = new Account();
    }

    public AccountBuilder WithFirstName(string firstName)
    {
        _account.FirstName = firstName;
        return this;
    }

    public AccountBuilder WithMiddleName(string middleName)
    {
        _account.MiddleName = middleName;
        return this;
    }

    public AccountBuilder WithLastName(string lastName)
    {
        _account.LastName = lastName;
        return this;
    }

    public Account Build(string prefix, string facultyNumber)
    {
        ArgumentNullException.ThrowIfNull(prefix);
        ArgumentNullException.ThrowIfNull(facultyNumber);

        var regex = new Regex("^[0-9]{6}$");
        if (!regex.IsMatch(facultyNumber))
        {
            throw new ArgumentOutOfRangeException(nameof(facultyNumber), "Faculty number should be 6-digits long");
        }

        // Transliterate name
        _account.FirstName = Transliteration.CyrillicToLatin(_account.FirstName);
        _account.MiddleName = Transliteration.CyrillicToLatin(_account.MiddleName);
        _account.LastName = Transliteration.CyrillicToLatin(_account.LastName);

        // Generate username and password
        // [cs/se][year-of-study][name-initials]
        var yearOfStudy = facultyNumber[..2];
        var firstLetterOfFirstName = _account.FirstName.Length > 0
            ? _account.FirstName.ToLowerInvariant()[..1]
            : _account.FirstName;
        var firstLetterOfMiddleName = _account.MiddleName.Length > 0
            ? _account.MiddleName.ToLowerInvariant()[..1]
            : _account.MiddleName;
        var firstLetterOfLastName = _account.LastName.Length > 0
            ? _account.LastName.ToLowerInvariant()[..1]
            : _account.LastName;
        _account.Username = $"{prefix}{yearOfStudy}{firstLetterOfFirstName}{firstLetterOfMiddleName}{firstLetterOfLastName}";
        _account.Password = facultyNumber;

        return _account;
    }
}
public static class Transliteration
{
    private static readonly ImmutableDictionary<char, string> CyrillicToLatinMapping = new Dictionary<char, string>
    {
        { 'а', "a" }, { 'А', "A" },
        { 'б', "b" }, { 'Б', "B" },
        { 'в', "v" }, { 'В', "V" },
        { 'г', "g" }, { 'Г', "G" },
        { 'д', "d" }, { 'Д', "D" },
        { 'е', "e" }, { 'Е', "E" },
        { 'ж', "zh" }, { 'Ж', "Zh" },
        { 'з', "z" }, { 'З', "Z" },
        { 'и', "i" }, { 'И', "I" },
        { 'й', "y" }, { 'Й', "Y" },
        { 'к', "k" }, { 'К', "K" },
        { 'л', "l" }, { 'Л', "L" },
        { 'м', "m" }, { 'М', "M" },
        { 'н', "n" }, { 'Н', "N" },
        { 'о', "o" }, { 'О', "O" },
        { 'п', "p" }, { 'П', "P" },
        { 'р', "r" }, { 'Р', "R" },
        { 'с', "s" }, { 'С', "S" },
        { 'т', "t" }, { 'Т', "T" },
        { 'у', "u" }, { 'У', "U" },
        { 'ф', "f" }, { 'Ф', "F" },
        { 'х', "h" }, { 'Х', "H" },
        { 'ц', "ts" }, { 'Ц', "Ts" },
        { 'ч', "ch" }, { 'Ч', "Ch" },
        { 'ш', "sh" }, { 'Ш', "Sh" },
        { 'щ', "sht" }, { 'Щ', "Sht" },
        { 'ъ', "a" }, { 'Ъ', "A" },
        { 'ь', "y" }, { 'Ь', "Y" },
        { 'ю', "yu" }, { 'Ю', "Yu" },
        { 'я', "ya" }, { 'Я', "Ya" },
        { ' ', " " }, { '-', "-" }
    }.ToImmutableDictionary();

    private static readonly ImmutableArray<(string Latin, char Cyrillic)> LatinToCyrillicMapping =
        CyrillicToLatinMapping
            .OrderByDescending(v => v.Value.Length)
            .Select(d => (Latin: d.Value, Cyrillic: d.Key))
            .ToImmutableArray();

    public static string CyrillicToLatin(string text)
    {
        return string.Join("", text.ToCharArray().Select(c => CyrillicToLatinMapping[c]));
    }

    public static string LatinToCyrillic(string text)
    {
        var startIdx = 0;
        var accumulator = new StringBuilder();
        while (startIdx != text.Length)
        {
            foreach (var (latin, cyrillic) in LatinToCyrillicMapping)
            {
                if (!text[startIdx..].StartsWith(latin))
                {
                    continue;
                }

                accumulator.Append(cyrillic);
                startIdx += latin.Length;
                break;
            }
        }

        return accumulator.ToString();
    }
}

Unit tests

public sealed class AccountTests
{
    [Theory]
    [InlineData("Иван", "", "Драганов", "196300", "cs19id", "196300")]
    [InlineData("Борис", "Стоянов", "Иванов", "226502", "cs22bsi", "226502")]
    public void Account_ShouldReturnAccountObject_WhenGivenName(
        string firstName,
        string middleName,
        string lastName,
        string facultyNumber,
        string expectedUsername,
        string expectedPassword)
    {
        // Arrange
        var account = new AccountBuilder()
            .WithFirstName(firstName)
            .WithMiddleName(middleName)
            .WithLastName(lastName)
            .Build("cs", facultyNumber);

        // Act

        // Assert
        account.Username.Should().Be(expectedUsername);
        account.Password.Should().Be(expectedPassword);
    }
}
\$\endgroup\$
16
  • 1
    \$\begingroup\$ Could you please link one of your previous questions where some part of the code has been already reviewed? \$\endgroup\$ Commented Oct 9, 2022 at 6:16
  • 1
    \$\begingroup\$ Btw in this case the Build method call is your Act inside the unit test. \$\endgroup\$ Commented Oct 9, 2022 at 6:17
  • 1
    \$\begingroup\$ @PeterCsala, thanks! I just linked the another code review. \$\endgroup\$
    – nop
    Commented Oct 9, 2022 at 11:22
  • 1
    \$\begingroup\$ @PeterCsala, if Build method call is Act, this is what I could think of i.imgur.com/mNt2U2r.png. Either way, I will leave the question like that. \$\endgroup\$
    – nop
    Commented Oct 9, 2022 at 11:51
  • 1
    \$\begingroup\$ The` Builder` object should not be keeping a reference to the Account object it creates. The Builder object should not be returning a reference to itself. \$\endgroup\$
    – radarbob
    Commented Oct 10, 2022 at 1:19

1 Answer 1

2
\$\begingroup\$

Account

  • Depending on the requirements it would make sense to make the class immutable by using init instead of set for the properties
    • You can even declare the Account as record and use the with operators inside the builder
public record Account(string FirstName = "", string MiddleName = "", string LastName = "", 
                      string UserName = "", string Password = "");
  • Marking the class as sealed seems to me a bit too defensive
    • Based on my previous experiences class like this is usually becomes pretty soon a base class for a couple derived classes (Student, Lecturer, etc.)

AccountBuilder

WithXYZ

  • Most of the With implementations what I have seen did the followings:
    • perform some preliminary checks (like not null)
    • perform an equality check to avoid unnecessary overwrites
    • perform the overwrite if needed
public AccountBuilder WithFirstName(string firstName)
{
    if (string.IsNullOrWhiteSpace(firstName))
        throw new ArgumentOutOfRangeException("Please provide a valid FirstName");

    if(!string.Equals(_account.FirstName, firstName, StringComparison.InvariantCulture))
        _account.FirstName = firstName;

    return this;
}

Build

  • Most of the time the builder function should not receive any parameter
    • Since you already have a fluent interface that's why you could easily create two WithXYZ methods for prefix and facultyNumber
  • If some properties must be set then check these first
public Account Build()
{
    if (string.IsNullOrWhiteSpace(_account.FirstName) || string.IsNullOrWhiteSpace(_account.LastName))
        throw new InvalidOperationException("FirstName, LastName must be set");
    //...
}
  • You can move the Regex to class level and make it static
    • Even you can set RegexOptions to Compiled
\$\endgroup\$
8
  • \$\begingroup\$ Thank you for your answer! I'm struggling with WithXYZ for prefix and facultyNumber. Does that mean that I should put Prefix and FacultyNumber properties in AccountBuilder? \$\endgroup\$
    – nop
    Commented Oct 10, 2022 at 10:46
  • \$\begingroup\$ @nop If they should not be externally available, because they are used only inside the Build method then it is enough to declare them as private fields. \$\endgroup\$ Commented Oct 10, 2022 at 10:50
  • \$\begingroup\$ Here is what I did based on the suggestions pastebin.com/9yWac2Ju \$\endgroup\$
    – nop
    Commented Oct 10, 2022 at 11:14
  • \$\begingroup\$ I was given the following information Returning a ref to itself is fine, especially if you like fluent syntax. Holding a reference to the generated object can be more problematic. You have to be careful how the builder and the object are used. Normally I would create the object only within the Build method. In your case, yes. I use builders mostly for tests. In your domain code, your invariants should be already protected via constructor/factory methods/methods. \$\endgroup\$
    – nop
    Commented Oct 10, 2022 at 20:34
  • 1
    \$\begingroup\$ You're probably right. Let's keep it that way. \$\endgroup\$
    – nop
    Commented Oct 12, 2022 at 12:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.