7
\$\begingroup\$

My requirement is to write a program that sort of mimics diff tools. Yes there are quite a few libraries and open source code that accomplishes this purpose, but I would like to write my own comparer.

Here's the starting point. I have a class called DataItem which looks like this:

public class DataItem
{
    public DataItem() { }
    public DataItem(string d, string v) { Data = d; Value = v; }

    public string Data { get; set; }
    public string Value { get; set; }
}

I have two lists of these class objects, let's call them PRE and POST and take some example values as follows. 'Data' part will be unique in a list.

preList: (Data,Value)
AAA,0
BBB,1
CCC,3
DDD,4
FFF,0
GGG,3

postList: (Data,Value)
AAA,0
BBB,2
DDD,4
EEE,9
FFF,3

Think of PRE as the original list, and POST as the list after some changes done. I would like to compare the two, and categorize them into three categories:

  1. Added Items - An item with a new 'Data' added to the list.
  2. Removed Items - An item was removed from the list.
  3. Diff Items - 'Data' is found in both PRE and POST lists, but their corresponding 'Value' is different.

So when categorized they should look like this:

Added Items:
EEE,9

Removed Items:
CCC,3
GGG,3

Diff Items:
BBB
FFF

I have another DiffItem class, to objects of which I would like to put the final results. DiffItem looks like this:

public class DiffItem
{
    public DiffItem() { }
    public DiffItem(string data, string type, string pre, string post) { Data = data; DiffType = type; PreVal = pre; PostVal = post; }

    public string Data { get; set; }
    public string DiffType { get; set; } // DiffType = Add/Remove/Diff
    public string PreVal { get; set; } // preList value corresponding to Data item
    public string PostVal { get; set; } // postList value corresponding to Data item
}

To accomplish this, first I extended IEqualityComparer and wrote a couple of comparers:

public class DataItemComparer : IEqualityComparer<DataItem>
{
    public bool Equals(DataItem x, DataItem y)
    {
        return (string.Equals(x.Data, y.Data) && string.Equals(x.Value, y.Value));
    }

    public int GetHashCode(DataItem obj)
    {
        return obj.Data.GetHashCode();
    }
}

public class DataItemDataComparer : IEqualityComparer<DataItem>
{
    public bool Equals(DataItem x, DataItem y)
    {
        return string.Equals(x.Data, y.Data);
    }

    public int GetHashCode(DataItem obj)
    {
        return obj.Data.GetHashCode();
    }
}

Then use the Except() and Intersect() methods as follows:

    static void DoDiff()
    {
        diffList = new List<DiffItem>();

        IEnumerable<DataItem> preOnly = preList.Except(postList, new DataItemComparer());
        IEnumerable<DataItem> postOnly = postList.Except(preList, new DataItemComparer());
        IEnumerable<DataItem> common = postList.Intersect(preList, new DataItemComparer());

        IEnumerable<DataItem> added = postOnly.Except(preOnly, new DataItemDataComparer());
        IEnumerable<DataItem> removed = preOnly.Except(postOnly, new DataItemDataComparer());
        IEnumerable<DataItem> diffPre = preOnly.Intersect(postOnly, new DataItemDataComparer());
        IEnumerable<DataItem> diffPost = postOnly.Intersect(preOnly, new DataItemDataComparer());

        foreach (DataItem add in added)
        {
            diffList.Add(new DiffItem(add.Data, "Add", null, add.Value));
        }
        foreach (DataItem rem in removed)
        {
            diffList.Add(new DiffItem(rem.Data, "Remove", rem.Value, null));
        }
        foreach (DataItem pre in diffPre)
        {
            DataItem post = diffPost.First(x => x.Data == pre.Data);
            diffList.Add(new DiffItem(pre.Data, "Diff", pre.Value, post.Value));
        }
    }

