12
\$\begingroup\$

This method removes n elements starting from a given index, from an array of a given type. If n is positive it removes elements forwards, if it is negative it removes them backwards (e.g. for an array {1,2,3,4,5} Remove(2,2) results in {1,2,5}; Remove (2,-2) results in {1,4,5})

It there a better way to do it?

public static class ArrayExtensions
{
    public static T[] RemoveAt<T>(this T[] array, int idx, int len)
    {
        T[] newArray;

        if (len > 0)
        {
            if (idx + len > array.Length)
                len = array.Length - idx;

            newArray = new T[array.Length - len];
            if (idx > 0)
                Array.Copy(array, 0, newArray, 0, idx);

            if (idx < array.Length - 1)
                Array.Copy(array, idx + len, newArray, idx, array.Length - idx - len);
        }
        else
        {
            newArray = new T[array.Length + len];
            if (idx > 0)
                Array.Copy(array, 0, newArray, 0, idx + len);

            if (idx < array.Length - 1)
                Array.Copy(array, idx, newArray, idx + len, array.Length - idx);
        }            

        return newArray;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ It might be worth looking into SkipWhile and TakeWhile with Linq, you could condense quite a bit. \$\endgroup\$ – Greg Jun 21 '16 at 15:25
  • \$\begingroup\$ Using the index version of Linq's 'where', would be quite straight forward. array.where( (number, index) => index < startIndex || index > endIndex).ToArray(). Where startIndex and endIndex are precomputed and validated. \$\endgroup\$ – hocho Jun 23 '16 at 7:15
12
\$\begingroup\$

I believe that if the len < 0, it's better to adjust idx and len values. This will eliminate two branches of code.

Another suggestion is to use more readable variable names.
For instance, method arguments could be named startIndex and length respectively.

public static T[] RemoveAt<T>(this T[] array, int startIndex, int length)
{
    if (array == null)
        throw new ArgumentNullException("array");

    if (length < 0)
    {
        startIndex += 1 + length;
        length = -length;
    }

    if (startIndex < 0)
        throw new ArgumentOutOfRangeException("startIndex");
    if (startIndex + length > array.Length)
        throw new ArgumentOutOfRangeException("length");

    T[] newArray = new T[array.Length - length];

    Array.Copy(array, 0, newArray, 0, startIndex);
    Array.Copy(array, startIndex + length, newArray, startIndex, array.Length - startIndex - length);

    return newArray;
}
\$\endgroup\$
5
\$\begingroup\$

On top of @Dmitry's answer

This method wouldn't be easy to use IMO. Having the ability to pass a negative length is just not natural.

In addition you didn't validate the methods argument enough. Assume a passed in array which is having only one item is used with a len ==-2 this would just blow in your face with an exception.

\$\endgroup\$
  • \$\begingroup\$ I don't agree. I think it's fine to specify a negative number to remove items before an item. It sound pretty natural to me. \$\endgroup\$ – t3chb0t Jun 21 '16 at 15:45
  • 2
    \$\begingroup\$ @t3chb0t if you often e.g use List <T>.RemoveRange () you wouldn't feel like that. \$\endgroup\$ – Heslacher Jun 21 '16 at 15:49
  • 3
    \$\begingroup\$ I agree with Heslacher. Passing negative values as count parameter might be natural in other languages (Python comes to mind), but it is definitely not something your average C# developer would instantly recognize and understand. To my knowledge there is not a single method in .Net framework which takes negative count and does not throw. If this functionality is a must for some reason, I would rather have some RemoveBefore method, which takes positive count, but removes items in the opposite direction. \$\endgroup\$ – Nikita B Jun 22 '16 at 7:15
  • \$\begingroup\$ If one believes that a negative length or count is not natural, then wouldn't an extension of that belief be that startIndex should refer to absolute index position and not be relative to the end with a negative count? \$\endgroup\$ – Rick Davin Jun 22 '16 at 14:05
  • \$\begingroup\$ Pity the first point about negative length are overshadowing the very valid second point about argument validation. \$\endgroup\$ – Rick Davin Jun 22 '16 at 14:12
4
\$\begingroup\$

My answer is similar to Dmitry's, but I will use LINQ and check the original startIndex and length. I prefer to not mutate things if possible, especially parameters, even if they're value types, so that it's consistent with how you treat reference types.

public static T[] RemoveAt<T>(this T[] array, int startIndex, int length)
{
    if (array == null)
        throw new ArgumentNullException("array");

    if (startIndex < 0 || startIndex > array.Length - 1)
        throw new ArgumentOutOfRangeException("startIndex");
    if (startIndex + length < 0 || startIndex + length > array.Length)
        throw new ArgumentOutOfRangeException("length");

    int newStartIndex = length >= 0 ? startIndex : (startIndex + 1 + length);
    int newLength = Math.Abs(length);
    return array.Take(startIndex).Concat(array.Skip(startIndex + length)).ToArray();
}

That will throw an exception if any of the skipped elements are outside of the array. If you want, you can make your method like Take(), where it will remove the specified indexes in the array and ignore the specified indexes that are outside the array.

public static T[] RemoveAt<T>(this T[] array, int startIndex, int length)
{
    if (array == null)
        throw new ArgumentNullException("array");

    int newStartIndex = length >= 0 ? startIndex : (startIndex + 1 + length);
    int newLength = Math.Abs(length);
    if(newStartIndex < 0)
    {
        newLength = Math.Max(newLength - newStartIndex, 0);
        newStartIndex = 0;
    }
    return array.Take(startIndex).Concat(array.Skip(startIndex + length)).ToArray();
}
\$\endgroup\$
4
\$\begingroup\$

Well, I guess even without LINQ you could make several things better.

Code Logic suggestions

1) Regardless of the length's sign, the resulting array length is going to be the same. Thus this logic

