Writing elegant code

I watched this video analyzing a chunk of code, in the hopes of refactoring it.

Always Return Early in Your Code | Code Cop #024 by Nick Chapsas (YouTube)

The original code is the laughably overblown example below.

public List<int> ProcessData(List<int> data)
{
  if (data != null)
  {
    if (data.Count > 0)
    {
      var processedData = new List<int>();
      foreach (var d in data)
      {
        processedData.Add(d * 2);
      }
      return processedData;
    }
    else
    {
      return new List<int>();
    }
  }
  else
  {
    return null;
  }
}

Nick rewrote it as the following:

List<int> ProcessData(List<int>? data)
{
  if (data is not { Count > 0 })
  {
    return []:
  }

  return data.Select(d => d * 2). ToList();
}

Nick’s is OK, but I don’t understand why he bothers to check for Count > 0 when Select() already short-circuits on this case.

@DmitryKandiner rewrote it as the following:

List<int> ProcessData(List<int>? data) => data?.Select(d => d * 2).ToList() ?? [];

This is really short and avoids the unnecessary length-check but it still deals with nullable code, which is silly. There is no need for this function to handle possibly null input data.

I commented the following:

We can also drop the null-check if we have nullability enabled (which any modern project should). Also, I prefer defining APIs with enumerables rather than lists, but if the design insists, I would do it with two methods. This gives callers the option of building lists but doesn’t require them to do so.

List<int> ProcessList(List<int> data)
{
    return ProcessSequence(data).ToList();
}

IEnumerable<int> ProcessSequence(IEnumerable<int> data)
{
    return data.Select(d => d * 2);
}

To which @swozzares replied that I could eliminate the return by using => (called an “expression body”). So I updated the sample with:

List<int> ProcessList(List<int> data)
  => ProcessSequence(data).ToList();

IEnumerable<int> ProcessSequence(IEnumerable<int> data)
  => data.Select(d => d * 2);

And I might as well include the test:

[Test]
public void TestProcessSequence()
{
    List<int> input = [1, 2, 3, 4, 5];
    List<int> expected = [2, 4, 6, 8, 10]; 
    
    Assert.That(ProcessList(input), Is.EqualTo(expected));
    Assert.That(ProcessSequence(input), Is.EqualTo(expected));
}

Someone named @Me_myself_and_I took offense to my comment that projects should be using nullability.

“Except that there are a lot of projects that pre-date that. Its good for new projects sure, but probably not worth the effort to refactor and re-test existing large projects. Devs really need to learn to think about existing systems and long-term maintainability not just new code.”

I think we should be clear about the context. The commentator’s argument is the same one I would make when I work on a legacy project. I think we should take care to show that, if you started a project in C# in the last five years, you shouldn’t be checking for null everywhere anymore. If you turned off nullable checks, then, yeah, you’ll still have to check for it. But you should be designing null-free APIs— where null is not allowed by default.