Long Functions Are Always A Code Smell

This article is in response to fellow MVP Alex Shelga’s recent article Long functions in dataverse plugins – is it still ” code smell”?.  I’ll start with the fact that there is plenty of room for personal preference and there is no magic equation that can be applied to code that can ultimately define code as good or great or bad.  Alex shared his opinion, and here I’ll share mine.  I’ll tell you right now, they will differ (which shouldn’t be a surprise if you’ve read the title).  It is my hope that no one feels that I’m “attacking” Alex (especially Alex), but that everyone can see this as what it is intended to be, a healthy juxtaposition of ideas.

Alex’s Argument

Before I go into my reasons for why long functions are always a code smell, I’ll list the two reasons Alex sees plugins as different and summarize his arguments for why that matters:

1. Plugins are inherently stateless
2. Often developed to provide a piece of very specific business logic

This, he says, “seems to render object-oriented approach somewhat useless in the plugins (other than, maybe, for “structuring”)”.  He then dives into this further and seems to imply that OO code is slower and more complicated, and is primarily used to allow for reusability, and if it’s not making the code more reusable, there is no reason to utilize it.  His final point then is that it doesn’t matter to the performance of the system or to unit testing if the code is longer, and in his personal preference, he finds a longer function more readable: “I’d often prefer longer code in such cases since I don’t have to jump back and forth when reading it / debugging it”.  (If this is you, make sure you memorize the Navigate Forward and Navigate Backward commands in your IDE (View.NavigateBackword Ctrl+- and View.NavigateBackword Ctrl+Shift+- in Visual Studio and Alt+Left Arrow and Alt+Right Arrow in VSCode) then you need to spend the next 10 minutes diving into functions to see what they are doing, and then backing out of them using the navigation shortcut keys.  It could change your life. Scout’s honor)

My Argument

There are no facts that he presents that are wrong, plugin logic is inherently stateless, and doesn’t lend itself to loads of reusability.  I also can’t argue if his personal choice of readability is for long functions is right or wrong. But what I can do is argue why I see shorter functions as more readable, as well as other reasons that I have that shorter functions are better for the health and maintainability of the plugin project.

Why (I find) shorter functions are more readable

If you were to pick up a 300 page book with the title “Execute”, that you’ve never read before, with no cover art or introduction, or chapters or table of contents, or synopsis on the back page, but given 60 seconds to examine it and tell someone what it was about, you’d be pretty hard pressed to give an accurate definition.  But, if the book had a table of contents with these chapter names:

1. Start at the Beginning
2. Create a Vision
3. Share the Vision
4. Create the Company
5. Invest in Others
6. Invite Others to Invest
7. Grow/Multiply

You could guess fairly confidently it’s a book about starting and growing a business.  If you were only interested in the details of how to get additional investors in a business, you might start at chapter 6.  If however the chapter names were as follows:

1. Prewar
2. Early Victories
3. Atrocities Beyond Belief
4. Final Battles
5. Capture
6. The Trial
7. The Verdict
8. Final Words

You could guess that the book is about a soldier/general that committed war crimes and was executed.  If you were only interested in learning if the individual had any remorse for their acts, you might start reading at chapter 8.  So not only do these chapter titles allow you to get a very quick understanding of what the book is about, they also allow you to skip large sections of the book when attempting to find a very narrow topic.  The same is true for code and long functions.  If a function is longer than your screen is tall, the first time you look at it, you will have no idea about what it does beyond the reach of your screen without scrolling and reading.  You’d have to read the entire function to determine what it does. This means that if you’re looking for a bug, you’ll need to read and understand half (on average) of the lines in the function before you could find where the bug is.  But, if the function is 15 lines long with 8 well named functions calls, you’d have a much better guess at what the entire function does and where the bug lies.  For example, given this Execute function:

public void Execute(ExtendedPluginContext context)
{
    var data = GetData(context);
    UpdateAttributes(context, data);
    CreateChildRecords(context, data);
    UpdateTarget(context, data);
}

