Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This code feels like it goes through too many conversions to accomplish my goal:

based on an IEnumerable of ids get those objects from a data store and set their DisplayOrder property to the position of the associated id.

Is there a more elegant solution that I'm missing?

public void ReorderElements(IEnumerable<int> elementSks)
{
    var elementsToReorder = GetSession().QueryOver<DocumentElement>()
        .WhereRestrictionOn(de => de.DocumentElementSk).IsIn(elementSks.ToArray()).List();

    var elementOrder = elementSks.Select((sk, i) => new { Sk = sk, Order = i })
        .ToDictionary(p => p.Sk, p => p.Order);

    elementsToReorder.ToList()
        .ForEach(e => e.DisplayOrder = elementOrder[e.DocumentElementSk]);

    this.Save(elementsToReorder);
}
share|improve this question
    
Are the elementSks in any particular order when the enter the method? –  sgriffinusa Mar 7 '12 at 19:55
    
@sgriffinusa The ids are posted in order from a web page and then sent to this repository method. –  ahsteele Mar 7 '12 at 21:40
    
Sorry, I should have been more specific, what I want to know is if they are sorted. –  sgriffinusa Mar 7 '12 at 21:43
    
@sgriffinusa yes the ids are in the order I want them in. The elements retrieved from the data store are ordered by their primary key. So I have to update them based on the position of the ids in elementSks. –  ahsteele Mar 7 '12 at 21:46
add comment

2 Answers 2

If the list on input is going to be short, or if performance doesn't matter to you, you can simplify the code by using IndexOf() instead of the dictionary:

public void ReorderElements(IEnumerable<int> elementSks)
{
    var elementsToReorder = GetSession().QueryOver<DocumentElement>()
        .WhereRestrictionOn(de => de.DocumentElementSk).IsIn(elementSks.ToArray()).List();

    var elementOrder = elementSks.ToList();

    foreach(var element in elementsToReorder)
         element.DisplayOrder = elementOrder.IndexOf(element.DocumentElementSk);

    this.Save(elementsToReorder);
}
share|improve this answer
add comment

Please try this. I wrote this without testing or even compiling.

public void ReorderElements(IEnumerable<int> elementSks)
{
    var skToElementsToReorder = GetSession().QueryOver<DocumentElement>()
        .WhereRestrictionOn(de => de.DocumentElementSk).IsIn(elementSks.ToArray())
        .ToDictionary(el => el.DocumentElementSk, el => el);

    int index = 0;
    foreach(int sk in elementSks)
    {
        // Assumes that the key exists - the .IsIn restriction right before helps
        skToElementsToReorder[sk].DisplayOrder = index++;
    }

    this.Save(elementsToReorder);
}

Somehow I think that this can help: http://code.msdn.microsoft.com/101-LINQ-Samples-3fb9811b (as long as you can zip elementSks together with an index. Let me try a simpler version:

public class /* or struct ? */Element 
{
    public string name;
    public in sk;
    public int index;
}

// I am not fluent in LINQ, but Linq.Zip might help to generate these?
public class SkTuple
{
    int sk;
    int index;
}

// Now you have an `IEnumerable` of each somehow, and you want to do an update at the same time as you do a join ... like this? 

http://stackoverflow.com/questions/709560/linq-in-line-property-update-during-join

This kind of got me thinking that maybe SQL is king after all - perhaps you want to just save this list of ids into a temp table where table id will be the index, and then call a stored procedure which will use a join and an update like this one:

http://geekswithblogs.net/faizanahmad/archive/2009/01/05/join-in-sql-update--statement.aspx

share|improve this answer
    
The linq to retrieve elementsToReorder is NHibernate's new QueryOver syntax. It's just a rehashing of their ICriteria expression syntax. It boils down to SQL and doesn't have that much affect on performance. Everything after the object retrieval is what I'm most worried about. –  ahsteele Mar 8 '12 at 3:43
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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