This does work and gets the job done. But I'm wondering if there's a 'better' way to do this. Note that I put quotes around the word 'better', because I don't have a proper definition for what would make this better. Perhaps there is a way to get this done without as many foreach loops and use of Except() and Intersect(), since I would imagine that behind the Linq there's quite a lot of iteration going on.

Simply put, is there cleaner code that I can write for this? I'm asking mostly out of academic interest and to expand my knowledge.

\$\endgroup\$
  • 1
    \$\begingroup\$ In your Equals and GetHashCode methods, you should account for one or both of the objects being null, for example: return string.Equals(x?.Data, y?.Data); \$\endgroup\$ – Rufus L Jul 18 '17 at 0:33
  • \$\begingroup\$ DiffType should be of enum type rather than string. enum DiffType { Add, Remove, Diff } \$\endgroup\$ – Maxim Jul 18 '17 at 6:25
  • \$\begingroup\$ You could just sort the two on key and make a pass but that would be a whole new approach and not really a code review. \$\endgroup\$ – paparazzo Jul 18 '17 at 9:29
  • \$\begingroup\$ If DataItem class implemented IComparable<DataItem> and IEquatable<DataItem>, you wouldn't need the DataItemComparer class and could omit all the noisy new DataItemComparer() parameters. \$\endgroup\$ – Rick Davin Jul 18 '17 at 11:00
  • \$\begingroup\$ A couple of questions: Does order matter? Are you saying that Data, e.g. "AAA". is unique in each list? \$\endgroup\$ – RobH Jul 18 '17 at 13:09
3
\$\begingroup\$

In my opinion many lines with IEnumerable<DataItem> on the left side make this code noisy. This

var preOnly = preList.Except(postList, new DataItemComparer());
var postOnly = postList.Except(preList, new DataItemComparer());
var common = postList.Intersect(preList, new DataItemComparer());

is clearer as for me.

As I said in comments you should use enum for DiffType instead of string.

public enum DiffType
{
    Add,
    Remove,
    Diff
}

public class DiffItem
{
    public DiffItem()
    {
    }

    public DiffItem(string data, DiffType type, string pre, string post)
    {
        Data = data;
        DiffType = type;
        PreVal = pre;
        PostVal = post;
    }

    public DiffType DiffType { get; set; }
}

Using AddRange method of the List this code

foreach (DataItem add in added)
{
    diffList.Add(new DiffItem(add.Data, "Add", null, add.Value));
}
foreach (DataItem rem in removed)
{
    diffList.Add(new DiffItem(rem.Data, "Remove", rem.Value, null));
}
foreach (DataItem pre in diffPre)
{
    DataItem post = diffPost.First(x => x.Data == pre.Data);
    diffList.Add(new DiffItem(pre.Data, "Diff", pre.Value, post.Value));
}

can be rewritten as

diffList.AddRange(added.Select(i => new DiffItem(i.Data,
                                                 DiffType.Add,
                                                 null,
                                                 i.Value)));
diffList.AddRange(removed.Select(i => new DiffItem(i.Data,
                                                   DiffType.Remove,
                                                   i.Value,
                                                   null)));
diffList.AddRange(diffPre.Select(i => new DiffItem(i.Data,
                                                   DiffType.Diff,
                                                   i.Value,
                                                   diffPost.FirstOrDefault(x => x.Data == i.Data)?.Value)));

And I believe an instance of the DiffItem should be immutable so setters of the properties can be removed.

Also instead of using DiffType you can create three subclasses representing each type of difference:

public class DiffItem
{
    public DiffItem()
    {
    }

    public DiffItem(string data)
    {
        Data = data;
    }

    public string Data { get; }
} 

public sealed class Addition : DiffItem { ... }
public sealed class Deletion : DiffItem { ... }
public sealed class Difference : DiffItem { ... }

It will allow to eliminate some constructor parameters that are useless for particular difference type.

