8
\$\begingroup\$

Note: the answer can come in either VB.NET or C#. I have no preference for this Q&A.


I'd like to tame the monstrosity I've created below.

The requirement is to obtain some input from the user and then use that information to generate a Let's Encrypt TLS certificate to be used by the application (a .NET 8 Blazor WebApp in this case).

The whole process consists of the nine steps we see here, with each new step relying on the success of the one just prior. If any one of them fails, we exit the function at that point.

The problem is this has resulted in a maintenance headache with all the cascading If blocks. To complicate matters, some of the steps require as input some of the output generated two or three steps back.

Is there some sort of design pattern or established technique that I can use to manage all of this a bit more cleanly? Surely there must be a better way.

The Mess

<SupportedOSPlatform("Windows")>
Public Shared Async Function ConfigureApplicationAsync(Args As String()) As Task(Of Result)
  Dim oCityCodeResult As Result(Of City)
  Dim oCsrResult As Result(Of (Challenge As IChallengeContext, Order As IOrderContext, DnsText As String))
  Dim oDnsRecord As (Content As String, Type As DnsRecordType, Name As String)
  Dim oTlsResult As Result(Of X509Certificate2)
  Dim oTcpResult As Result(Of Net.IPAddress)
  Dim oDnsResult As Result
  Dim oCrtResult As Result
  Dim sCityCode As String
  Dim lIsAdmin As Boolean
  Dim oResult As Result
  Dim oCity As City

  With New WindowsPrincipal(WindowsIdentity.GetCurrent)
    lIsAdmin = .IsInRole(WindowsBuiltInRole.Administrator)
  End With

  Try
    If Args.Any(Function(Arg) Arg.ToLower = "install") Then
      If lIsAdmin Then
        oCityCodeResult = Await CityCode.RegisterAsync

        If oCityCodeResult.IsSuccess Then
          oTcpResult = Await Tcp.GetPrivateAddressAsync("Getting the local private IP address...")

          If oTcpResult.IsSuccess Then
            oCity = oCityCodeResult.Value
            sCityCode = oCity.Code.ToLower

            oDnsRecord.Content = oTcpResult.Value.ToString
            oDnsRecord.Type = DnsRecordType.A
            oDnsRecord.Name = sCityCode

            oDnsResult = Await Dns.RegisterAsync(oDnsRecord, $"Registering {oCity.Name} in the public DNS...")

            If oDnsResult.IsSuccess Then
              oCsrResult = Await Csr.SubmitAsync(sCityCode, "Submitting a Certificate Signing Request...")

              If oCsrResult.IsSuccess Then
                oDnsRecord.Content = oCsrResult.Value.DnsText
                oDnsRecord.Type = DnsRecordType.Txt
                oDnsRecord.Name = $"_acme-challenge.{sCityCode}"

                oDnsResult = Await Dns.RegisterAsync(oDnsRecord, $"Updating the DNS with the request's validation token...")

                If oDnsResult.IsSuccess Then
                  oCsrResult = Await Csr.ValidateAsync(oCsrResult.Value.Challenge, oCsrResult.Value.Order, "Validating the request...")

                  If oCsrResult.IsSuccess Then
                    oTlsResult = Await Tls.GenerateAsync(oCsrResult.Value.Order, sCityCode, "Generating a TLS certificate...")

                    If oTlsResult.IsSuccess Then
                      oDnsResult = Await Dns.CleanupAsync(oDnsRecord, "Cleaning up the DNS after a successful validation...")

                      If oDnsResult.IsSuccess Then
                        oCrtResult = Await Crt.ImportAsync(oTlsResult.Value, "Importing the certificate locally...")

                        If oCrtResult.IsSuccess Then
                          oResult = Result.Ok
                        Else
                          oResult = Result.Fail(oCrtResult.Errors.Select(Function(Err) Err.Message))
                        End If
                      Else
                        oResult = Result.Fail(oDnsResult.Errors.Select(Function(Err) Err.Message))
                      End If
                    Else
                      oResult = Result.Fail(oTlsResult.Errors.Select(Function(Err) Err.Message))
                    End If
                  Else
                    oResult = Result.Fail(oCsrResult.Errors.Select(Function(Err) Err.Message))
                  End If
                Else
                  oResult = Result.Fail(oDnsResult.Errors.Select(Function(Err) Err.Message))
                End If
              Else
                oResult = Result.Fail(oCsrResult.Errors.Select(Function(Err) Err.Message))
              End If
            Else
              oResult = Result.Fail(oDnsResult.Errors.Select(Function(Err) Err.Message))
            End If
          Else
            oResult = Result.Fail(oTcpResult.Errors.Select(Function(Err) Err.Message))
          End If
        Else
          oResult = Result.Fail(oCityCodeResult.Errors.Select(Function(Err) Err.Message))
        End If
      Else
        oResult = Result.Fail("The service installation must be run as an administrator.")
      End If
    Else
      oResult = Result.Ok
    End If

  Catch ex As Exception
    oResult = Result.Fail(ex.ToString)

  End Try

  Return oResult
