Do a Lot to Do Nothing

Today's anonymous submitter works in finance. I'll let them start the introduction:

This is a legacy application that has been running for nearly a decade in production so one could say that it's been thoroughly tested by daily production use and nothing needs changing

This is a collection of two C# methods, and we'll start with ValueAGPFund, which isn't a WTF per se, but definitely not code I'd want to maintain either.

public Valuation ValueAGPFund(int valuationId, ValueAFundParameters parameters, CapitalAccount capitalAccount, int? lotId)
{
    if (parameters.UseActiveCoefficientSet)
    {
        parameters.CoefficientSet = _coefficientSetQueries.GetActive();
    }
    parameters.InternationalDveCoefficientSets = _coefficientSetQueries.GetInternationalDveActive();
    var referenceData = _referenceDataFactory.CreateReferenceData(parameters, capitalAccount);
    if (lotId != null)
    {
        var di = referenceData.FundDirectInvestments.Where(x => x.PositionId == lotId);
        referenceData.FundDirectInvestments = di;
    }

    var countryMappings = _countryQueries.GetFullIsoCountryList();
    var valuation = _valuationFactory.Initialise(referenceData, parameters, countryMappings);
    valuation = ApplyValuators(valuation, referenceData, _valuatorFactory.CreateValuators(valuation, this));

    var valuationForCoverage = _valuationQueries.GetWithDirectValuationsAndFundValuations(valuationId);
    valuation = ApplyCoverage(valuation, valuationForCoverage);

    foreach (var fv in valuation.FundValuations)
    {
        _logger.Info($"Debugging distributions: for fund (parameter fund id = {parameters.FundId}, valuation fund id = {valuation.FundId}, fund valuation fund id = {fv.GpFundId}) in valuation {valuationId}," +
            $" loaded fund investment distributions from {string.Join(", ", fv.FundInvestmentDistributions.Select(x => $"{x.InvestmentId}:{x.TransactionDate:yyyy/MM/dd}"))}");
    }

    foreach (var fv in valuation.FundValuations.Where(x => parameters.InvestmentIds.Contains(x.EqtInvestmentId)))
    {
        fv.ValuationId = valuationId;
        _fundValuationCommands.Add(fv);
    }

    foreach (var dv in valuation.DirectValuations.Where(x => x.LotIdDiOnly == lotId))
    {
        dv.ValuationId = valuationId;
        _directValuationCommands.Add(dv);
    }

    foreach (var vw in valuation.ValuationWarnings)
    {
        vw.ValuationId = valuationId;
        _valuationWarningCommands.Add(vw);
    }

    var previousValuation = CheckPreviousValuationIfRequired(valuationId, parameters, capitalAccount, lotId);

    if (previousValuation != null)
        valuation.ChildValuations.Add(previousValuation);

    if (parameters.Frequency == ValuationFrequency.Daily)
    {
        var unapprovedValuations = _valuationQueries.GetList(valuation.FundId, valuation.ValuationDate, valuation.Frequency, valuation.Purpose)
                                                    .Where(x => x.IsApproved == ValuationStatus.Unapproved)
                                                    .ToList();

        _valuationCommands.Delete(unapprovedValuations.Select(x => x.Id).ToArray());
    }

    valuation.Id = valuationId;
    _valuationCommands.Update(valuation);
    _valuationCacheService.Refresh(valuation.Frequency, true);

    return valuation;
}

The key problem with this function is that it's got loads of side effects. It modifies the parameters parameter, which while it was passed by value, the value itself is a reference, so you are updating it on the caller, whether the caller likes it or not. It also modifies a bunch of internal class members. It's also just… doing a lot of different steps. It's not a WTF, but it's bad code. Note the call in the middle to CheckPreviousValuationIfRequired- we're going to come back to that in a second.

Let's take a look at how it's called.

private Valuation CheckPreviousValuationIfRequired(int valuationId, ValueAFundParameters parameters, CapitalAccount capitalAccount, int? lotId)
{
    if ((parameters.Frequency == ValuationFrequency.Quarterly || parameters.Frequency == ValuationFrequency.Monthly)
        && ValuationPurposeHelper.UserGenerated(parameters.Frequency).Contains(parameters.Purpose))
    {
        var inPeriodParams = new ValueAFundParameters
        {
            FundId = parameters.FundId,
            ValuationDate = parameters.ValuationDate.GetPreviousValuationDate(parameters.Frequency),
            CreatedBy = parameters.CreatedBy,
            Purpose = ValuationPurpose.InPeriodCalculation,
            Frequency = parameters.Frequency,
            InvestmentIds = parameters.InvestmentIds,
            UseActiveCoefficientSet = true,
            UseAmericanDve = parameters.UseAmericanDve,
            ValuationOptions = parameters.ValuationOptions
        };

        var openingValuation = _valuationQueries.GetInPeriodOpeningValuation(inPeriodParams.FundId, inPeriodParams.ValuationDate, valuationId);

        //return openingValuation == null
        //       ? null
        //       : ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId);
        return openingValuation == null
                ? ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId)
                : null;
    }

    return null;
}

This function checks the input parameters. Depending on the values, it will either return null, or it will call ValueAGPFund. Wait a second, ValueAGPFund calls this function. That's not good.

But let's really focus in on the return statement and its comment:

        //return openingValuation == null
        //       ? null
        //       : ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId);
        return openingValuation == null
                ? ValueAGPFund(openingValuation.Id, inPeriodParams, capitalAccount, lotId)
                : null;

The current version checks if openingValuation is null, and if it is, tries to access it, thus triggering a NullReferenceException. This function either returns null or throws a NullReferenceException. So all that worrying about side effects and circular calls doesn't matter, but this likely isn't correct. The comment indicates that there used to be a correct version, which only called ValueAGPFund if the valuation wasn't null- but that version likely had all the problems of circular calls and unpredictable side effects.

As it stands, the application as a whole works. Since CheckPreviousValuationIfRequired only ever returns null or throws an exception, and since ValueAGPFund is only called from here, it looks like these functions could just both be removed without problems. But our submitter is wary of doing that:

The problem is that I first need to figure out whether 1) this piece of code produces any side effects and 2) nobody is relying on the System.NullReferenceException being thrown here.

No worries, though, right? I'm sure your unit tests will catch any regressions caused by removing that. Because this is the kind of code that definitely has great unit tests.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.