0
\$\begingroup\$

I'm having multiple FirstOrDefault() statements, in this case 2, on a collection.

         var id = data.Col.FirstOrDefault(x => x.Type == Types.Id)?.Value;
         var email = data.Col.FirstOrDefault(x => x.Type == Types.Email)?.Value;

In the worst scenario this will loop 2 times over my entire collection.

I'm searching for a way to only loop over my collection once (and if possible break early when all has been found).

Is this a proper way of doing it or could it be written better?:


            string id = null;
            string email = null;
            bool foundIdType = false;
            bool foundEmailType = false;

            foreach (var item in data.Col)
            {
                if (!foundIdType && item.Type == Types.Id)
                {
                    foundIdType = true;
                     id = item.Value;

                    if (foundIdType && foundEmailType)
                        break;
                }

                if (!foundEmailType && item.Type == Types.Email)
                {
                    foundEmailType = true;
                    email = item.Value;

                    if (foundIdType  && foundEmailType)
                        break;
                }
            }
\$\endgroup\$
3
  • \$\begingroup\$ Are you certain you want to do FirstOrDefault? IMHO it is rare that I use that, most of the time SingleOrDefault is the correct logic, since I expect to find a maximum of one. Getting multiple results and then picking the first one (for no apparent reason) seems iffy to me. \$\endgroup\$
    – BCdotWEB
    Jul 5 at 5:39
  • 1
    \$\begingroup\$ Is there a significant consideration for performance gain? Because code should not only be judged by its squeezed performance, but also by its readability and maintainability, both of which get diminished by optimizing this logic. The validity of your fix hinges on what you concretely stand to gain from this; you can't just blindly assume that further optimization is always the better choice. \$\endgroup\$
    – Flater
    Jul 5 at 9:54
  • \$\begingroup\$ It is a rather large collection (50k items) and each request (get) and iteration costs me time and cpu, thus money. \$\endgroup\$
    – juFo
    Jul 5 at 10:02

1 Answer 1

2
\$\begingroup\$

Your method is quite correct.
However, it can be simplified by getting rid of Boolean variables.

string id = null;
string email = null;

foreach (var item in data.Col)
{
    if (id == null && item.Type == Types.Id)
        id = item.Value;

    if (email == null && item.Type == Types.Email)
        email = item.Value;

    if (id != null && email != null)
        break;
}

What type does the Col collection have?
If performance is critically important to you, then you can look at ways to increase it, such as Loop unrolling and the Sentinel value.

You can also cache values. When inserting data into a collection, save their very first values to the id and email variables, and the search is no longer needed.

\$\endgroup\$
1
  • \$\begingroup\$ thanks for the sentinel value reminder! \$\endgroup\$
    – juFo
    Jul 5 at 10:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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