HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Simple class to let a thread make a clean work

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simplemakethreadletworkclassclean

Problem

I was working on the last days with threads and I didn't want to use ugly methods like Abort(), etc.

So I did a simple class which does use one of the most performant and better alternative to Abort(). The class is named as CThread.cs which would stand for CleanThread.cs.

Class code:

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

public class CThread
{
    public List CreatedThreads = new List();

    private void MyFunction(Action callback, bool inLoop, int currentIndex)
    {
        if (inLoop)
        {
            while (!CreatedThreads[currentIndex].IsCancellationRequested)
            {
                callback();
            }
        }
        else { callback(); }
    }

    private int getLastIndex()
    {
        int index = 0;
        foreach (CancellationTokenSource item in CreatedThreads)
        {
            ++index;
        }

        return (index - 1);
    }

    public new void Start(Action callback, bool inLoop)
    {
        CancellationTokenSource tokenSource = new CancellationTokenSource();

        CreatedThreads.Add(tokenSource);
        int currentIndex = getLastIndex();
        Task task = Task.Run(() => MyFunction(callback, inLoop, currentIndex)); // use Task.Factory.StartNew for .net 4.0
    }

    public new void Stop(int threadIndex)
    {
        CreatedThreads[threadIndex].Cancel();
        Console.WriteLine("Thread has been stopped successfully! Press any key to continue...");
        Console.ReadKey();
    }
}


and finally, how I use it:

```
using System;

namespace CThreadExample
{
internal class Program
{
private static void printFunction()
{
Console.WriteLine("Hello world!");
System.Threading.Thread.Sleep(120);
}

private static void Main(string[] args)
{
CThread myThread = new CThread();
myThread.Start(printFunction, true);

// when the user pres

Solution

This doesn't actually work as intended. You're using the CancellationTokenSource as a bool to decide whether to re-invoke the callback. That's not stopping a thread; that is not re-invoking the callback when it returns. The only reason why it appears to work for you is:

  • Your test callback is very short. Try inserting a long-running callback.



  • You print "Thread has been stopped successfully! Press any key to continue..." right after calling Cancel(). That statement is false; the only thing that has happened at that point is the CancellationTokenSource has been cancelled.



Other non-functional issues:

  • The CreatedThreads list doesn't hold threads, it holds CancellationTokenSources. Name the list appropriately.



  • Why is CreatedThreads public? It seems like something you wouldn't want the class to expose. Make it private.



  • This method is very strange:



private int getLastIndex()
{
    int index = 0;
    foreach (CancellationTokenSource item in CreatedThreads)
    {
        ++index;
    }

    return (index - 1);
}


Why does it not just return CreatedThreads.Count - 1 rather than iterating through the whole list? At that point there's no need for a method for it; Start can just do int currentIndex = CurrentThreads.Count - 1;

  • It is very awkward to have the Stop method take an index of the thread to stop, especially when an index was not supplied to or returned from Start. How is the caller of the method to know what index their thread is?



Finally I assume the only reason why you are calling Console methods in the CThread class is because you're using it for testing. Otherwise you definitely should not be doing that.

Code Snippets

private int getLastIndex()
{
    int index = 0;
    foreach (CancellationTokenSource item in CreatedThreads)
    {
        ++index;
    }

    return (index - 1);
}

Context

StackExchange Code Review Q#127801, answer score: 6

Revisions (0)

No revisions yet.