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.

First, I want to say that this code works, but I want to make sure this is the right way of doing it. I am writing this for a SMS text API that loops through each phone number. I sometimes gets hung up on 1 number or just takes time. It roughly takes 6-8 sec per number. So I thought I would write something that would make it multitreaded. I wrote this to allow the number of threads at one time to go up depending on the work load. So, I guess this is more of a code review. Thanks.

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

namespace TestTreading
{
    class Program
    {
        static public BlockingCollection<int> Queue = new BlockingCollection<int>();
        static public BlockingCollection<int> Finished = new BlockingCollection<int>();
        static public BlockingCollection<Thread> Threads = new BlockingCollection<Thread>();
        static public int QUCount = 0;
        static public int THCount = 0;
        static public int FNCount = 0;

        static void Main(string[] args)
        {
            BuildList();
            DateTime StartTime = DateTime.Now;

            QUCount = Queue.Count;
            THCount = Threads.Count;
            FNCount = Finished.Count;

            while (QUCount > 0)
            {
                THCount = Threads.Count;
                if (THCount < 20)
                {
                    Thread th1 = new Thread(LoopMethod);
                    Threads.Add(th1);
                    THCount = Threads.Count;
                    th1.Start();
                }
            }

            DateTime EndTime = DateTime.Now;
            Console.WriteLine(StartTime);
            Console.WriteLine(EndTime);
            Console.WriteLine(EndTime - StartTime);
            Console.ReadLine();
        }

        static void LoopMethod()
        {
            int QueueItem = Queue.Take();
            QUCount = Queue.Count;
            for (long i = 0; i < 100000; i++)
            {
                long result = i / 2;
            }
            string Display = string.Format("ID:{0} Left In Queue:{1}  Finished:{2}  Threads Running:{3}", QueueItem, QUCount, FNCount, THCount);
            Console.WriteLine(Display);
            Thread.Sleep(10);

            Finished.Add(QueueItem);
            FNCount = Finished.Count;
            Thread CurTh = Thread.CurrentThread;
            Threads.TryTake(out CurTh);
        }


        static void BuildList()
        {
            for (long i = 0; i < 5000; i++)
            {
                Random RandomNum = new Random();
                int Num = RandomNum.Next(1, 2);
                Queue.Add(Num);
            }
        }

    }
}
share|improve this question
    
Which .NET version are you using? Can you show the SMS API (ideal solution should use asynchronous calls to SMS API) –  almaz Mar 21 '13 at 22:08
    
@almaz Why? There is no need for offloading (since there is no GUI thread) and there also don't seem to be any scalability issues. So why do you say asynchronous calls would be ideal? Especially since they would make the code much more complicated in pre-C# 5. –  svick Mar 21 '13 at 22:48
    
@svick Because if SMS API exposes async calls then there is no need in creating additional threads. –  almaz Mar 22 '13 at 8:12
add comment

1 Answer

static public BlockingCollection<int> Queue
static public BlockingCollection<int> Finished
static public BlockingCollection<Thread> Threads
static public int QUCount
static public int THCount
static public int FNCount

There is no need to make any of these fields public. You shouldn't change the visibility from the default of private unless you have a reason to do it.

for (long i = 0; i < 5000; i++)
{
    Random RandomNum = new Random();
    int Num = RandomNum.Next(1, 2);
    Queue.Add(Num);
}

This is a very complicated way to generate 5000 1s. If you actually want to generate random numbers, you can't create a new Random for each iteration, you should instead create it once before the loop and then just call Next() inside it. That's because Random is initialized by the current time by default, and the time as measured by .Net doesn't change that often. That means you would get runs of the same number, which is not random.

THCount = Threads.Count;
FNCount = Finished.Count;

These lines don't do anything here, both collections are going to be empty at this point. In fact, the Count fields are completely useless, you should just use the Count property of each collection instead.

while (QUCount > 0)
{
    if (THCount < 20)
    {
        Thread th1 = new Thread(LoopMethod);
        th1.Start();
    }
}

This code doesn't make much sense. It's certainly possible that it will start all 20 threads even if there's just a single item in the queue, because you could create all of the threads before the first one starts.

What's much worse is that it will keep on iterating while the other threads are working. This means a whole single core of your CPU will be fully busy, doing basically nothing.

Thread th1 = new Thread(LoopMethod);

Threads are relatively heavy-weight, you should usually create a new thread only for a long running operation. If you have thousands of operations, you shouldn't create thousands of threads. Instead, you should create just a few of them and have a loop inside each.

Threads.Add(th1);

There is no need to do this. You don't actually need a collection of threads, you just need to know their count. So you should use an int field for that, and increment that value in a thread-safe manner.

Thread CurTh = Thread.CurrentThread;
Threads.TryTake(out CurTh);

This code looks like it's trying to take the current thread out of the collection. In fact, it does no such thing. Instead, it removes the first item from the collection (i.e. the oldest item that hasn't yet been removed from the collection) and assigns that to the CurTh variable. The out modifier means, among other things, that the parameter is used for output only and if it has some value before the method is called, it's going to be ignored.

int QUCount
int THCount
int FNCount
Thread th1

You shouldn't abbreviate your field and variable names like this. You should write their names in full, (e.g. QueueCount or thread), because code is read more often than it is written, so it makes sense to strive for code that is easy to read, not code that is easy to write. (And it doesn't even make much difference with autocompletion.)


Usually, it's much better to use TPL for parallel processing, instead of manually using threads. Using GetConsumingEnumerable() and Parallel.ForEach() would make your code shorter, more readable and much more efficient. (CPU efficiency usually doesn't matter much for code like this, but you're wasting CPU a lot.)

class Program
{
    static BlockingCollection<int> Queue = new BlockingCollection<int>();
    static BlockingCollection<int> Finished = new BlockingCollection<int>();

    static void Main()
    {
        BuildList();

        Parallel.ForEach(
            Queue.GetConsumingEnumerable(),
            new ParallelOptions { MaxDegreeOfParallelism = 20 },
            ProcessItem);
    }

    static void ProcessItem(int i)
    {
        int result = /* somehow calculate the result here */;
        Finished.Add(result);
    }
}

(I have also removed your logging for brevity.)

share|improve this answer
add comment

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.