    T[] newArray;
    if (len > 0)
    {
        newArray = new T[array.Length - len];
    }
    else
    {
        newArray = new T[array.Length + len];
    }

can be simplified to

    var newArray = new T[array.Length - Math.Abs(len)];

2) The code has kind of asymmetric parameter validation. You have the check for a positive case

    if (idx + len > array.Length)
        len = array.Length - idx;

but you do not have it for the negative case. In your example ({0,1,2,3,4}) I will see different behavior when calling

    array.RemoveAt(2, 4);
    array.RemoveAt(2, -4);

though actually doing similar things.

3) Also I would throw exceptions for all incorrect indexes and lengths. Look, here I am actually accessing similar non-existing place

    array.RemoveAt(4, 3);
    array.RemoveAt(6, 1);

but getting different behavior. First is considered to be OK, and second brings a .NET exception. Both reactions, imo, are not really nice.

4) Actually, I generally agree with Heslacher, negative length looks weird in C#. This feature is redundant because you can always just shift index in the method call. Simultaneously, logic becomes more complicated.

But if I desperately wanted to implement it, I would prefer to still have non-negative length and just add an enum parameter for direction with default value

    public static T[] RemoveAt<T>(
        this T[] array, 
        int idx, 
        int len, 
        Direction direction = Direction.Forward) 

Code style suggestions

1) I support other responders about naming here, and also I would prefer "count" instead of "length" as it is done in the similar Microsoft method. And, well, RemoveRange is clearer then RemoveAt.

2) Also you should add braces around if bodies, here is some reasoning.

3) Also you could create two little methods with filling the resulting array to improve readability, e.g. FillLeftHalf() & FillRightHalf().

\$\endgroup\$
  • \$\begingroup\$ Nice answer. I would add for the last item that the two little methods should be private. Also might be overkill to have Direction.Forward enumeration when a simple bool would suffice. I would name the bool reverseDirection so that the default value of false would mean forward. \$\endgroup\$ – Rick Davin Jun 22 '16 at 14:10
  • \$\begingroup\$ @RickDavin Well, I though about simple bool but perhaps it is also not really in C# style. Compare to this method or this one. Both use last bool param to slightly extend the logic of the method, not to change or switch it. But maybe there are other examples or this is just a question of taste. \$\endgroup\$ – psfinaki Jun 23 '16 at 9:21
2
\$\begingroup\$

like @Greg said in his comment - you can make it really short with LINQ and you don't have to check any ranges:

static IEnumerable<T> Remove<T>(this IEnumerable<T> items, int startIndex, int count)
{
    return
        count > 0
        ? items.TakeWhile((x, i) => i < startIndex).Concat(items.Skip(startIndex + count))
        : items.TakeWhile((x, i) => i < (startIndex + count + 1)).Concat(items.Skip(startIndex + 1));
}
\$\endgroup\$
  • \$\begingroup\$ TakeWhile() will ignore negative indexes, but Skip() will throw an exception for negative indexes, so you should be consistent and use TakeWhile() with SkipWhile() or Take() with Skip(). \$\endgroup\$ – Risky Martin Jun 21 '16 at 16:17
  • \$\begingroup\$ Yeah, I realize now that mixing them here wasn't probably my best idea today ;-) \$\endgroup\$ – t3chb0t Jun 21 '16 at 18:14
  • \$\begingroup\$ What I like best about this answer is your use of IEnumerable<T> rather than limiting it to just an array of T[]. For simple looping, this is great and uses less memory, and the developer can always use a .ToArray() later if need be. \$\endgroup\$ – Rick Davin Jun 22 '16 at 14:20

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.