Ayende @ Rahien

It's a girl

Elegant code

I just like this code, so I thought I would publish it.

   1: public static class ArrayExtension
   2: {
   3:     public static T[] GetOtherElementsFromElement<T>(this T[] array , T element)
   4:     {
   5:         var index = Array.IndexOf(array, element);
   6:         if (index == -1)
   7:             return array;
   8:         return array.Skip(index + 1).Union(array.Take(index)).ToArray();
   9:     }
  10: }

And the unit test:

   1: public class ReplicationUnitTest
   2: {
   3:     [Fact]
   4:     public void Will_distribute_work_starting_with_next_node()
   5:     {
   6:         var nodes = new[] { 1, 2, 3 };
   7:         Assert.Equal(new[] { 3, 1 }, nodes.GetOtherElementsFromElement(2));
   8:         Assert.Equal(new[] { 1, 2 }, nodes.GetOtherElementsFromElement(3));
   9:         Assert.Equal(new[] { 2, 3 }, nodes.GetOtherElementsFromElement(1));
  10:         Assert.Equal(new[] { 1, 2, 3 }, nodes.GetOtherElementsFromElement(4));
  11:     }
  12: }

Comments

Justin Lee
02/07/2009 07:02 PM by
Justin Lee

I'm not too sure what you're doing, but I assume you're getting other elements other than "2" in the first case, other than "3" in the second case, other than "1" in the third case and other than "4" in the last case.

Couldn't you just have done a negate intersect? In SQL terms, Except.

Ayende Rahien
02/07/2009 07:03 PM by
Ayende Rahien

Notice the ordering of the elements there.

Justin Lee
02/07/2009 07:32 PM by
Justin Lee

Ah. Indeed. Now I see. Elegant indeed.

Peter Morris
02/07/2009 08:09 PM by
Peter Morris

I see what it is doing but can't see what you intend to use it for. Why when you remove the element do you want to swap the groups of numbers before/after its position in the source?

I think it's poorly named too :-) At first I'd have suggested

public static T[] Exclude <t(this T[] array , T element)

but seeing as it swaps the groups of numbers before and after I don't think that is sufficient, so without knowing your intent I can't think of a suitable name :-)

Ayende Rahien
02/07/2009 08:10 PM by
Ayende Rahien

Check the test name.

and now imagine that instead of numbers, you would have urls of nodes in a grid

Peter Morris
02/07/2009 08:29 PM by
Peter Morris

I read the test name, in fact the test told me more than the code. I don't know anything about workload distribution so your hint is lost on me :-)

I'd have expected items to be taken off the front, like a "round robin" approach, so I suspect the goal is to achieve something I am unfamiliar with.

PS: If you get a second :-)

stackoverflow.com/.../rhino-mocks-returning-a-d...

Ted
02/07/2009 08:36 PM by
Ted

An Array just holds stuff. If you want some kind of queue-like behavior, then you should properly abstract that into an appropriately named structure. The code is not self descriptive of its behavior and it could be argued that you are violating the SRP for Array by bolting this on via an extension method.

Andr&#233;s G. Aragoneses
02/07/2009 09:03 PM by
Andrés G. Aragoneses

This would be a good candidate for inclusion in Mono.Rocks. Do you know this library? It has a lot of useful extension methods.

Patrik H&#228;gne
02/07/2009 10:19 PM by
Patrik Hägne

I might be missing something but what are your reasons for not using IList(T) instead of array or even better IEnumerable(T)? As far as I can see this is an O(n) implementation and the same would be true for an IEnumerable-implementation.

Peter Morris
02/07/2009 10:53 PM by
Peter Morris

I agree with TED. It looks like you are trying to make Array have behaviour that is relevant only to a specific case. What you really should be doing is creating a new class completely.

Not every problem requires extension methods as a solution. It reminds me of that common saying about a man with only a hammer treats every problem like it's a nail.

Chris Wright
02/08/2009 02:27 AM by
Chris Wright

I like this extension method. It's unfortunate that arrays are second-class citizens in Java and C#, since arrays get special syntax that makes them easier to use. Java's by far the worse offender, lacking operator overloads.

I've only seen reasonable arrays in dynamic languages (Python and Boo; I don't know any others well enough) and D (which has a lot of other neat features, and I use at home all the time, but has more than its share of bugs).

Petar Repac
02/08/2009 04:12 AM by
Petar Repac

GetOtherElementsFromElement could work with IEnumerable and could be an iterator IMO. Seems that would be more efficient.

As indicated by others this method is poorly named in regard that it works on any array. What about RoundRobinStartAfter ?

Another point is that semantic could be better. The method should take an index instead of an element. The current implementation doesn't support arrays that contain multiple instances of the same value.

On the other hand the concerns that I mention could be not important in your particular case.

Peter Morris
02/08/2009 08:08 AM by
Peter Morris

Having thought about it you are dequeuing something aren't you? This to me just looks like a special queue implementation, so I think you should create a specialised queue class and the method should be (wait for it......) dequeue :-)

Andrey Shchekin
02/08/2009 08:41 PM by
Andrey Shchekin

Why Union and not Concat?

Nice, but does not look as an extension method, too tied to the specific usage.

Yann Schwartz
02/08/2009 10:20 PM by
Yann Schwartz

This could be also expressed in pure LINQ, without the need to iterate more than once on any item :

list.Concat(list).SkipWhile( x => x != item)

      .Skip(1).

      TakeWhile(x => x!=item);

(provided item is unique in the list)

Yann Schwartz
02/08/2009 10:29 PM by
Yann Schwartz

Well, my one-liner sends an empty list if the item is not in the list. I'm trying to find a way to accomodate this in the expression, to no avail.

A ugly way to do this (without an indexOf :)

var res = array.Concat(array).SkipWhile( x => x != item)

.Skip(1).

TakeWhile(x => x!=item);

return res.Length == 0 ? array : res;

Vadi
02/09/2009 07:53 AM by
Vadi

Dont you think if the index + 1 will fail if the index passed on is the last element ?

Ayende Rahien
02/09/2009 08:21 AM by
Ayende Rahien

Vadi,

Check the test, that scenario is covered.

Ben Scheirman
02/10/2009 03:20 PM by
Ben Scheirman

I don't care for the naming...

much better as GetElementsExcept(2) or perhaps just array.AllExcept(2);

bennyb
02/11/2009 01:44 PM by
bennyb

var nodes = new[] { 1, 2, 3, 3,5,2 };

Assert.Equal(new[] { 3, 1 }, nodes.GetOtherElementsFromElement(2));

will not work.

+1 for Ben

array.AllExcept(2); // is a more better meaningful method name

Ayende Rahien
02/11/2009 03:18 PM by
Ayende Rahien

Benny,

Duplicate elements cannot exists in the array, that is an implicit assumption.

Assuming that we remove the duplicate elements, it should result int:

5,1,2

Comments have been closed on this topic.