1
\$\begingroup\$

I'm making a comparing section, which I've just started typing, trying to get something working. It's working now, but as you can see its pretty messy. I feel this could be done in one or two less queries, or I should break it apart into smaller methods.

foreach (XMLObject source in Sources)
{
    var matchedElement =
        destList.FirstOrDefault(s.ElementName == source.ElementName);
        // checks to see if it exists in one list but not the other

    if (matchedElement == null) // element is in one list and not the other
    {
        var alter =
            lstAlteration.FirstOrDefault(
                a => a.ElementName == source.ElementName && a.ChangeInfo.ChangeFrom == source.ElementName);
            // check to see if the the element is a Removal (Changed From would not be blank)

        if (alter == null) // Removal has not been triggered
        {
            alter = new Alteration
                        {
                            ElementName = source.ElementName,
                            ChangeInfo = new ChangeDetail( {ChangeTo = source.ElementName}
                        };
        }
        else
        {
            alter.ChangeInfo.ChangeTo = source.ElementName;
        }

        lstAlteration.Add(alter);
    }
}
\$\endgroup\$
3
\$\begingroup\$
foreach (var source in Sources.Where(x => !destList.Exists(s => s.ElementName == x.ElementName))
{
    var alter = lstAlteration.FirstOrDefault(a => a.ElementName == source.ElementName
                                 && a.ChangeInfo.ChangeFrom == source.ElementName);

    if (alter == null) {
        alter = new Alteration
                    {
                        ElementName = source.ElementName,
                        ChangeInfo = new ChangeDetail()
                    };

        lstAlteration.Add(alter);
    }

    alter.ChangeInfo.ChangeTo = source.ElementName;
}
  • Sorter
  • Only one .ChangeInfo.ChangeTo = source.ElementName; execution
\$\endgroup\$
  • 1
    \$\begingroup\$ Shouldn't where clause be "not exists in destList"? \$\endgroup\$ – abuzittin gillifirca May 27 '13 at 7:36
  • \$\begingroup\$ I like that the "not exists" is filtered at the get-go, fundamentally simplifying the proceeding logic; allowing the loop to be focused on what its supposed to be doing. \$\endgroup\$ – radarbob Jun 4 '13 at 15:09
3
\$\begingroup\$

Just a small note as I think Peters answer is pretty clean and tidy.

You may want to consider a consistant naming convention. For the same list type you use a prefix of lst (lstAlterations) and a suffix of List (destList). I'm not a huge fan of this convention (others might beg to differ) so instead I would probably look at naming the lists something like just alterations and destinations.

\$\endgroup\$
  • \$\begingroup\$ you are absolutely right! \$\endgroup\$ – Spooks May 27 '13 at 11:07
2
\$\begingroup\$

I'd like to suggest the following:

        foreach (XMLObject source in Sources)
        {
            if (destList.Exists(s.ElementName == source.ElementName))
                continue;
            // perform a lightweight check
            if (lstAlteration.Exists(a => a.ElementName == source.ElementName 
                                        && a.ChangeInfo.ChangeFrom == source.ElementName)) 
            {
                lstAlteration.Where(a => a.ElementName == source.ElementName
                                         && a.ChangeInfo.ChangeFrom == source.ElementName).First()
                                         .ChangeInfo.ChangeTo = source.ElementName;
            }
            else
            {
                lstAlteration.Add(new Alteration
                            {
                                ElementName = source.ElementName,
                                ChangeInfo = new ChangeDetail( {ChangeTo = source.ElementName}
                            });
            }
        }
  1. Reduce nesting
  2. Perform lightweight checks instead of instance materializing.
  3. Compact code, ommiting extra variable
\$\endgroup\$

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.