Code efficiency for nested loops c#

Question

Here is my function in which I am using nested loops like:

public string HGSearchNew(HolidayFeedService.PackageHolidays.SearchCriteria objsearchcriteria, dynamic search)
{
        XmlDocument xdoc = new XmlDocument();
        XmlNodeList ndepartures = xdoc.SelectNodes("Destinations/Departure");
        string sFinalDeparture = objsearchcriteria.DepartureAirport.ToUpper();

        for (int i = 0; i < objsearchcriteria.DepartureAirport.Split('|').Length; i++)
        {

            for (int j = 0; j < ndepartures.Count; j++)
            {
                if (objsearchcriteria.DepartureAirport.Split('|')[i].ToUpper() == ndepartures[j]["Name"].InnerText.ToUpper())
                {
                    if (!sFinalDeparture.Contains(objsearchcriteria.DepartureAirport.Split('|')[i].ToUpper()))
                        sFinalDeparture += objsearchcriteria.DepartureAirport.Split('|')[i].ToUpper() + "|";

                    break;
                }
            }
        }
        return sFinalDeparture;
    }

I want to make this code efficient like instead of using loops,use of Contains or Any functions for comparing. kindly help me out?

You should save objsearchcriteria.DepartureAirport.Split('|') inside variable and use that variable instead. otherwise its terrible idea to do split on same string over and over. — M.kazem Akhgary
– M.kazem Akhgary, Commented May 18, 2016 at 13:44
Both. With the ridiculous waste you have there (same with sFinalDeparture as string, instead of creating the string at the end) you just waste memory and speed left and right. — TomTom
– TomTom, Commented May 18, 2016 at 13:46
Few things: in addition to what Akhgary said (which affect - heavily - efficiency) also don't repeat ToUpper() unless necessary and be aware that it's not right way to perform case insensitive string comparison. Recreate a string multiple times is slow, use StringBuilder. Also note that Contains() and Any() with XmlDocument won't affect performance (just slightly degrade them) but for sure not improve. If you want to efficiently use LINQ you should use XDocument (or, with XmlDocument) use XPath whenever possible. Well this is more a question for Code Review than Stack Overflow... — Adriano Repetti
– Adriano Repetti, Commented May 18, 2016 at 13:47
Where does this false premise that Linq magically improves performance over nested loops come from? — user6144226
– user6144226, Commented May 18, 2016 at 13:51
@user6144226: if i've understood OP's code you can see LINQ's power here. — Tim Schmelter
– Tim Schmelter, Commented May 18, 2016 at 14:09

Tim Schmelter · Accepted Answer · 2016-05-18 14:06:15Z

5

I think you can replace the whole method with this readable and efficient LINQ approach:

public string HGSearchNew(HolidayFeedService.PackageHolidays.SearchCriteria objsearchcriteria)
{
    XmlDocument xdoc = new XmlDocument();
    XmlNodeList ndepartures = xdoc.SelectNodes("Destinations/Departure");
    string[] departureTokens = objsearchcriteria.DepartureAirport.Split('|');

    var matches = ndepartures.Cast<XmlNode>()
        .Select(node => node.Name)
        .Intersect(departureTokens, StringComparer.InvariantCultureIgnoreCase);

    return string.Join("|", matches);
}

answered May 18, 2016 at 14:06

Tim Schmelter

462k79 gold badges719 silver badges980 bronze badges

Sign up to request clarification or add additional context in comments.

7 Comments

TomTom Over a year ago

WHooow. Nice. And yes, that is an attempt at intersect... VERY nice.

TomTom Over a year ago

Actually it may be faster to code everything in upper string - case sensitive comparisons are IIRC faster than case insensitive. Not sure it actually matters, though (as in: unless there are a LOT of checks).

Tim Schmelter Over a year ago

Apart from the fact that you could fail the Turkey Test it's also more efficient to use StringComparison overloads of string-methods (or in this case the Intersect overload that takes a comparer) because you don't give the garbage collector loads of temporary strings(ToUpper) as food.

user6144226 Over a year ago

I do think this is a bit differnt from OP's code since it removes non-matches. But I bet this is what OP wanted anyway.

Tim Schmelter Over a year ago

How have you measured it? With StopWatch or DateTime? If the string is large this approach will definitely be faster. Even if the timespan you've measured is correct, how often have you called this method(warmup time)? However, look at your method compared to this and tell me what is easier to read and maintain ;)

|

Collectives™ on Stack Overflow

Code efficiency for nested loops c#

1 Answer 1

7 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

7 Comments

Your Answer

Sign up or log in

Post as a guest

Related