14
\$\begingroup\$

The original question is on careercup.

Write a multi threaded C code with one thread printing all even numbers and the other all odd numbers. The output should always be in sequence ie. 0,1,2,3,4....etc

Now I want to use C# for it.

class Program
{
    static Object obj = new Object();
    static Thread t1;
    static Thread t2;
    static LinkedList<int> a = new LinkedList<int>();
    static void Main(string[] args)
    {
        for (int i = 0; i < 10; i++)
        {
            a.AddLast(i);
        }

        t1 = new Thread(PrintOdd);
        t2 = new Thread(PrintEven);
        t1.Name = "Odd";
        t2.Name = "Even";
        t1.Start();
        t2.Start();
        t1.Join();
        t2.Join();

        Console.WriteLine("Done!");
        Console.Read();
    }

    private static void PrintOdd()
    {
        while (true)
        {
            if (a.Count == 0)
                break;
            lock (obj)
            {
                int x = a.First();
                if (x % 2 != 0)
                {
                    Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                    a.RemoveFirst();
                }
            }
        }
    }

    private static void PrintEven()
    {
        while (true)
        { 
            lock (obj)
            {
                if (a.Count == 0)
                    break;
                int x = a.First();
                if (x % 2 == 0)
                {
                    Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                    a.RemoveFirst();
                }
            }
        }
    }
}

Any improvements?

\$\endgroup\$
19
\$\begingroup\$

This looks very good to me, but I am not very advanced at C# myself. I can give you some tips, though.

First, you should not use ambiguous names like t1 and t2. You should use more descriptive names like EvenThread and OddThread instead.

Second, you can shorten this:

while (true)
{
  if (a.Count == 0)
    break;

Into this:

while (Numbers.Count > 0)  // For PrintOdd()

And this:

while (Numbers.Count > 1)  // For PrintEven()

Also, you could look into other forms of mutual exclusion techniques, such as semaphores and mutexes, instead of just using an Object.

This is an implementation with a SemaphoreSlim (only code shown is changed):

using System.Threading;

class Program
{
    static SemaphoreSlim ThreadLock = new SemaphoreSlim(1,1);
    static Thread Odd;
    static Thread Even;
    static LinkedList<int> Numbers = new LinkedList<int>();
    static void Main(string[] args)
    {
        for (int i = 0; i < 10; i++)
        {
            Numbers.AddLast(i);
        }

        Odd = new Thread(PrintOdd);
        Even = new Thread(PrintEven);
        Odd.Name = "Odd";
        Even.Name = "Even";
        Odd.Start();
        Even.Start();
        Odd.Join();
        Even.Join();

        Console.WriteLine("Done!");
        Console.Read();
    }

    private static void PrintOdd()
    {
        while (Numbers.Count > 0)
        {
            ThreadLock.Wait();

            int x = Numbers.First();
            if (x % 2 != 0)
            {
                Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                Numbers.RemoveFirst();
            }

            ThreadLock.Release();
        }
    }

    private static void PrintEven()
    {
        while (Numbers.Count > 1)
        {
            ThreadLock.Wait();

            int x = Numbers.First();
            if (x % 2 == 0)
            {
                Console.WriteLine(Thread.CurrentThread.Name + " " + x);
                Numbers.RemoveFirst();
            }

            ThreadLock.Release();
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

Instead of using same logic in two methods, you can write just one method for both of the threads.

Please see below:

    private static LinkedList<int> _numbers;
    private static readonly object _numLock = new object();

    static void Main(string[] args)
    {
        _numbers = new LinkedList<int>();
        for (int i = 0; i < 50; i++)
        {
            _numbers.AddLast(i);
        }

        var evenThread = new Thread(x => PrintNumbers(true));
        var oddThread = new Thread(x => PrintNumbers(false));

        evenThread.Name = "Even";
        oddThread.Name = "Odd";

        evenThread.Start();
        oddThread.Start();

        evenThread.Join();
        oddThread.Join();

        Console.WriteLine("Done");
        Console.Read();
    }

    private static void PrintNumbers(bool isEven)
    {
        bool condition = true;
        while (condition)
        {
            lock (_numLock)
            {
                var first = _numbers.First;
                if (isEven && first.Value % 2 == 0)
                {
                    WriteAndRemove(first);
                }
                else if (!isEven && first.Value % 2 == 1)
                {
                    WriteAndRemove(first);
                }
                condition = isEven ? _numbers.Count > 1 : _numbers.Count > 0;
            }

        }
    }

    private static void WriteAndRemove(LinkedListNode<int> first)
    {
        Console.WriteLine(String.Concat(Thread.CurrentThread.Name," " ,first.Value));
        _numbers.RemoveFirst();
    }
\$\endgroup\$
  • \$\begingroup\$ You don't need an else if with the same contents, if you change the condition to (first.Value % 2 == (isEven ? 1 : 0)) \$\endgroup\$ – Toby Speight Jan 29 at 16:04
-3
\$\begingroup\$

Instead of checking the modulo via if (x % 2 == 0) and if (x % 2 != 0) you could simply use Console.WriteLine(Thread.CurrentThread.Name + " " + (2*x)); and Console.WriteLine(Thread.CurrentThread.Name + " " + (2*x+1)); to display even an odd numbers respectively. As a mathematician this seems more reasonable to me.

\$\endgroup\$
  • \$\begingroup\$ If the collection starts at 1 this won't work. For such an interview question one should always assume that the input for the method to be written can change ( like in the real job ) \$\endgroup\$ – Heslacher Dec 18 '14 at 8:19
  • \$\begingroup\$ Yeah but since all even and odd numbers should be printed this would produce the desired result. \$\endgroup\$ – Labello Dec 18 '14 at 8:22
  • \$\begingroup\$ Won't work. Because you have 2 threads. It prints with the code in the question, using your idea, Odd 1 Odd 3 Odd 5 ... Odd 19. No Even there ;-( \$\endgroup\$ – Heslacher Dec 18 '14 at 8:37
  • \$\begingroup\$ This does seem a little strange because 2*x is always even. So without the actual code there is nothing more I can do. \$\endgroup\$ – Labello Dec 18 '14 at 9:02
  • 1
    \$\begingroup\$ The original code works, because the loops will iterate until a.Count == 0 and they only print and remove the entry from the list if the modulo condition is true. \$\endgroup\$ – Heslacher Dec 18 '14 at 9:34
-3
\$\begingroup\$

Above application is not a really good solution.

There are the following problems in it:

  • Why do you want to thread to wait? It does not finds even or odd number since it is checking on first value only.

  • If your list size is odd number like 11 or 13

Please do proper unit test to ensure it also for even list size.

\$\endgroup\$
  • 6
    \$\begingroup\$ Welcome to Code Review AMIT - I have reviewed your answer, and while I believe it is an answer, what it's saying is wrong. The code does not have the problems your answer says it does. \$\endgroup\$ – rolfl Jan 23 '15 at 11:23

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.