Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 18 | Comments: 72

filter by tags archive

Elegant code

time to read 7 min | 1359 words

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

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

Notice the ordering of the elements there.

Justin Lee

Ah. Indeed. Now I see. Elegant indeed.

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

Check the test name.

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

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
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

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

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

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

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

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

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

Why Union and not Concat?

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

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

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

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

Ayende Rahien

Vadi,

Check the test, that scenario is covered.

Ben Scheirman

I don't care for the naming...

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

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

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

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. RavenDB 3.0 New Stable Release - 13 hours from now
  2. Production postmortem: The industry at large - about one day from now
  3. The insidious cost of allocations - 3 days from now
  4. Buffer allocation strategies: A possible solution - 6 days from now
  5. Buffer allocation strategies: Explaining the solution - 7 days from now

And 3 more posts are pending...

There are posts all the way to Sep 11, 2015

RECENT SERIES

  1. Find the bug (5):
    20 Apr 2011 - Why do I get a Null Reference Exception?
  2. Production postmortem (10):
    01 Sep 2015 - The case of the lying configuration file
  3. What is new in RavenDB 3.5 (7):
    12 Aug 2015 - Monitoring support
  4. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
View all series

RECENT COMMENTS

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats