Fixing Code Smells in Three Lines

Can so many programming pitfalls be so densely packed, and can we untangle an example that isn’t just academic? Let’s start with some code.

public static IEnumerable<T> Part<T>(T[] a, int n, int s, int c)
{
	var A = new T[c > s ? c : s];
	var t = 0;  //track how many times we add

	for (var i = (n - 1) * s; i < a.Length; i++)
		if (++t <= s)
			A[t - 1] = a[i];

	return A;
}

If you could only see the method signature, would you have any idea what this does? Does having visibility into the body logic help you understand it any better? This function is used for paging arrays, and while it does work and may even perform extremely well, the developer who wrote this is relying upon others to be as clever as them. To quote Martin Fowler, “Any fool can write code that a computer can understand. Good programmers write code that humans can understand.” This is typical of code that I’ve encountered and it’s full of poor practices, antipatterns, and code smells:

1. Naming Things – First, the function name should reflect what it is doing, performing a paging operation. Poor or short variable names make it difficult to know what they are used for. If you must leave a comment describing a variable, why not just name the thing appropriately?
2. Code Readability/Simplicity – Good code should be easily readable, whether you are a novice or seasoned developer. Also, is this the simplest way of achieving the desired result?
3. Primitive Obsession/Long Parameter List – With all those integers in a row, it’s hard to keep straight the correct value I should be passing.
4. You Aren’t Going to Need It (YAGNI) – Resist the temptation to add features until you actually need them. Do we really need an option to produce an array larger than a page of items?
5. Magic Numbers – Why are we subtracting “1”?
6. Not Invented Here – Some developers distrust things they did not create themselves. There are LINQ extension methods that work great, so why not use them?

Let’s fix this mess.

public static IQueryable<T> ApplyPaging<T>(this IOrderedQueryable<T> query, (int PageNumber, int PageSize) options)
{
	const int PAGE_START_NUMBER = 1;
	return query.Skip((options.PageNumber - PAGE_START_NUMBER) * options.PageSize).Take(options.PageSize);
}

This is one of the first pieces of code that I often contribute to projects. Paging operations are so common, it is a wonder why this extension method isn’t included in the framework. Implementing paging is fairly straightforward by calling .Skip()/.Take() everywhere you need it, but that would be repetitive.

Even if you couldn’t see the logic, from the name, it is obvious that it produces a page of items, and it will only do this when it makes sense. Have you ever pondered the meaning of a page of random data? The IOrderedQueryable type constraint ensures that this is used appropriately and provides an assurance that you are going to get a meaningful result.

Notice the named tuple parameter. It is a combination of related information, with communicative names. Why not just keep two integer parameters? Primitive obsession and long parameter lists can be problematic especially when like types are defined next to each other. Was that “PageSize” first, then “PageNumber”, or worse with integers named “n”, “s”, and “c”? A variable to control collection capacity adds unnecessary complexity and is a violation of YAGNI, so it is gone. The remaining options are so closely related that they naturally lend themselves as a data object. A named tuple is the quickest and easiest way of achieving this. Could we have created a class just for these options? Sure, but that is not doing the simplest thing that could possibly work.

What’s with the constant? We could have easily subtracted 1. Hard-coding a value is problematic because it is a magic number and the "1" doesn’t tell me anything about why it needs to be there. You can tell by the constant’s name what it is used for, leading to better code documentation.

There you have it. A useful, repeatable function that produces predictable results with unambiguous input. I like the simplicity of this, and it is infinitely more readable!

Improve Performance

In production I have always been bothered by the extra performance hit required to obtain the total number of pages. Unless you are implementing some sort of infinite scroll of items, it is usually helpful to know how much data one must page through. If you are using EF, then I recommend adding the Z.EntityFramework.Plus library. It has a nifty way of deferring execution of queries. All deferred queries will be executed in a batch, resulting in just one connection to your database server but with multiple result sets returned. You execute the batch when you start using any of the results.

What does an extension method look like if we are batching?

public static ICollection<T> GetPage<T>(this IOrderedQueryable<T> query, (int PageNumber, int PageSize) options, out int totalRecords)
{
	//use our new extension method, defer executing the query
	var futureQuery = query.ApplyPaging(options).Future();

	//defer executing a count for all the records in the same query
	totalRecords = query.DeferredCount().FutureValue();

	//execute batch
	return futureQuery.ToList();
}

Note that we created two deferred queries and then executed them immediately. We could execute asynchronously (await futureQuery.ToListAsync()), but we would have to move the “out” variable into the return object. Also, the return type should not be an IQueryable (the library doesn't allow for it anyway). Paging should be the last thing you do; otherwise, you end up manipulating pages. The first extension method does return an IQueryable because the .Future() extension requires that type.

Can this method be improved? Of course, but it would require more lines of code and this post was about brevity. The improvements that could be made are adding guard clauses, or better yet, encapsulating valid integers as value objects for “PageNumber” and “PageSize”.