A Secret to Writing Nice Code

We all know the basics of writing nice, readable, maintainable code. Shorter code is usually better, so omit needless lexemes. Use meaningful names because heavy commenting is a hack. Parallelism (the literary kind) trumps all. But I find one of the unsung heroes of nice code is choosing to create just the right helper function.

Here's a react example.

{items.length < 2 ? (
  <>{items.map(item => renderItem(item, allOtherVars)}</>
) : (
  <ItemGroup>{items.map(item => renderItem(item, allOtherVars)}</ItemGroup>
)}

We want the ItemGrouping wrapper if the array has more than one item. The renderItem function is non-trivial and only exists because we don't want to duplicate all the code inside it. But declare a helper like so.

export const ItemGroupIf = ({ if: ig, children, ...otherProps }: Props & { if: boolean }) =>
  ig ? <ItemGroup {...otherProps}>{children}</ItemGroup> : <>{children}</>;

This allows us to rewrite the client code as this.

<ItemGroupIf if={items.length > 1}>
  {items.map(item => {
    /* the body of renderItem */
  })}
</ItemGroupIf>

This allows us to avoid declaring the workaround function renderItem, avoid passing around several variables to it, to almost hide the condition (possibly reducing cyclomatic complexity depending on how its counted), and primarily just removes the one-off "wiring" in favor of a re-usable component that provides these same benefits wherever else in the codebase this issue occurs.

Yet the helper in question doesn't do anything complex or "worth saving" in a dedicated utility function, so if you're just browsing the Utility class or file or wherever you keep these things and happen upon it, it seems like a stupid function.

Here's another example in C#.

The following function, which is one of many just like it, is getting information from several API calls and amalgamating them into a larger object. A few of the API calls are required and a few are optional. Each call returns a wrapper object containing either the result or an error object. So we use a lot of if statements checking return values.

public FinalResult GetAggregateResult(int id)
{
    var primary = await FirstCall(id);
    if (primary.Problem != null) 
    {
       logger.Error(primary.Problem);
       return new FinalResult(primary.Problem);
    }

    var secondary = await SecondCall(primary.secondId);
    if (secondary.Problem != null) 
    {
       logger.Error(secondary.Problem);
       return new FinalResult(secondary.Problem);
    }

    var optional1 = await OptionalCall1(id);
    if (optional1.Problem != null) 
    {
       logger.Info(optional1.Problem);
    }

    var optional2 = await OptionalCall2(primary.pieceId);
    if (optional2.Problem != null) 
    {
       logger.Info(optional2.Problem);
    }

    return new FinalResult() {
      x = primary.result;
      y = secondary.result.moneyShot;
      a = optional1.Problem != null ? optional1.result : null;
      b = optional2.Problem != null ? optional2.result?.piece?.subpiece : null;
    };
}

The try-catch was invented so we could separate the happy and unhappy path, simplifying both. But none of our calls can actually throw on error because the caller might decide its optional. Exceptions throws are expensive and should be avoided for normal processing, and in many of our use-cases a 404 is perfectly normal. Yet the if statements dominate the conversation the function is trying to have with its upper and lower levels.

So here's a few stupid functions. The first two are instance methods on the returned wrappers, the second two are C# extension method on C#'s awaitable Task type. (Define ResultException as a simple wrapper if not already done so.)

public T ThrowIfError()
{
    if (Problem != null)
        throw new ResultException(this);
    else
        return Value;
}

public T LogIssue(Logger logger)
{
    if (Problem != null)
        logger.Info(Problem);
    return Value;
}

public static async Task<T> ThrowIfError<T>(this Task<LowerResult<T>> gettingResult) 
    => (await gettingResult).ThrowIfError();

public static async Task<T> LogIssue<T>(this Task<LowerResult<T>> gettingResult, Logger logger) 
    => (await gettingResult).LogIssue(logger);

Now we write our caller code to use exception handling, but we, not the lower levels, decide if a call throws or not.

public FinalResult GetAggregateResult(int id)
{
  try 
  {
    var primary = await FirstCall(id).ThrowIfError();

    var secondary = await SecondCall(primary.secondId).ThrowIfError();

    var optional1 = await OptionalCall1(id).LogIssue(logger);

    var optional2 = await OptionalCall2(primary.pieceId).LogIssue(logger);

    return new FinalResult() {
      x = primary;
      y = secondary.moneyShot;
      a = optional1;
      b = optional2?.piece?.subpiece;
    };
  } 
  catch (ResultException rex)
  {
    logger.Error(rex.Problem);
    return new FinalResult(rex.Problem);
  }
}

The instance method ThrowIfError either unwraps and returns the Result, or unwraps the Error and throws an exception, separating the unhappy path. No changes happened to the lower level calls. The extension method allows us to avoid extra parenthesis in (await Call(x)).ThrowIfError() which are easy to forget and hard on the eyes.

Similarly the LogIssue unwraps the Result and always returns it, even if null, but quietly logs any issue so we don't need to clutter the caller with checks and logs.

We all like code where it's easy to see what's going on, and we almost always prefer less code over more. But in all the conversation I hear about naming conventions and architecture and "how we do things here", I rarely hear anyone championing the humble helper function as a strong tool for producing nice code.