Return to Answer

• 1

• 75.5k
• 18
• 194
• 467

• 75.5k
• 18
• 194
• 467

using (Process cThread = new ProcessThread())
{
 m_cProcessThread = new Thread(new ThreadStart(cThread.InitProcess));
}

This would dispose cThread right away, which IMO would make the next line...

m_cProcessThread.Start();

...possibly throw some ObjectDisposedException, depending on what's going on in InitProcess - see @DanLyons' answer.

Your code file should have the following using statements at the top:

using System.ServiceProcess;
using System.ComponentModel;
using System.Threading;

This would allow you to get rid of the full qualifications, so this:

System.ServiceProcess.ServiceBase[] ServicesToRun = new System.ServiceProcess.ServiceBase[] { new ServiceTest() };

Can be shortened to that:

ServiceBase[] ServicesToRun = new ServiceBase[] { new ServiceTest() };

Or even:

var ServicesToRun = new ServiceBase[] { new ServiceTest() };

The naming style is confusing. Why all these prefixes?

private System.Threading.Thread m_cProcessThread;

Should be simply:

private Thread processThread;

No need for all these whitespaces, no need for this m_ prefix, and no need for that cryptic c either.

Why isn't components following the same convention anyway? If you're prefixing all instance-level fields with an m_, then I would expect components to be called m_components - be consistent!

That said, as much as I don't like random and Hungarian prefixes, I like starting my private fields with an underscore, like _components and _processThread, so as to avoid having to qualify them with this later on when I use them in a method with a components or a processThread parameter... but that's just personal preference; all that matters, is consistency.

Now, you have a Thread object called m_cProcessThread, and then you have a ProcessThread object called cThread - that is utterly confusing! I'd call the Thread object thread, and the ProcessThread object, processThread.

The whitespace here is annoyingly inconsistent:

protected override void Dispose( bool disposing )
{
 if ( disposing )
 {
 if (components != null) 
 {
 components.Dispose();
 }
 }
 base.Dispose( disposing );
}

There is no reason to fight your IDE to get these extra whitespaces after ( and before ). This is much less disturbing to read:

protected override void Dispose(bool disposing)
{
 if (disposing)
 {
 if (components != null) 
 {
 components.Dispose();
 }
 }
 base.Dispose(disposing);
}

Your OCD about disposing disposables isn't necessarily a bad thing. The rule of thumb however, is that whichever scope is creating a disposable object, should be responsible for disposing it. This means your OnStart method should either be disposing the thread it's creating (with the issues mentioned above), or not be creating it at all.

I'm not sure why you're not using Task objects over Thread though. Threads are much more complicated and error-prone than tasks. If this code is targeting .net 4.0 or higher, you should consider leveraging the Task Parallel Library (TPL, under System.Threading.Tasks), and use Task objects instead.