Refactor foreach loops

Question

I have 2 objects and I am comparing them to see which fields are equal and which are different.

But I am not sure my loop is 100% correct. Does this look ok? If so, what other ways could this be coded to make it 'neater'. e.g. using LINQ

I want to: loop through each DocumentType in both objects, and for each documentType - I want to loop through each DocumentTypeVersion and compare this.

foreach (DocumentType documentType in documentTypes)
{
    foreach (DocumentType importDocumentType in importedDocumentTypes)
    {

        if (documentType.Id == importDocumentType.Id)//ensure its the same documentType we are comparing
        {
            foreach (DocumentTypeVersion version in documentType.Versions)
            {
                foreach (DocumentTypeVersion importedVersion in importDocumentType.Versions)
                {
                    if (version.Id == importedVersion.Id)//ensure its the same documentTypeVersion we are comparing
                    {
                        //check if the versions are equal
                        bool areVersionsEqual = Equals(version, importedVersion);

                    }

                }
            }
        }
    }
}

Answer 1

An option would be to create two IEqualityComparers; one for DocumentType and one for DocumentTypeVersion. You could then easily call Intersect() and get the correct ones.

public class DocumentTypeIdComparer : IEqualityComparer<DocumentType>
{
    public bool Equals(DocumentType docType1, DocumentType docType2)
    {
        return !(docType1 == null || docType2 == null) && docType1.Id == docType2.Id;           
    }
}

public class DocumentTypeVersionComparer : IEqualityComparer<DocumentTypeVersion>
{
    public bool Equals(DocumentTypeVersion docTypeVersion1, DocumentTypeVersion docTypeVersion2)
    {
        return !(docTypeVersion1 == null || docTypeVersion2 == null) && Object.Equals(docTypeVersion1, docTypeVersion2);           
    }
}

You will then be able to reduce your code to this;

var equalDocumentsById = documentTypes.Intersect(importDocumentTypes, new DocumentTypeIdComparer());
var equalVersions = documentTypes.Versions.Intersect(importDocumentTypes.Versions, new DocumentTypeVersionComparer());
var equalDocuments = equalDocumentsById.Where(doc => doc.Versions.Intersect(equalVersions, new DocumentTypeVersionComparer()).Any());

And then process equalDocuments as you please. If you want the documents which are not equal, simply switch .Count > 0 with .Count == 0.

Answer 2

My first approach:

        foreach (var documentType in documentTypes)
        {
            var importedDoc = importedDocumentTypes.FirstOrDefault(doc => doc.Id == documentType.Id);
            if(importedDoc == null) 
                continue; // or error handling for not found


            foreach (var version in documentType.Versions)
            {
                var importedVersion = importedDoc.Versions.FirstOrDefault(ver => ver.Id == version.Id);
                if(importedVersion == null)
                    continue; // or error handling

                bool areVersionsEqual = Equals(version, importedVersion);

            }
        }

The idea is to remove the granularity of nesting in the code, which makes it more readable and understandable. Read more on Programmers.

Don't forget to write tests for stuff like that, and keep in mind that you only do a one-way check (you check if B is contained in A but NOT A is contained in B). This is equal to your code, and I don't know if you want that.

Answer 3

I know this question has been answered, but I feel like this is a perfect case for the SequenceEqual method. Rather than having to handle the 2 equalitycomparers in the same layer you can change your code into one line--a SequenceEqual() call that kicks off another SequenceEqual inside the comparer to test the versions. So in the outer equalitycomparer you could do the comparison of the Id and then call another SequenceEqual on the Versions collections of both.

Modifying Dion's example above to use SequenceEqual, the revamped comparers: UPDATE: Object.Equals is not going to work for you. You need to compare the properties if you want objects that aren't the exact same but have the exact same values for properties.

public class DocumentTypeComparer : IEqualityComparer<DocumentType>
{
    public bool Equals(DocumentType docType1, DocumentType docType2)
    {
        return !(docType1 == null || docType2 == null) 
               && docType1.Id == docType2.Id 
               && docType1.Versions.SequenceEqual(doctype2.Versions, new DocumentTypeVersionComparer());           
    }
}

public class DocumentTypeVersionComparer : IEqualityComparer<DocumentTypeVersion>
{
    public bool Equals(DocumentTypeVersion docTypeVersion1, DocumentTypeVersion docTypeVersion2)
    {
        if (docTypeVersion1.ReferenceEquals(docTypeVersion2)) return true;
        return !(Object.ReferenceEquals(docTypeVersion1, null) || Object.ReferenceEquals(docTypeVersion2, null)) 
               && docTypeVersion1.SomeProperty == docTypeVersion2.SomeProperty 
               && docTypeVersion1.SomeOtherProperty == docTypeVersion2.SomeOtherProperty);
    }           
}

And your one-liner to check whether the collections differ:

var collectionsMatch = documentTypes.SequenceEqual(importedDocumentTypes, new DocumentTypeComparer());

Example one-liner using possible sorting method:

var collectionsMatch = documentTypes.OrderBy(d => d.Id).SequenceEqual(importedDocumentTypes.OrderBy(d => d.Id), new DocumentTypeComparer());

Note that you will have to sort your collections before doing a comparison. I didn't include what to sort on because that's up to your implementation, but I recommend doing the ordering of Versions inside the DocumentTypeComparer, you don't want the DocumentTypeComparer to only work when the collections it's applied to have a sorted Version property. Also when implementing IEqualityComparer you will have to implement GetHashCode (resources below).

Example implementation of GetHashCode from MSDN: http://msdn.microsoft.com/en-us/library/vstudio/bb342073(v=vs.100).aspx

Discussion on best way to implement the method: https://stackoverflow.com/questions/263400/what-is-the-best-algorithm-for-an-overridden-system-object-gethashcode

Answer 4

Var

Use the var keyword when defining foreach loop variables.

e.g.

foreach (DocumentType documentType in documentTypes)

should be

foreach (var documentType in documentTypes)

LINQ

Note this is untested as I don't have access to your types

It's possible to do it in LINQ, although I'm unsure I'd recommend it:

var equalVersions = documentTypes
    .SelectMany(documentType=> documentType.Versions.Select(version=> new {Id=documentType.Id, Version= version}))
    .Where(idVersionPair=> importedDocumentTypes.Any(
        importedDocument=>importedDocument.Id == idVersionPair.Id && 
        importedDocument.Versions.Any(importedVersion=> importedVersion.Equals(idVersionPair.Version))))
    .Select(IdVersionPair=> IdVersionPair.Version);

As you can see, it's fairly unreadable compared to some of the other answers, but I leave it here as proof it can be done.