current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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");
            }
        }
    }
}
share|improve this question
    
Do you need to write this yourself, or are you allowed to use library functions such as msdn.microsoft.com/en-us/library/vstudio/… –  jessehouwing yesterday

3 Answers 3

up vote 4 down vote accepted

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);
share|improve this answer
    
Thanks, I didn't know that hashsets ignore if it already contains the element. It makes sense –  Aaron yesterday
    
@Aaron: it's the only thing that would make sense. What else could it do? –  njzk2 21 hours ago
up vote 4 down vote

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.

share|improve this answer
1  
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. –  njzk2 21 hours ago
up vote 2 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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