Calculate pairs in a Set ("Sherlock and Pairs" HackerRank challenge)

Question

I have started reading clean code and want to improve my coding practices. Here is my attempt at solving an online puzzle. Please review the code and let me know how could I have written it better in terms of logic, coding practices or any other advice you might have.

Problem Statement

Sherlock is given an array of N integers (\$A_0, A_1 \ldots A_{N-1}\$) by Watson. Now Watson asks Sherlock how many different pairs of indices \$i\$ and \$j\$ exist such that \$i\$ is not equal to \$j\$ but \$A_i\$ is equal to \$A_j\$.

That is, Sherlock has to count the total number of pairs of indices \$(i,j)\$ where \$A_i =Aj\$ AND \$i≠j\$.

Input Format

The first line contains \$T\$, the number of test cases. \$T\$ test cases follow. Each test case consists of two lines; the first line contains an integer \$N\$, the size of array, while the next line contains N space separated integers.

Output Format

For each test case, print the required answer on a different line.

Constraints

\$1≤T≤10\$

\$1≤N≤10^5\$

\$1≤A[i]≤10^6\$

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

namespace NoOfPairsInASet
{
    class Solution
    {
        static Dictionary<Int64, Int64> CountOfElements = new Dictionary<Int64, Int64>();
        static Int64 Sum = 0;
        static Int64[] data;
        static void Main(string[] args)
        {
            int TestCases = Convert.ToInt32(Console.ReadLine());
            //int TestCases = 1;
            for (int t = 0; t < TestCases; t++)
            {
                CalculatePairs();
                Console.WriteLine(Sum);
            }
        }

        private static void CalculatePairs()
        {
            int NoOfElements = Convert.ToInt32(Console.ReadLine());
            string Elements = Console.ReadLine();

            //int NoOfElements = 3;
            //string Elements = "1 1 2 1 2";

            string[] ArrayOfElements = Elements.Split(' ');
            data = new Int64[ArrayOfElements.Length];

            ConvertStringToInt(ArrayOfElements);
            GenerateDictionary();
            CalculateSum();

        }

        private static void ConvertStringToInt(string[] ArrayOfElements)
        {
            for (Int64 i = 0; i < ArrayOfElements.Length; i++)
            {
                data[i] = Convert.ToInt64(ArrayOfElements[i]);
            }
        }

        private static void GenerateDictionary()
        {
            CountOfElements.Clear();
            int DEFAULTCOUNT = 1;
            for (Int64 i = 0; i < data.Length; i++)
            {
                if (CountOfElements.ContainsKey(data[i]) == true)
                {
                    IncrementCounter(i);
                }
                else
                {
                    CountOfElements.Add(data[i], DEFAULTCOUNT);
                }
            }
        }

        private static void IncrementCounter(Int64 i)
        {
            Int64 count = CountOfElements[(data[i])];
            count++;
            CountOfElements[(data[i])] = count;
        }

        private static void CalculateSum()
        {
            Sum = 0;

            foreach (KeyValuePair<Int64, Int64> item in CountOfElements)
            {
                if (item.Value > 1)
                {
                    Sum += CalculatePermutationOfValue(item.Value, 2);
                }
            }
        }

        private static Int64 CalculatePermutationOfValue(Int64 n, Int64 r)
        {
            //Int64 P = factorial(n) / factorial(n - r); // with r = 2, this simplifies to n*n-1
            Int64 P = n * (n - 1);
            return P;
        }

        //private static Int64 factorial(Int64 n)
        //{
        //    Int64 fact = 1;
        //    for (Int64 i = n; i >= 1; i--)
        //    {
        //        fact = fact * i;
        //    }
        //    return fact;
        //}
    }
}

I have kept the test cases still commented. Is that okay? Also, I have still kept the factorial function as it can be used in the future. Should I remove the factorial function or can I keep it commented?

Answer 1

Dead code

Kill it. If you have some commented out code that you aren't using just delete it. It adds noise to the reader and doesn't add anything to the code. Not to mention factorial is an incredibly simple thing to add back in:

public int Factorial(int number)
{
    return number == 0 ? 1 : Enumerable.Range(1, number).Aggregate((i, j) => i * j);
}

Note from the above that, like you, I prefer non-recursive solutions where possible.

You also have dead code here:

private static Int64 CalculatePermutationOfValue(Int64 n, Int64 r)
{
    //Int64 P = factorial(n) / factorial(n - r); // with r = 2, this simplifies to n*n-1
    Int64 P = n * (n - 1);
    return P;
}

Remove the r parameter and delete the commented code. Also, don't bother with P = ... just return n * (n - 1);.

Naming

You've used a sensible name for namespace and class - kudos!
Prefer long to Int64 - it looks nicer and is more standard.
All local variables should be named in camelCase P -> p.
Quite a few of your names are quite good - but can be tightened up in places: try to be as descriptive as you can.

Other stuff

Everything is static - not necessarily a problem but some people get upset about that sort of thing.

---

This function can be massively simplified (I think):

private static void IncrementCounter(Int64 i)
{
    Int64 count = CountOfElements[(data[i])];
    count++;
    CountOfElements[(data[i])] = count;
}

To

 CountOfElements[(data[i])]++;

---

You should rely on fields less and pass things through parameters. Side effects suck when you're trying to follow execution. E.g.

private static void GenerateDictionary()

It returns void but it generates a dictionary? Now I have to dig into the code to find where my dictionary has been generated.

---

You should write instructions out the Console. Users don't like spamming the keyboard hoping to do the right thing(TM).

---

Never trust a user to input a sensible value:

int TestCases = Convert.ToInt32(Console.ReadLine());

Should be more along the lines of:

var numberOfTestCases = 0;
// This only works if MinimumNumberOfTestCases >= 1
while (numberOfTestCases < MinimumNumberOfTestCases || numberOfTestCases > MaximumNumberOfTestCases)
{
    Console.WriteLine(UserMessages.PromptForNumberOfTestCases);
    int.TryParse(Console.ReadLine(), out numberOfTestCases);
}

Note that I've pretended that you have a class called UserMessages with a property/field containing the text to prompt to a user. I've also assumed that you have created well named constants for the min/max number of test cases.

Linq

You should defintely learn some LINQ (Language integrated query). For example, your ConvertStringToInt becomes redundant. I didn't mention it earlier but that's also a poorly named function. It converts the items of a string array to ints - that isn't clear from the name.

Anyway, with LINQ:

ArrayOfElements.Select(item => int.Parse(item)).ToArray();

Notice that I'm just doing a straight forward parse - if the item isn't a valid integer we can't continue so might as well fail quickly.