Problem
I’m currently working on a project that I had to create basically a running queue of items. My thought was to do this by a list, but surprisingly there weren’t any methods to remove the first sequence or last sequence of items in a list that I found. I wrote the following class with extension methods for List<T>:
using System;
using System.Collections.Generic;
using System.Linq;
namespace Extensions
{
public static class ExtensionClass
{
public static bool TrimFirst<T>(this List<T> list, int maxCount)
{
try
{
if (list.Count <= maxCount)
{
throw new InvalidOperationException
("Items in list <= maxCount, unable to trim list.");
}
while (list.Count > maxCount)
{
T first = list.First<T>();
list.Remove(first);
}
return true;
}
catch
{
return false;
}
}
public static bool TrimLast<T>(this List<T> list, int maxCount)
{
try
{
if (list.Count <= maxCount)
{
throw new InvalidOperationException
("Items in list <= maxCount, unable to trim list.");
}
while (list.Count > maxCount)
{
T last = list.Last<T>();
list.Remove(last);
}
return true;
}
catch
{
return false;
}
}
public static bool ItemExists<T>(this List<T> list, T item)
{
foreach (T listItem in list)
{
if (item.ToString() == listItem.ToString())
{
return true;
}
}
return false;
}
}
}
This seems something that I will likely use in the future, so I would like to optimize it and make it as flexible as possible. Does anyone have any suggestions? For reference, the try/catch/exception are in case I need to modify the code to deal with that in the future, although I was not quite sure about the way that I wrote that part of the code. Anyone that needs this function, feel free to use this code!
I have also added the ItemExists() method since I hate the Any() and Exists() methods’ use of a predicate. As far as I know, this method processes the items by enumeration, just at the built-in methods do, except they possibly use a Parallel.ForEach loop. Any suggestions for optimizing this method?
Solution
if (list.Count <= maxCount)
{
throw new InvalidOperationException("Items in list <= maxCount, unable to trim list.");
}
This doesn’t seem like an exceptional case to me. If the idea is to trim the list to a certain size and the list is already smaller than that size — the job is already done! In that case I would replace this with return true;.
while (list.Count > maxCount)
{
T last = list.Last<T>();
list.Remove(last);
}
This is the main guts of your method. Since the preceding if statement has already established that list.Count > maxCount, you know that the loop is always going to run at least one. Therefore you could replace this with a do-while loop. This is a negligible performance improvement.
do
{
T last = list.Last<T>();
list.Remove(last);
}
while (list.Count > maxCount)
catch
{
return false;
}
Two problems here.
- YAGNI
- Using boolean values to indicate whether or not the method was successful is a design flaw. This harks back to the old HRESULT days which were universally loathed. Instead, let the exception do the job of alerting the calling code to errors (this is what exceptions are for). In the case that there is no exception, there is no use for the returned
truevalue.
You could also consider returning a reference to the list itself from the method, allowing people to use method chaining syntax, which is all the rage these days – especially with LINQ..
public static List<T> TrimFirst<T>(this List<T> list, int maxCount)
{
if (list.Count <= maxCount)
return list;
do
{
T first = list.First<T>();
list.Remove(first);
}
while (list.Count > maxCount);
return list;
}
One final thing. It’s usually a good idea to defend against NullReferenceExceptions by asserting that your argument is not null.
if (list == null)
throw new ArgumentNullException("list");
ArgumentNullExceptions are always preferable to NullReferenceExceptions.
public static bool ItemExists<T>(this List<T> list, T item)
{
foreach (T listItem in list)
{
if (item.ToString() == listItem.ToString())
{
return true;
}
}
return false;
}
This method has some problems:
Using ToString() for equality comparison is likely to yield many false positives. The default implementation for ToString is just to return the fully qualified name of the type eg. "System.SomeClass". This will mean that many objects will be considered equal when they are really not!
It would be more semantically correct to use the Object.Equals method, since this method contains sensible defaults for types which have chosen not to override it (reference equality for reference types, value equality for value types).
public static bool ItemExists<T>(this List<T> list, T item)
{
foreach (T listItem in list)
{
if (item.Equals(listItem))
return true;
}
return false;
}
More preferable still is to get an IEqualityComparer instance for the objects. This will take into account special cases such as classes which implement System.IEquatable. The equality comparer can be found with System.Collections.Generic.EqualityComparer<T>.Default.
public static bool ItemExists<T>(this List<T> list, T item)
{
var comparer = EqualityComparer<T>.Default;
foreach (T listItem in list)
{
if(comparer.Equals(item, listItem))
return true;
}
return false;
}
Even better yet, the List class already has a method called Contains which does all of these things 🙂
list.Contains(item);
LINQ has extension methods for any IEnumerable<> which will help with this.
There’s .Take(x), which will return the first x elements, and .Skip(x) which will skip x elements, then return the rest.
Of course, these don’t actually change the List<> they’re operating on – they just return a new one. But that’s a safer pattern to use – it means you have the option of keeping the untrimmed version around if you need to.
Consider using RemoveRange method as follows
list.RemoveRange(index, count)
Note: It’s not defined for IList.