Now these are probably some pretty poor function names, but you can immediately see that the plugin is getting data, updating some attributes, creating child records and then updating the target.  But just a small improvement in the naming would give even more details:

public void Execute2(ExtendedPluginContext context)
{
    var account = GetAccount(context);
    SetMaxCreditLimit(context, account);
    CreateAccountLogEntries(context, account);
    UpdateTargetStatus(context, account);
}

Now it’s easy to see that there is a call to get the account, which is used it to set the max credit limit and create some log entries and then update the status of the target.  If there is a bug with the status getting updated incorrectly, or the max credit limit not being set, or the log entries not having enough details, it is easy to see what function needs to be looked at first, and what functions can be ignored.  Small functions (when done well) are more efficient for understanding.

Another positives from smaller functions is the error log in the trace.  If my Execute function is 300 lines long and it has a null ref, I’ve got to look at 300 lines of code to guess where the null ref could have occurred.  But since the function name is included in the stack trace for plugins (even when the line number isn’t), if the 300 lines where split into 10 functions of 30 lines, then I’d know the function that would be causing the error and would only have a tenth of the code to analyze for null ref.  That’s huge!

My final note comes into play with nesting “ifs”.  Many times I will walk into a project with 300 line Execute functions nested 10-12 levels deep with “if” statements.  This especially causes issues when it comes to trying to line up curly braces, or when an “else” statement occurs when the matching “if” is not on the screen:

                if (bar)
                {
                    if (baz)
                    {
                        Go();
                    }
                }
                else
                {
                    Fight();
                }
            }
            else
            {
                // Wait, what is this else-ing?
                Win();
            }
        }
    }
}

Although there is nothing that says a longer function has to nest “ifs”, if your function is only 10 lines long, it limits the maximum possible number of nested “ifs”.

When Shorter Functions Help With Testing

Alex mentioned that Testing frameworks like FakeXrmEasy (and I’ll through my XrmUnitTest framework in here as well) don’t care about the length of an Execute function.  It’s a black box.  While this is true, as a test creator, the more complex the logic, the more helpful it is to test it in parts, rather than the whole.  For example, in my Execute2 function above, if there are 3 different branches of logic in GetAccount and 2 in SetMaxCreditLimit, and 4 in CreateAccountLogEntries, and 1 in UpdateTargetStatus, this results in 24 different potential dependent paths to test.  Contrast this to testing the parts separately, and only having 10 different tests with only the required setup for each specific function.  This is much more maintainable.  Personally I believe that this can be taken to the extreme as well, and trying to test 100 functions to perfection is usually not the ideal time investment as well, so I may have a couple tests of the execute function start to finish, and cherry pick some of the more complicated functions to test, rather than try to test everything. 

In Conclusion

Take time to analyze other peoples opinions and determine if you agree or disagree to the point where you are be prepared to argue why.  We are all learning and growing in our craft as developers, which requires us to continue to allow new ideas to challenge our existing conventions.  Share it, blog about it, and grow, remembering to always “Raise La Bar”.

TypeScript Makes Writing Cleaner Code Easier

Clean Coders know that Boolean Arguments can be dangerous and as a general rule, should be avoided.  I had this thought in mind when writing a TypeScript unit test recently.  I had this bit of code which is rather ugly and not immediately obvious what it was doing.

getPaymentMethodsSpy.and.callFake(() => ActionsStubs.GetPaymentMethods.fakeResponseWithPaymentCount(1));
await Payment.setPayments("");
CommonLib.setValue(Payment.fields.paymentMethod, 1);

