1
\$\begingroup\$

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?

\$\endgroup\$
  • 2
    \$\begingroup\$ Did you consider a queue (FIFO) or stack (LIFO) collection? \$\endgroup\$ – nickles80 Jul 24 '13 at 16:40
  • \$\begingroup\$ I have considered using a queue, but the way I thought of implementing it would require rewriting a large amount of code. I have realized now that I could just enqueue then dequeue with a while loop inside of an if statement, but that is essentially what I am doing here. Would there be any advantages to using that technique? \$\endgroup\$ – danielunderwood Jul 24 '13 at 17:39
  • 1
    \$\begingroup\$ @danielu13 - There's also LinkedList<> which specifically provides functions for adding to or removing from both ends. \$\endgroup\$ – Bobson Jul 25 '13 at 17:22
  • \$\begingroup\$ Great suggestion! I wasn't aware of that, but I looked it up and it offers performance gains for removing at the beginning of a list. I actually changed my code to use this. Thanks! \$\endgroup\$ – danielunderwood Jul 25 '13 at 17:55
2
\$\begingroup\$
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 true value.

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.

\$\endgroup\$
  • \$\begingroup\$ Great suggestions! In addition to removing the exception, I removed the try/catch entirely and will just do it in the code that is using these methods if necessary. I especially liked the suggestion about returning the list for method chaining. Thanks! \$\endgroup\$ – danielunderwood Jul 24 '13 at 15:46
  • \$\begingroup\$ @danielu13 No problem. FYI it's a bit of a faux-pas to edit the code in your question to match my suggestion. This invalidates my answer and could be confusing to other people who read this question. \$\endgroup\$ – MattDavey Jul 24 '13 at 15:51
  • \$\begingroup\$ My fault I've always wondered about that. Anyhow, I changed it back to help limit confusion \$\endgroup\$ – danielunderwood Jul 24 '13 at 15:54
  • \$\begingroup\$ No worries! Other people might have far better improvements to your original code anyway - you wouldn't want to limit your options :) \$\endgroup\$ – MattDavey Jul 24 '13 at 15:57
  • 1
    \$\begingroup\$ “This is a negligible performance improvement.” And also a small readability decrease, I think it's not worth it ion this case. \$\endgroup\$ – svick Jul 24 '13 at 16:58
2
\$\begingroup\$
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);
\$\endgroup\$
  • \$\begingroup\$ I figured that using ToString() would product errors if it were not defined for the object that it was being used on, but the application that I was building it for was using strings exclusively, so I didn't really focus on that. Thanks for the Contains suggestion though; I looked over that earlier, but I refactored my code to use that instead. \$\endgroup\$ – danielunderwood Jul 24 '13 at 17:36
2
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

Consider using RemoveRange method as follows

list.RemoveRange(index, count)

Note: It's not defined for IList.

\$\endgroup\$
  • \$\begingroup\$ I had looked at this before, but I just looked over it again and apparently before I missed the fact that it does indeed reindex the list so it can be reused at index 0. Thanks! \$\endgroup\$ – danielunderwood Jul 25 '13 at 16:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.