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;
}
}
5 Answers 5
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;
}
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.
-
\$\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– t3chb0t2016年06月21日 15:45:17 +00:00Commented Jun 21, 2016 at 15:45
-
2\$\begingroup\$ @t3chb0t if you often e.g use
List <T>.RemoveRange ()
you wouldn't feel like that. \$\endgroup\$Heslacher– Heslacher2016年06月21日 15:49:39 +00:00Commented Jun 21, 2016 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 negativecount
and does not throw. If this functionality is a must for some reason, I would rather have someRemoveBefore
method, which takes positivecount
, but removes items in the opposite direction. \$\endgroup\$Nikita B– Nikita B2016年06月22日 07:15:55 +00:00Commented Jun 22, 2016 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– Rick Davin2016年06月22日 14:05:12 +00:00Commented Jun 22, 2016 at 14:05 -
\$\begingroup\$ Pity the first point about negative length are overshadowing the very valid second point about argument validation. \$\endgroup\$Rick Davin– Rick Davin2016年06月22日 14:12:21 +00:00Commented Jun 22, 2016 at 14:12
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();
}
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().
-
\$\begingroup\$ Nice answer. I would add for the last item that the two little methods should be
private
. Also might be overkill to haveDirection.Forward
enumeration when a simple bool would suffice. I would name the boolreverseDirection
so that the default value of false would mean forward. \$\endgroup\$Rick Davin– Rick Davin2016年06月22日 14:10:50 +00:00Commented Jun 22, 2016 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– psfinaki2016年06月23日 09:21:15 +00:00Commented Jun 23, 2016 at 9:21
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));
}
-
\$\begingroup\$
TakeWhile()
will ignore negative indexes, butSkip()
will throw an exception for negative indexes, so you should be consistent and useTakeWhile()
withSkipWhile()
orTake()
withSkip()
. \$\endgroup\$Risky Martin– Risky Martin2016年06月21日 16:17:06 +00:00Commented Jun 21, 2016 at 16:17 -
\$\begingroup\$ Yeah, I realize now that mixing them here wasn't probably my best idea today ;-) \$\endgroup\$t3chb0t– t3chb0t2016年06月21日 18:14:37 +00:00Commented Jun 21, 2016 at 18:14
-
1\$\begingroup\$ What I like best about this answer is your use of
IEnumerable<T>
rather than limiting it to just an array ofT[]
. 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– Rick Davin2016年06月22日 14:20:22 +00:00Commented Jun 22, 2016 at 14:20
SkipWhile
andTakeWhile
with Linq, you could condense quite a bit. \$\endgroup\$