\$\endgroup\$
  • \$\begingroup\$ When you say using var over IEnumerable<DataItem>, is there a specific advantage to doing that, or is it solely for improving readability? It's just one of those habits that I never got used to using var, and prefer using strongly typed variables, but I do agree using var is cleaner. Using AddRange() definitely improves readability. And I like what you did with DiffItem, never thought of going that far but it's definitely very clean and efficient. \$\endgroup\$ – Sach Jul 18 '17 at 17:20
  • 1
    \$\begingroup\$ @Sach In this case var stands only for readability. There are cases where var must be used (for anonymous types). Four years ago I had your habit too and wasn't using var, and now I don't understand why :) \$\endgroup\$ – Maxim Jul 19 '17 at 2:46
1
\$\begingroup\$

I don't know if this is cleaner but it is another approach.

Perhaps is there a way to get this done without as many 'foreach' loops and use of Except() and Intersetc()

It is probably more efficient and I like it better but I like seeing logic loops.
I left debug statements as not fully tested.

public static void ListDiff2()
{
    List<string> listA = new List<string>() { "AAA", "BBB", "DDD" };
    List<string> listB = new List<string>() { "AAA", "CCC" };
    //List<string> listA = new List<string>() { "AAA", "BBB", "CCC", "DDD", "FFF", "GGG" };
    //List<string> listB = new List<string>() { "AAA", "BBB", "DDD", "FFF", "EEE" };

    IEnumerable<string> listAsorted = listA.OrderBy(x => x);
    IEnumerable<string> listBsorted = listB.OrderBy(x => x);

    var listAenumerator = listAsorted.GetEnumerator();
    var listBenumerator = listBsorted.GetEnumerator();

    bool listAcanmove = listAenumerator.MoveNext();
    bool listBcanmove = listBenumerator.MoveNext();

    while (listAcanmove | listBcanmove)
    {
        string valueA = listAenumerator.Current;
        string valueB = listBenumerator.Current;

        Debug.WriteLine($"valueA = {valueA}");
        Debug.WriteLine($"valueB = {valueB}");

        if (!listAcanmove && listBcanmove)
        {
            Debug.WriteLine($"{valueB} in B not in A");
            listBcanmove = listBenumerator.MoveNext();
        }
        if (listAcanmove && !listBcanmove)
        {
            Debug.WriteLine($"{valueA} in A not in B");
            listAcanmove = listAenumerator.MoveNext();
        }
        else
        {
            int comp = string.Compare(valueA, valueB);
            if (comp == -1)
            {
                Debug.WriteLine($"{valueA} in A not in B");
                listAcanmove = listAenumerator.MoveNext();
            }
            else if (comp == 1)
            {
                Debug.WriteLine($"{valueB} in B not in A");
                listBcanmove = listBenumerator.MoveNext();
            }
            else
            {
                Debug.WriteLine($"{valueA} {valueB} in B and A");

                //perform value comparison here

                listAcanmove = listAenumerator.MoveNext();
                listBcanmove = listBenumerator.MoveNext();
            }
        }
        Debug.WriteLine("");
    }
    Debug.WriteLine($"done");
}
\$\endgroup\$
  • \$\begingroup\$ thanks. This looks like a more generic approach though, and does not quite solve my situation? Instead of two List<string> lists I want to do the comparison on class objects, and I want both Data and Value parts to be available for future use once comparisons are done. \$\endgroup\$ – Sach Jul 18 '17 at 17:10
  • \$\begingroup\$ @Sach Look again, it solves your situation. Just substitute DataItem for string and do a little work. See "//perform value comparison here". It does exactly what you asked for "get this done without as many 'foreach' loops and use of Except() and Intersetc()". \$\endgroup\$ – paparazzo Jul 18 '17 at 17:24
  • \$\begingroup\$ OK I got it now. Yes it's a different approach, thanks. I will implement this too and see. \$\endgroup\$ – Sach Jul 18 '17 at 17:49

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.