End Function
\$\endgroup\$
4
  • 2
    \$\begingroup\$ That's a classic example of arrow head antipattern. Use guard clauses (if not admin, if not city code result success, etc.) and early returns instead of a single return statement at the very end to simplify the logic. \$\endgroup\$ Commented May 5 at 20:50
  • \$\begingroup\$ "Use guard clauses (if not admin, if not city code result success, etc.)" I must be missing something somewhere. Wouldn't that result in the same indentation free-for-all? Do you have an example? \$\endgroup\$
    – InteXX
    Commented May 5 at 21:09
  • \$\begingroup\$ I just found this. I'll have a close look at those guard clauses, thanks. Care to make it an answer? \$\endgroup\$
    – InteXX
    Commented May 5 at 21:13
  • \$\begingroup\$ Oops... I may not be able to fix this after all. I've just discovered that Steps 5-9 have to be contained in a retry loop; it turns out that certificate validation fails intermittently, for apparently transient causes. I may have to approach the whole thing differently. \$\endgroup\$
    – InteXX
    Commented May 5 at 21:17

2 Answers 2

9
\$\begingroup\$

Your ConfigureApplicationAsync method calls 9 async methods in a predefined sequence (which might be too much responsibilities for a single method btw). Each of the actions depends on the previous actions' result.

Your flow control logic is repeated over and over again:

enter image description here

I've used only three steps in my mermaid diagram to illustrate the core problem.

When you start Googling you will find a lots of articles suggesting chain of responsibility, middleware, or pipeline patterns usage. You might bump into the Rules (engine) pattern. Here the basic idea is that each and every step/rule is encapsulated into a dedicated structure. The flow control is separated from the steps/rules. I would suggest to apply this pattern for your method.

Let me demonstrate it through the 3 most inner method calls.

oTlsResult = await Tls.GenerateAsync(oCsrResult.Value.Order, sCityCode, "Generating a TLS certificate...");
if (oTlsResult.IsSuccess)
{
    oDnsResult = await Dns.CleanupAsync(oDnsRecord, "Cleaning up the DNS after a successful validation...");

    if (oDnsResult.IsSuccess)
    {
        oCrtResult = await Crt.ImportAsync(oTlsResult.Value, "Importing the certificate locally...");

        if (oCrtResult.IsSuccess)
        {
            oResult = Result.Ok;
        }
        else
        {
            oResult = Result.Fail(oCrtResult.Errors.Select(Err => Err.Message));
        }
    }
    else
    {
        oResult = Result.Fail(oDnsResult.Errors.Select(Err => Err.Message));
    }
}
else
{
    oResult = Result.Fail(oTlsResult.Errors.Select(Err => Err.Message));
}

Each async method call relies on one or many previous steps' result. How can we make the steps a bit more loosely coupled to be able to encapsulate them into separate structures? By using a context :D

interface IStep
{
    Task<IEnumerable<Error>> ExecuteAsync(Dictionary<string, object> context);
}
  • Each step receives a context which contains all previous results
  • If the given step succeeds then it adds its own result to the context and returns an empty array
  • If the given step fails then it returns the errors
class TlsStep: IStep
{
    public async Task<Result<IEnumerable<Error>> ExecuteAsync(Dictionary<string, object> context)
    {
        // Retrieves parameters
        var oCsrResult = context["oCsrResult"] as Result<(IChallengeContext Challenge, IOrderContext Order, string DnsText)>;
        var sCityCode =  context["sCityCode"] as string;

        // Performs action
        var oTlsResult = await Tls.GenerateAsync(oCsrResult.Value.Order, sCityCode, "Generating a TLS certificate...");
        
        // Handles action's result properly
        if(oTlsResult.IsSuccess)
        {
            context[nameof(oTlsResult)] = oTlsResult;
            return Array.Empty<Error>();
        }
        
        return oTlsResult.Errors;
    }
}

