2
\$\begingroup\$

I decided to do a bit more in C# by writing two methods that find duplicates, one being a naive approach, and the other one utilizing the LINQ capabilities of C# as a form of learning and training. Below is my code, and I would appreciate any critiques of it in terms of style, design, and if there are more succinct ways of doing it.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Practice
{
    #region FindDuplicate
    class FindDuplicate
    {
        private int[] input;
        private List<int> dupList;
        private IEnumerable<int> dupes;

        public FindDuplicate(int[] input)
        {
            this.input = input;
        }


        /**
         * This is a more advanced version of printing out the duplicates in the sense that we are using
         * SQL-Like statements in the form of Group, where, and select. If these were SQL it may look like:
         * SELECT * FROM INPUT s1
         * JOIN INPUT s2
         * ON s2.value = s1.value 
        */
        public FindDuplicate finderVariantTwo()
        {
            //Note how we do not need to sort the array, the grouping will do it for us.
            this.dupes = this.input.GroupBy(result => result)
                .Where(whereValueClause => whereValueClause.Count() > 1)
                .Select(indexValue => indexValue.Key);

            return this;
        }

        /**
         * This is the "naive" variant, and the one that first came to my mind. The idea here is simple: 
         * First, 
         */
        public FindDuplicate getDuplicates()
        {
            this.dupList = new List<int>();

            //for this, let us first sort the array to make it easier for the loop. 
            Array.Sort(this.input);

            for(int i = 0; i < input.Length; i++)
            {
                //this is to make sure we do not have an index out of bounds.
                int next = (i < input.Length-1) ? i+1 : input.Length - 1;

                if(input[i] == input[next] && !dupList.Contains(input[i]))
                {
                    dupList.Add(input[i]);
                }
            }

            return this;
        }


        public void getDuplicatesTwo()
        {
            String result = "Result => {";
            foreach (int i in this.dupes)
            {
                result += i + ",";
            }
            result += "}";
            Console.WriteLine(result);
        }

        public void printResultVariantOne()
        {
            String result = "Result => {";
            foreach(int i in this.dupList)
            {
                result += i + ",";
            }
            result += "}";
            Console.WriteLine(result);
        }
        #endregion
    }

    class TestFindDuplicates
    {
        static void Main(string[] args)
        {
            int[] input = { 2, 3, 2, 4, 5, 5, 1, 10, 3, 10, 9, 9 };

            FindDuplicate find = new FindDuplicate(input);

            find.getDuplicates().printResultVariantOne();

            Console.WriteLine("\n");

            find.getDuplicatesTwo().printResultVariantTwo();

            //to stop the window from closing as soon as it finishes running
            Console.ReadKey();
        }
    }
}

One thing I do want to know though, and I know this is not the question/answer side of SO, so my apologies. But, is it possible to somehow transform my input into a generic? I tried looking around the web, but none of the answers were sufficient. That is to say, is there a way for me to say input is of type T instead of a hard-coded int value?

\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

Improvable

class FindDuplicate

Class names should be nouns, e.g. DuplicateFinder.


public FindDuplicate finderVariantTwo()
public FindDuplicate getDuplicates()

This is not a user friendly API. You first need to create an instance of the FindDuplicate class and then you can search for duplicates. These methods could just return the result.

If you want to implement multiple algorithms for searching for duplicates you should take a look a the Strategy pattern.


this.input.GroupBy(result => result)
    .Where(whereValueClause => whereValueClause.Count() > 1)
    .Select(indexValue => indexValue.Key);

Usully long verbose names are a good thing but here they don't. What is result? What is whereValueClause or indexValue? They are more confusing then helpful.

Generic items are mostly like x if there is no better name e.g. the first letter(s) of its type name.

Inside the Where you work with a group so the letter g would be just fine. The same applies to the Select where you work with groups agian.

Consider this:

input
    .GroupBy(x => x)
    .Where(g => g.Count() > 1)
    .Select(g => g.Key);

#region FindDuplicate

Regions are mostly a matter of taste. I don't like them because you have even more tiny + to expand/collapse. I find if you start using regions then your class/method/loop etc is too big and you should start thinking about refactoring it. Some people use them for grouping similar methods or even fields/properties. I suggest using them sparingly and only if you have a really good reason (usually there isn't and you need a new class/file/namespace).

(I wonder how many downvotes will this part cost me).


Generic search

You ask

