5
\$\begingroup\$

I am preparing for a interview for a junior level c# position. I take two arrays and find the common int between both of them. Let me know if you have any feedback.

using System;
using System.Collections.Generic;

namespace CommonElement
{
    class MainClass
    {
        static void EnterArray1IntoHashSet (int[] array1, HashSet<int> hs){
            foreach (int num in array1)
            {
                if (!hs.Contains (num)) 
                {
                    hs.Add (num);
                }
            }
        }

        static int FindCommonInt(int[] array2, HashSet<int> hs){
            foreach (int num in array2)
            {
                if (hs.Contains (num)) 
                {
                    return num;
                }
            }
            return -1;
        }

        public static void Main (string[] args)
        {
            int[] array1 = new int[7]{ 5, 6, 7, 8, 1, 2, 3 };
            int[] array2 = new int[7]{ 13, 22, 3, 45, 67, 73, 85};
            HashSet<int> hs = new HashSet<int> ();
            EnterArray1IntoHashSet (array1, hs);
            int commonElement = FindCommonInt(array2, hs);
            if (commonElement != -1)
            {
                Console.WriteLine (commonElement);
            } 
            else 
            {
                Console.WriteLine ("No Common Element");
            }
        }
    }
}
\$\endgroup\$
7
\$\begingroup\$

Improving EnterArray1IntoHashSet

Why do you pass a hash set to EnterArray1IntoHashSet ? It would be better to let the method create, populate and return the hash set.

You don't need to check if an element is inside a hash set before adding it: if the element already exists, it will be simply ignored.

The name of this function is unnecessarily tedious. It has a nice general purpose, and you can name it accordingly.

static HashSet<int> ToHashSet(int[] array1)
{
    HashSet<int> set = new HashSet<int>();
    foreach (int num in array1)
    {
        set.Add(num);
    }
    return set;
}

Coding style

The placement of opening braces { is inconsistent: sometimes you put it on the next line, sometimes on the same line. The .NET convention is to put it on the next line.

It's also unusual to put a space between method name and the ( when calling them, for example this:

Console.WriteLine (commonElement);

Just omit the unnecessary space:

Console.WriteLine(commonElement);
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks, I didn't know that hashsets ignore if it already contains the element. It makes sense \$\endgroup\$ – Aaron Aug 8 '15 at 23:35
  • \$\begingroup\$ @Aaron: it's the only thing that would make sense. What else could it do? \$\endgroup\$ – njzk2 Aug 9 '15 at 3:37
  • 1
    \$\begingroup\$ Bonus: set the initial capacity of the HashSet in the constructor with the size of the array: new HashSet<int>(array1.Length);. \$\endgroup\$ – Jesse C. Slicer Feb 1 '18 at 16:35
5
\$\begingroup\$

Returning -1 is ambiguous.

FindCommonInt({1,2}, {3,4}) #=> -1
FindCommonInt({1,2, -1}, {3,4, -1}) #=> -1

Maybe you should raise an exception if you don't find the item.

|improve this answer|||||
\$\endgroup\$
  • 1
    \$\begingroup\$ or return the index in the second array (which would make -1 valid). or even return a list of all common ints, in which case the not found result would be an empty list. \$\endgroup\$ – njzk2 Aug 9 '15 at 3:39
3
\$\begingroup\$

Also @janos did a good job with the ToHashSet() method this is superflous because one of the constructors of the HashSet just takes an IEnumerable to be initialized with.

So this

HashSet<int> hs = new HashSet<int>(array1);

does the same thing like the ToHashSet() method.

But do you need a HashSet at all? I would say no, because you should use the Enumerable.Intersect() method. This would result in

IEnumerable<int> commonElements = array1.Intersect(array2);
if (commonElements.Any())
{
    foreach (int element in commonElements)
    {
        Console.WriteLine(element);
    }

}
else
{
    Console.WriteLine("No Common Element");
}

What happens by using your implementation if the arrays intersect by more than one element ? You will only find one of them. Using the approach above will return each common element.

|improve this answer|||||
\$\endgroup\$
0
\$\begingroup\$

On top of the other answers, I think in an interview it would be a good idea to use "main" to set up variables and call a function which encapsulates everything.

The question asks to find the common element from 2 Arrays, not a hashset and an array. I would edit the code to make a function which takes in 2 arrays, converts one to a hashset, then calls a second function which takes an array and a hashset (like the one you have).

I think this would appeal to interviewers. You're showing them that you can be given a task like this and write it so it can be useable very easily by others (abstracting out complexity). You're not just proving you can code, but that you can be used (as a resource) by the company in an efficient manner.

So your "main" becomes:

public static void Main (string[] args)
    {
        int[] array1 = new int[7]{ 5, 6, 7, 8, 1, 2, 3 };
        int[] array2 = new int[7]{ 13, 22, 3, 45, 67, 73, 85};
        int commonElement = FindCommonInt(array1, array2);

        ...console.write, etc if you actually need it

So now another dev can come alone and use FindCommonInt(int[] a, int[] b) without worrying about doing any of the messy converting to hashsets etc.

|improve this answer|||||
\$\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.