By encapsulating the steps into their own structures, the flow control becomes straightforward

var context =  new Dictionary<string, object>();

IStep[] steps = 
[
    ...
    new TlsStep(),
    new DnsStep(),
    new CrtStep()
];

foreach(var step in steps)
{
    var errors = await step.ExecuteAsync(context);
    if(errors.Any())
    {
        return Result.Fail(errors.Select(Err => Err.Message));
    }
}

return Result.Ok;

I've put together a simple dotnet fiddle (with some mock implementations) as a playground: https://dotnetfiddle.net/BjaCDo

\$\endgroup\$
7
  • \$\begingroup\$ I want to emphasize that this suggested solution is far from perfect. Using things like context["oCsrResult"] is super fragile. The main point here is that you can add an abstraction to your flow control to make that simpler. \$\endgroup\$ Commented May 6 at 8:15
  • 1
    \$\begingroup\$ That, my friend, is a thing of beauty. As far as the fragility you mention, the first thing that comes to mind is a Dictionary<enum, object>() instead. But you may have even a better idea than that. The bottom line, though, is that I'm going to use this. Bonus: it allows for easy separation and grouping of the various steps for use elsewhere as needed. Thanks! You've made my day. Have I gushed enough yet? \$\endgroup\$
    – InteXX
    Commented May 6 at 8:45
  • 1
    \$\begingroup\$ @InteXX This answer might be perfect for your provided code, but if you have similar issue elsewhere in your project, then I think you will need to take a look at MediatR library, which would give you a simple way to handle similar issues by using mediator pattern. \$\endgroup\$
    – iSR5
    Commented May 10 at 16:40
  • 1
    \$\begingroup\$ FYI I just finished putting this together, it works great! It's got all kinds of upsides and no downsides. Thanks again. \$\endgroup\$
    – InteXX
    Commented May 10 at 20:46
  • 1
    \$\begingroup\$ "super fragile" I created a Context class that inherits from Dictionary(Of MyEnum, ResultBase), so I could add type-safe properties to it. That shored things up quite a bit. \$\endgroup\$
    – InteXX
    Commented May 17 at 21:08
4
\$\begingroup\$

You have two issues here, the first one is that you should invert the condition for early return, which would decrease the nesting levels.

The second issue is understanding the related executions and dependencies. If we try to summarize the code flow in simple terms, we would have the following :

  1. Verify User Permission
  2. Verify Arguments
  3. Verify CityCode
  4. Get private IP Address
  5. Register DNS Record
  6. Submit CSR
  7. Update DNS Record
  8. Validate CSR
  9. Generate TLS Certificate
  10. Clean up DNS
  11. Install the certificate locally

When we group them together by feature, we get :

  • Application [1,2,3,4]:
    • Verify User Permission
    • Verify Arguments
    • Verify CityCode
    • Get private IP Address
  • DNS Record [5,7,10]
    • Register DNS Record
    • Update DNS Record
    • Clean up DNS
  • Certificates [6,8,9]
    • Submit CSR
    • Validate CSR
    • Generate TLS Certificate
  • Server [11]
    • Install the certificate locally

Since they're all chained up in a sequence in order to update the newly created DNS record. If one of them failed, then the new DNS record should not be committed. It could be fine, but from the code, it seems any new DNS record should contain an acme challenge (at least for this particular scenario). So, what happens to the created DNS record if CSR request fails or any of the following processes fail?

This is some of the questions I thought about while reviewing the code.