But, is it possible to somehow transform my input into a generic?

Yes it is. You need to make the method generic, this means is should take any collection IEnumerable<T> and a comparer implementing the IEqualityComparer<T> interface so that it can tell whether two items are the same.

Here is one example.

The first one uses GroupBy

public static IEnumerable<T> FindDuplicates<T>(this IEnumerable<T> values, IEqualityComparer<T> comparer)
{
    return 
        values
        .GroupBy(x => x, comparer)
        .Where(g => g.Count() > 1)
        .Select(g => g.Key);
}

Comparer

For simple cases you can use the default comparer like

EqualityComparer<T>.Default

and for more complex types like a Person

class Person 
{ 
    public string FirstName { get; set; }

    public string LastName { get; set; }
}

you would write a class that implements the IEqualityComparer<T> interface:

class PersonComparer : IEqualityComparer<Person>
{ 
    public bool Equals(Person x, Person y)
    {
        return x.FirstName == y.FirstName && x.LastName == y.LastName;
    }

    public int GetHashCode(Person obj)
    {
        return $"{obj.LastName}, {obj.FirstName}".GetHashCode();
    }
}

This is really just a very simple example demonstrating the two methods that need to be implemented. You might want to make the name comparison case insensitive or trim the names or calculate a different hash-code. There are many ways to do it but now you know how to start.


C# 7

With C# 7 you could nicely implement it with a dictionary:

public static IEnumerable<T> FindDuplicates<T>(this IEnumerable<T> values, IEqualityComparer<T> comparer)
{
    var dictionary = new Dictionary<T, int>(comparer);
    foreach (var value in values)
    {
        dictionary[value] = dictionary.TryGetValue(value, out int counter) ? ++counter : 0;
    }
    return dictionary.Where(x => x.Value > 1).Select(x => x.Key);
}
\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for the detailed explanation. I did figure out the generic part, I have my method return a T[], since I wasn't positive on how to make say an array accept a method that returned IEnumerable. \$\endgroup\$ Commented Feb 7, 2017 at 17:57
  • \$\begingroup\$ One thing I am wondering though, since I am still a novice in C#, is how does one instantiate a IEqualityComparer, do I need to pass in any specific logic into it which explains how the comparison works? \$\endgroup\$ Commented Feb 7, 2017 at 17:58
  • \$\begingroup\$ @SomeStudent I edited the answer and added an example under Comparer. \$\endgroup\$
    – t3chb0t
    Commented Feb 7, 2017 at 18:08
1
\$\begingroup\$

I wouldn't use class for this, it's more of an extension method. Your LINQ version looks nice, you can make it into an extension method like this:

public static T[] FindDuplicates<T>(this IEnumerable<T> source)
{
    if (source == null)
    {
        throw new ArgumentNullException(nameof(source));
    }
    return source.GroupBy(result => result)
        .Where(whereValueClause => whereValueClause.Count() > 1)
        .Select(indexValue => indexValue.Key).ToArray();
}

Note that we have a null check there as this is an extension method we need to make sure it's working properly for anyone using it and of course handle exceptions if such occur.

There is some stuff that can be improved in your class but again you should only use this single method which makes your class redundant and I wont go over it.

Just to give a brief idea if you want to improve in general:

  • Naming
  • StringBuilder vs string concatenation
  • Weird return types
  • Some unnecessary comments
  • Regions

One comment that catches my eye:

Note how we do not need to sort the array, the grouping will do it for us.

Grouping doesn't means sorting.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for the clarification, looking at it my comment is indeed incorrect. I decided to use regions as a means of being able to allow for cleaner code viewing. As to the return type which returns just this, it was the ability of chaining methods together from the same class. \$\endgroup\$ Commented Feb 7, 2017 at 1:14
  • \$\begingroup\$ One more quick question, how to utilize the extension method? Only way I see using it is my creating my own separate static class, and placing it in there, otherwise it won't work. \$\endgroup\$ Commented Feb 7, 2017 at 1:27
  • \$\begingroup\$ you have to create a static class, name doesnt matter, MyExtensions, EnumerableMagic, ... \$\endgroup\$
    – pm100
    Commented Feb 7, 2017 at 1:57
  • \$\begingroup\$ @SomeStudent #region is great, I use it to separate constructors, properties, methods and events in classes. As well as explicit interface implementations. \$\endgroup\$ Commented Feb 7, 2017 at 2:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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