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 is my ugly code which needs to sort my data. I'm not really familiar with C# since actually I'm a Java programmer.

How could this code be rewritten without loosing performance? I know I could use reflection, but it is bad for performance.

sortings is a list of the fields that have to be sorted and if it has to be ASC and DESC. leistungen is the data to be sorted.

The classes:

public class Aufzeichnung
{
    public string Mitarbeiter { get; set; }
    public int Dauer { get; set; }
    public double Kosten { get; set; }
}

public class Leistung
{
    public int ID { get; set; }
    public string Art { get; set; }
    public string Angebot { get; set; }
    public string Jahr { get; set; }
    public string Berater { get; set; }
    public string Assistent { get; set; }
    public double Preis { get; set; }
    public List<Aufzeichnung> Aufzeichnungen { get; set; }
}

public class LaufendeLeistung
{
    public string Kunde { get; set; }
    public List<Leistung> Leistungen { get; set; }
}

The algorithm:

internal static IEnumerable<A> Sort(List<A> leistungen, IEnumerable<Sorting> sortings)
    {
        for (int i = 0; i < sortings.Count(); i++)
        {
            var sort = sortings.ElementAt(i).Ascending ? 1 : -1;

            if (sortings.ElementAt(i).Field.Equals("Kunde"))
            {
                leistungen.Sort((a,b) => string.Compare(a.Kunde,b.Kunde)*sort);
            } 
            else 
            {
                for (var j = 0; j < leistungen.Count(); j++)
                {
                    if (sortings.ElementAt(i).Field.Equals("Leistung"))
                    {
                        leistungen[j].Leistungen.Sort((a, b) => string.Compare(a.Art, b.Art) * sort);
                    }
                    else if (sortings.ElementAt(i).Field.Equals("Angebot"))
                    {
                        leistungen[j].Leistungen.Sort((a, b) => string.Compare(a.Angebot, b.Angebot) * sort);
                    }
                    else if (sortings.ElementAt(i).Field.Equals("Jahr"))
                    {
                        leistungen[j].Leistungen.Sort((a, b) => string.Compare(a.Jahr, b.Jahr) * sort);
                    }
                    else if (sortings.ElementAt(i).Field.Equals("Berater"))
                    {
                        leistungen[j].Leistungen.Sort((a, b) => string.Compare(a.Berater, b.Berater) * sort);
                    }
                    else if (sortings.ElementAt(i).Field.Equals("Assistent"))
                    {
                        leistungen[j].Leistungen.Sort((a, b) => string.Compare(a.Assistent, b.Assistent) * sort);
                    }
                    else if (sortings.ElementAt(i).Field.Equals("Mitarbeiter"))
                    {
                        for (var k = 0; k < leistungen[j].Leistungen.Count(); k++)
                        {
                            if (leistungen[j].Leistungen[k].Aufzeichnungen != null)
                            {
                                leistungen[j].Leistungen[k].Aufzeichnungen.Sort((a, b) => string.Compare(a.Mitarbeiter, b.Mitarbeiter) * sort);
                            }
                        }

                    }

                }
            }
        }
        return leistungen;
    }
share|improve this question
    
Can you provide the definition for Sorting? (You can insert it in blockquote with a note to not review it, if you like.) –  EBrown 19 hours ago

4 Answers 4

The idomatic way to enable sorting is to implement the IComparable interface. And this will definitely simplify the sorting code. So:

public class Leistungen : IComparable<Leistungen> {  }
public class Aufzeichnung : IComparable<Aufzeichnung> { }

then you can sort like this:

List<Leistungen> myLeistungen;  // pretend we instantiated it too.

myLeistungen.Sort();

And if you want different sort algorithms then create an IComparer class for each one as suggested by @ratchetfreak. then you can do the following - which will override your IComparalble implementation in the class Leistungen:

myLeistungen.Sort(myDifferentComparer);

IComparable Implementation

This demonstrates how to compare multiple properties. Both of your classes will implement using this pattern. When Leistungen.CompareTo() gets down to comparing its List<Aufzeichnung>, well Aufzeichnung.CompareTo() takes care of that!

public class Leistungen : IComparable<Leistungen> {
    \\ implementing the generic version means we don't 
    \\ check for or cast to the correct type.
    public int CompareTo(Leistungen other) {
        int result = 1; // "this" is > "other"

        if(other == null) return result;

        result = this.Art.CompareTo(other.Art);

        if(result == 0)
            result = CompareAngebot(other);

        return result;
    }

    protected int CompareAngebot(Leistungen other) {
        int result = 1;
        result = this.Angebot.CompareTo(other.Angebot);

        if(result == 0)
           result = CompareJahr(other);

        return result;
    }

    protected int CompareJahr(Leistungen other) { // you get the idea }

    // ....

    protected int CompareAufzeichnungList(Leistungen other) {
        // last property in our compare chain, so it's real simple

        return this.Aufzeichnungen.CompareTo(other.Aufzeichnungen);
    }

}
share|improve this answer
    
If I understand the code in the question correctly, then it actually provides a way to order by one property first and another property afterwards (with a choice of asc/desc for each property). –  hangy 56 mins ago

you could define a IComparer that holds several IComparers and returns the first result that isn't 0:

class CascadedComparer<T> : IComparer<T>{