In addition to @peter-csala answer, you have multiple ways to avoid this nesting monstrosity, and it depends on which your project needs it or not. The simplest one (if it's only a couple of methods) you can invert the conditions, and refactor your code to have a class that contains all of steps from 5-11, then recall that class with the new dns record something like this :

public static async Task<Result> ConfigureApplicationAsync(string[] Args)
{
    try
    {
        if (!Args.Any(Arg => Arg.ToLower() == "install"))
            return Result.Ok;

        if(!isAdmin)
            return Result.Fail("The service installation must be run as an administrator.");

        var oCityCodeResult = await CityCode.RegisterAsync;

        if(!oCityCodeResult.IsSuccess)
            return Result.Fail(oCityCodeResult.Errors.Select(Err => Err.Message));

        var oTcpResult = await Tcp.GetPrivateAddressAsync("Getting the local private IP address...");

        if(!oTcpResult.IsSuccess)
            return Result.Fail(oTcpResult.Errors.Select(Err => Err.Message));

        var content = oTcpResult.Value.ToString;
        
        var city = oCityCodeResult.Value;
        
        var cityCode = city.Code.ToLower;
        
        var dnsRecord = (content, DnsRecordType.A, cityCode) 
        
        var result = await DnsRecord.CreateWithTlsCertificateAsync(dnsRecord);
        
        return result;
    }
    catch (Exception ex)
    {
        return Result.Fail(ex.ToString());
    }
}

In the above example, assume we moved all code that related to steps 5 through 11. With this, we have a better handling on DNS creation, and in the ConfigureApplicationAsync method you only worry about the results. There are many patterns I could think of like Mediator, Factory, Builder, Visitor, and Service design patterns. All of which can be combined and used to to fulfill your requirements.

Now, say you have almost the same thing happens all around the project, and you want to avoid repeating yourself. In this case, you may need to look into design patterns, to see which pattern may actually fix your issues.

One pattern that may help you is a Mediator Pattern (as I mentioned in the comments). In .NET, you can use MediatR library. Simple, and easy to use, and also it's used widely with web applications. You can then refactor your code by it's business concern.

First we will need to create folder structure to make things more organized and easy to access. We need to separate them by feature then by action, so We can do something like :

  • Application
    • DomainNameServer
      • DomainNames
      • SSLCretificates
        • Commands
          • CreateCSRCommand.cs
          • ValidateCSRCommand.cs
        • Handlers
          • CreateCSRCommandHandler.cs
          • ValidateCSRCommandHandler.cs
      • DNS
        • Commands
          • CreateDNSRecordCommand.cs
          • UpdateDNSRecordCommand.cs
        • Handlers
          • CreateDNSRecordCommandHandler.cs
          • UpdateDNSRecordCommandHandler.cs

Taking one of them using MediatR:

public sealed class CreateDnsRecordCommand : IRequest<Result> 
{   
    public string Content { get; set; }
    public DnsRecordType Type { get; set; }
    public string Name { get; set; }
}

public sealed class CreateDnsRecordCommandHandler : IRequestHandler<CreateDnsRecordCommand, Result>
{
    private readonly Dns _dns;
    
    //DI
    public CreateDnsRecordCommandHandler(Dns dns)
    {
        _dns = dns;
    }
    
    public async Task<Result> Handle(CreateDnsRecordCommand command, CancellationToken cancellationToken)
    {
        // code to handle the creation of DNS Record
    }   
}

You can implement a handler that handles multiple handlers (aggregation) to which feature is required or use publishers or events, just ensure of the separation between the domain and application layers.

Now, in your Blazor app, you just inject, and use the handlers or you could create a service that would expose some methods and use the handlers internally.

\$\endgroup\$
6
  • \$\begingroup\$ Ah, I see what you're getting at now. Yes, those guard clauses do simplify things greatly, don't they? No more arrowheads! FYI I'm also a fan of the mediator pattern; it cleanly solves this mess (from the page you linked). It's not too late to work that into what I have with the Context/Loop approach; I'll probably end up doing that. FWIW I ended up with steps whose results drive decision trees on which groups of steps are next. And some steps have retry loops that affect the entire group. It's getting quite complex. \$\endgroup\$
    – InteXX
    Commented May 20 at 20:22
  • \$\begingroup\$ That's a good site reference, thanks! I'll be able to make good use of that. \$\endgroup\$
    – InteXX
    Commented May 20 at 20:23
  • \$\begingroup\$ @InteXX you can take a look at Strategy design pattern, it's similar to what you've described. \$\endgroup\$
    – iSR5
    Commented May 21 at 4:19
  • \$\begingroup\$ Well now isn't that interesting! Completely by accident, while dealing with these extra complexities, I've ended up with a design that pretty much exactly mirrors the Strategy pattern. The only significant difference is that the Execute() method is called elsewhere, not within the Context. But the rest is all there. How 'bout that 🙂 \$\endgroup\$
    – InteXX
    Commented May 21 at 19:42
  • \$\begingroup\$ @InteXX There are a lot of design patterns that already cover every day coding challenges. So, don't be surprised if you've applied a solution that you later on discovers that it's an actual design pattern ;) . Try to move your Execute method into the Context, this would make things much easier for your future self and other contributors as well.+ \$\endgroup\$
    – iSR5
    Commented May 21 at 23:23

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.