The fakeResponseWithPaymentCount sets the getPaymentMethodsSpy to generate the given number of fake existing payments for a count (1 in this case) as a response from the custom action, GetPaymentMethods.  Each odd numbered payment is a bank, and each even numbered one is a credit card.   So for this snippet, line 1 fakes a response of 1 Bank Payment, line 2 sets the Payment on the form, and line 3 selects that payment as the payment to use.  Simple right?  Not.  Good luck remembering this and re-creating it for processing a Credit Card.
So how does TypeScript here help make this cleaner?  Before we get there, lets do the obvious first step and refactor these statements out to a method:

async function selectExistingPayment(isBank: boolean) {
    getPaymentMethodsSpy.and.callFake(() => ActionsStubs.GetPaymentMethods.fakeResponseWithPaymentCount(2));
    await Payment.setPayments("");
    CommonLib.setValue(Payment.fields.paymentMethod, isBank ? 1 : 2);
}

This is way more usable, except when I’m looking at a calls site for it, it’s still really not clear:

selectExistingPayment(true);
selectExistingPayment(false);

What does false mean?  Don’t select it?  Sure, you could create an enum and define “credit card”, and “bank” as the two options, but this is just a unit test helper method.  I’m not exposing it anywhere, and even if I did, that’s a lot of extra work (Ok, so it’s a tiny bit of work, but it can feel like a lot #1stWorldProblems).  The simple solution TypeScript offers is string Union Types.

async function selectExistingPayment(type: "CC" | "BANK") {
    getPaymentMethodsSpy.and.callFake(() => ActionsStubs.GetPaymentMethods.fakeResponseWithPaymentCount(2));
    await Payment.setPayments("");
    CommonLib.setValue(Payment.fields.paymentMethod, type === "BANK" ? 1 : 2);
}

So now the calls sites look like this:

Extremely readable as to what I’m doing, and you get compile time error checking that "Dad" (or any other values besides "CC" and "BANK" ) is not a valid Existing Payment Method!

How To Define Custom Action Parameters For Plugin Contexts

In my previous post “I’ve Been Doing Plugin Parameters Wrong For 7 Years”, I walk through IMHO the correct way to access parameters from the requests coming in for standard Dynamics 365 for Customer Engagement (hereafter referred to as CRM) Messages.  Applying the same logic for custom action plugins was a little more complicated because naturally, there are no message classes in the SDK for custom actions.  The CrmSvcUtil does generate early-bound classes for custom actions, which work fairly well for input parameters since they are editable, but response parameters by default are read-only.  So any attempt to set a value in the response via the parameter property, is not allowed.  There is also no defined list of parameter logical names, which could be used to set the values in the response context in a late-bound manner.

So, how does one work around the read-only response parameters and lack of a defined list of parameter logical names?  By extending CrmSvcUtil to make the read-only fields editable, and to generate a list of parameter logical names.  This isn’t very simple to do since you have to work directly with the DOM (no, not the html DOM, the C# DOM, it’s way less fun).  The good news is I have already done this, and provide it as a set of options in the Early-Bound Generator (If you aren’t already using the Early-Bound Generator, just download it from the Plugin Store within the XrmToolBox).  On the Actions Tab, just check “Generate Attribute Name Consts” and “Make Responses Editable”:



With those two options checked, when actions are generated, responses will be editable, and both request and response classes will contain a static class with consts for each parameter.  The absolute simplest and most immediate method of taking advantage of these generated action request/response classes would be to manually create them in the plugin like so:

public void Execute(IServiceProvider serviceProvider)
{
    var context = (IPluginExecutionContext) serviceProvider.GetService(typeof(IPluginExecutionContext));
    var request = new your_CustomActionRequest
                  {
                      Parameters = context.InputParameters
                  };
    var response = new your_CustomActionResponse
                   {
                       Results = context.OutputParameters
                   };

    // Use the Request and Response Properties
    response.Result = Process(request.SomeInputValue);
}

This isn’t horrible, but if you call any other methods, you’ll probably end up passing in the context and the request and/or response to that method.   The request and the response truly are part of the context, so if would make sense if we can make them just appear as a part of the context.  This can be done by creating your own custom context class:

public class ActionContext<TRequest, TResponse> : DLaB.Xrm.Plugin.DLaBExtendedPluginContextBase
    where TRequest : Microsoft.Xrm.Sdk.OrganizationRequest, new() 
    where TResponse : Microsoft.Xrm.Sdk.OrganizationResponse, new()
{
    public TRequest Request { get; set; }
    public TResponse Response { get; set; }
    public ActionContext(IServiceProvider serviceProvider, DLaB.Xrm.Plugin.IRegisteredEventsPluginHandler plugin) : base(serviceProvider, plugin)
    {
        Request = new TRequest
        {
            Parameters = PluginExecutionContext.InputParameters
        };
        Response = new TResponse
        {
            Results = PluginExecutionContext.OutputParameters
        };
    }
}

*Note* I’m using the DLaBExtendedPluginContextBase as my base class since this exposes all the IPluginExecutionContext methods for you (this is available on Nuget DLaB.Xrm.2015/2016). If you don’t want to use that, your ActionContext base class will need to implement the IPluginExecutionContext interface. 

Now our plugin code can be written with even less keystrokes:

public void Execute(IServiceProvider serviceProvider)
{
    var context = new ActionContext<your_CustomActionRequest, your_CustomActionResponse>(serviceProvider, this);

    // Use the Request and Response Properties
    context.Response.Result = Process(context.Request.SomeInputValue);
}

It might even make sense to create your own specific action context class to perform additional validation.  This would technically be cleaner code as well, since the context should be responsible for determining if it’s valid, not the plugin.  For example, here is a Custom Action class for processing payment that performs some validations:

using System;
using Consoto.Xrm.Plugin;
using DLaB.Xrm.Plugin;
using Microsoft.Xrm.Sdk;
using Input = Consoto.Xrm.Entities.cnst_SubmitPaymentRequest.Fields;
 
namespace Consoto.Xrm.ExternalRest.Finance.Actions
{
    public class SubmitPaymentContext : ActionContext<Entities.cnst_SubmitPaymentRequest, Entities.cnst_SubmitPaymentResponse>
    {
        public const int CreditCardPaymentType = 1;
        public const int BankPaymentType = 2;
 
        public SubmitPaymentContext(IServiceProvider serviceProvider, IRegisteredEventsPluginHandler plugin) :
            base(serviceProvider, plugin)
        {
            AssertIsPopulated(Input.paymentType);
            AssertIsPopulated(Input.customerNumber);
            AssertIsPopulated(Input.firstName);
            AssertIsPopulated(Input.lastName);
            AssertIsPopulated(Input.tax);
 
            switch (Request.paymentType) {
                case CreditCardPaymentType:
                    AssertIsPopulated(Input.creditCardNumber);
                    AssertIsPopulated(Input.creditCardExpMonth);
                    AssertIsPopulated(Input.creditCardExpYear);
                    break;
                case BankPaymentType:
                    AssertIsPopulated(Input.accountNumber);
                    AssertIsPopulated(Input.routingNumber);
                    break;
                default:
                    throw new InvalidPluginExecutionException($"Payment Type of {Request.paymentType} is unknown!");
            }
        }
    }
}

This has the added benefit of not having to redefine the Request and Response Types in the plugin:

public void Execute(IServiceProvider serviceProvider)
{
    var context = new YourCustomActionContext(serviceProvider, this);

    // Use the Request and Response Properties
    context.Response.Result = Process(context.Request.SomeInputValue);
}

It used to take me over an hour (depending on the number of parameters in the custom action) to create the correctly defined custom action contexts, and of course I’d usually have a type-o or two that would require time debugging and re-updating.  Now that all the parameters are automatically generated, I get to spend that time on more productive things, like this blog post ;)

If you have any comments/feedback/suggestions/better methods, I’d love to hear them.  Happy coding!

Refactor Makes “Perfect”

I was recently doing a unit test of some Typescript logic for a Form in Dynamics 365 for Customer Engagement (Hereafter referred to as CRM).  Our CRM instance maintains a list of Localities and when a user selects a State/County it should perform different actions based on the number of localities found for the state/county combination:

• 1 found: Set the Lookup to the single Locality 
• 1+ found: Filter the Localities Lookup by state and county
• 0 found: Filter the Localities Lookup only by state and allow the user to choose.

In writing the test case for the first lookup, this is what I came up with at first:

it("should set locality if a single locality exists for the state/county", async (done) => {
    spyOn(restLibMock.CrmWebApiLib, "getList").and.callFake((r) => CrmWebApiLibStubs.getList.defaultValuesUsingSelect(r, 1));
    new Squire()
        .mock(restLibMock.path, restLibMock)
        .require(["dfnd_/scripts/lead/CustomerStage"], async (m: { CustomerStage: typeof CustomerStage }) => {
            await m.CustomerStage.setLocalityFilter();
            expect(CommonLib.getValue(LeadCommon.fields.locality)).toBeTruthy("because the locality should have been set because only one locality exists for the given city/state");
            done();
        });
});

It works, but it sucks:

• It’s really confusing (I’m going to explain it line by line, and you still probably won’t understand it)
• A lot will need to be duplicated to handle the other 2 cases.

First thing it does is setup a fake for the CrmWebApiLib module in restLibMock using a Jasmine Spy.  The fake “defaultValuesUsingSelect” just returns an result where the fields selected are populated with their own values, and the 1 just means that one item should be returned:

spyOn(restLibMock.CrmWebApiLib, "getList").and.callFake((r) => CrmWebApiLibStubs.getList.defaultValuesUsingSelect(r, 1));

The next 3 lines are setup for injecting the mock as CrmWebApiLib module, into CustomerStage.  It accepts a callback with a parameter that has a CustomerStage Property that has the mock CrmWebApiLib injected:

new Squire()
    .mock(restLibMock.path, restLibMock)
    .require(["scripts/lead/CustomerStage"], async (m: { CustomerStage: typeof CustomerStage }) => {

The final lines are the actual test that needs to be performed.  Call setLocalityFilter, and verify that that Locality attribute has had it’s value set:

await m.CustomerStage.setLocalityFilter();
expect(CommonLib.getValue(LeadCommon.fields.locality)).toBeTruthy("because the locality should have been set because only one locality exists for the given city/state");
done();

Even if you do understand how this works now, I’d be willing to bet the “future you” who looks at it in 5 days, will require significant time/energy to re-figure out what it’s doing.  No bueno.

Here is the refactored version with the helper method:

it("should set locality if a single locality exists for the state/county", async (done) => {
    const sut = await getCustomerStageWithMockedCrmWeb(1);
    await sut.setLocalityFilter();
    expect(CommonLib.getValue(LeadCommon.fields.locality)).toBeTruthy("because the locality should have been set because only one locality exists for the given city/state");
    done();
});

function getCustomerStageWithMockedCrmWeb(numResults: number): Promise<typeof CustomerStage> {
    return new Promise((resolve) => {
        spyOn(restLibMock.CrmWebApiLib, "getList").and.callFake((r) => CrmWebApiLibStubs.getList.defaultValuesUsingSelect(r, numResults));
        new Squire()
            .mock(restLibMock.path, restLibMock)
            .require(["scripts/lead/CustomerStage"], (m: { CustomerStage: typeof CustomerStage }) => {
                resolve(m.CustomerStage);
            });
    });
}

The actual test method is now dead simple, Get the SUT (Subject Under Test), set the locality filter, and assert that the locality was set and the new helper method “getCustomerStageWithMockedCrmWeb” can now be reused in the other three calls with minimal coding required/repeated.  If “future you” looked at that code in 5 months, it would immediately make sense.    Refactor, Refactor, Refactor.