    IList<IComparer<T>> comparings; //fill in constructor

    public int Compare(T x, T y){
        foreach(var comp in comparings){
            int r = comp.compare(x, y);

            if(r!=0) 
                return r;
        }
        return 0; //all returned 0
    }

}

Then you can pass an instance of that filled with the field specific comparers to the sort method of leistungen[j].Leistungen.

The Sorting doesn't need to hold the string value of the field to compare but instead a Comparer (or a mapping function).

Ortherwise you can build a map<string, IComparer<A>> and then you can get the comparer directly from the map.

share|improve this answer
    
To be honest, i didn't exactly understand how to implement this in my code... –  Emaborsa 20 hours ago

I think that there is some potential for optimization even without rewriting the code completely.

  1. By using sortings.ElementAt(i) over and over again, you are actually getting the enumerable element every time. As of now, I see no reason not to use foreach (var sorting in sortings) and using the sorting variable inside the loop, instead of the for loop.
  2. Depending on how many and which sort fields get passed to the Sort method via the sortings parameter, you may be re-sorting the same list(s) over and over again. Since List.Sort actually re-sorts the array every time, this not negligible.
  3. There is exactly one case where you need to sort the leistungen list itself and one where the Aufzeichnungen list needs to be sorted. You can identify them before iterating through everything (just use the last entry of the sortings list that matches the field name to get the correct sort order) and apply them seperately from the loop.

From an outside perspective, it could also be unexpected that the leistungen list is sorted in place and then returned as an IEnumerable<A>. I would at leas add some documentation that states that the input list will be modified.

share|improve this answer
    
1. You're right, I can export the variable in order taht the code is more readable. 2+3.I know, but i asked a way to simplify the code, not just to improve performance. –  Emaborsa 20 hours ago

One of the improvements you can make is a foreach on sortings instead of using .ElementAt(i) (which will always be slower).

Another improvement would be to convert all those if statements in the else block to a switch.

Third, you should be using nameof(...) instead of string constants. This way, if something gets refactored, the name change persists without needing to go through all the strings and look for names to change. (For example, nameof(Leistung.Angebot) will return Angebot.) Do note: this is C#6.0 only.

Another suggestion is to create a Comparison<Leistung> variable to hold your delegate for comparison in. That way, you can create it once and run through each sort as needed. This does require you to create an extra else if and duplicate your for (var j = 0; j ... loop, but it makes it a bit more maintainable (and likely faster) in the end.

Fifth, I would recommend replacing for (var j = 0; ... and for (var k = 0; ... with foreach loops. They should be faster (if I recall correctly), and you don't have a need to keep track of j or k in them.

Another C#6.0 feature you can take advantage of is the ?. null-condition check to remove the null-check in Mitarbeiter sort. Aufzeichnungen?.Sort will only call .Sort if Aufzeichnungen is not null.

Lastly, there's no need for string.Equals. In C# you can directly compare strings with ==.


All in all, here is the new version (with all the edits I recommend).

Yes, it's longer, but it's also more robust, duplicates less, and should likely perform better.

internal static IEnumerable<A> Sort(List<A> leistungen, IEnumerable<Sorting> sortings)
{
    foreach (var sorting in sortings)
    {
        var sort = sorting.Ascending ? 1 : -1;

        if (sorting.Field == nameof(LaufendeLeistung.Kunde))
        {
            leistungen.Sort((a, b) => string.Compare(a.Kunde, b.Kunde) * sort);
        }
        else if (sorting.Field == nameof(Aufzeichnung.Mitarbeiter))
        {
            Comparison<Aufzeichnung> comparison = (a, b) => string.Compare(a.Mitarbeiter, b.Mitarbeiter) * sort;

            foreach (var element in leistungen)
            {
                foreach (var subElement in element.Leistungen)
                {
                    subElement.Aufzeichnungen?.Sort(comparison);
                }
            }
        }
        else
        {
            Comparison<Leistung> comparison;

            switch (sorting.Field)
            {
                case nameof(Leistung):
                    comparison = (a, b) => string.Compare(a.Art, b.Art) * sort;
                    break;
                case nameof(Leistung.Angebot):
                    comparison = (a, b) => string.Compare(a.Angebot, b.Angebot) * sort;
                    break;
                case nameof(Leistung.Jahr):
                    comparison = (a, b) => string.Compare(a.Jahr, b.Jahr) * sort;
                    break;
                case nameof(Leistung.Berater):
                    comparison = (a, b) => string.Compare(a.Berater, b.Berater) * sort;
                    break;
                case nameof(Leistung.Assistent):
                    comparison = (a, b) => string.Compare(a.Assistent, b.Assistent) * sort;
                    break;
                default: // This will perform the same sort as sorting by `Leistung`. The nice thing about this is if the value in `sorting.Field` is an invalid field, it will still sort by *something*.
                    comparison = (a, b) => string.Compare(a.Art, b.Art) * sort;
                    break;
            }

            foreach (var element in leistungen)
            {
                element.Leistungen.Sort(comparison);
            }
        }
    }

    return leistungen;
}
share|improve this answer

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.