Joystick helper class

I don't use C# very often so would be good to get some feedback on this helper class

```
using SharpDX.DirectInput;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace SprintTimer
{
class JoystickHelper
{
private DirectInput directInput;
private Thread pollingThread;
private Joystick joystick;
private int startButtonOffset = -1;
private int lapButtonOffset = -1;

public JoystickHelper()
{
directInput = new DirectInput();
}

public List DetectDevices() {
List joystickDescriptors = new List();

// check for gamepads
foreach (var deviceInstance in directInput.GetDevices(DeviceType.Gamepad, DeviceEnumerationFlags.AllDevices))
{
joystickDescriptors.Add(new JoystickDescriptor(deviceInstance.InstanceGuid, deviceInstance.InstanceName));
}

// check for joysticks
foreach (var deviceInstance in directInput.GetDevices(DeviceType.Joystick, DeviceEnumerationFlags.AllDevices))
{
joystickDescriptors.Add(new JoystickDescriptor(deviceInstance.InstanceGuid, deviceInstance.InstanceName));
}

return joystickDescriptors;
}

public void StartCapture(Guid joystickGuid, int startButtonOffset, int lapButtonOffset)
{
this.startButtonOffset = startButtonOffset;
this.lapButtonOffset = lapButtonOffset;
StartCapture(joystickGuid);
}

public void StartCapture(Guid joystickGuid)
{

joystick = new Joystick(directInput, joystickGuid);

joystick.Properties.BufferSize = 128;

joystick.Acquire();

pollingThread = new Thread(new ThreadStart(PollJoystick));
pollingThread.Start();

// Spin for a while waiting for t

A few things:

-
The DetectDevices method can be shortened with the help of LINQ:

public IList DetectDevices()
{
    return directInput.GetDevices(DeviceType.Gamepad, DeviceEnumerationFlags.AllDevices)
                      .Concat(directInput.GetDevices(DeviceType.Joystick, DeviceEnumerationFlags.AllDevices))
                      .Select(d => new JoystickDescriptor(d.InstanceGuid, d.InstanceName))
                      .ToList();
}

I would also question if it is really required to return a List<>. An IEnumerable<> might be sufficient and then you can get rid of the ToList() as well.

-
This is somewhat dubious:

// Spin for a while waiting for the started thread to become alive
    while (!pollingThread.IsAlive) ;

Not sure what problem this is supposed to solve but I doubt it is really necessary.

-
Finishing a thread by calling Abort() is very nasty. You should introduce a class member flag like _QuitPolling which you set to true once you want to quit. You can then Join() with a timeout and still Abort() if it hasn't finished. Your main loop in PollJoystick would then be changed to

while (!_QuitPolling)
{
     ....
}

And StopCapture would change into:

public void StopCapture()
{
    if (pollingThread != null)
    {
        _QuitPolling = true;
        if (!pollingThread.Join(TimeSpan.FromMilliseconds(500)))
        {
            pollingThread.Abort();
        }
    }

    if (joystick != null)
    {
        joystick.Dispose();
    }
}

-
The multiple of data is still data and not datas.

-
The main thread method PollJoystick can be simplified by introducing a little helper method which filters out relevant updates and again a bit of LINQ:

Helper method:

private bool IsRelevantUpdate(JoystickUpdate state)
{
    return state.Offset >= JoystickOffset.Buttons0 && state.Offset <= JoystickOffset.Buttons127 && state.Value == 128;
}

Refactored main loop:

JoystickUpdate[] data = joystick.GetBufferedData();
foreach (JoystickUpdate state in data.Where(IsRelevantUpdate))
{
    ...
}

Resulting in nesting reduced by 2 levels.

Update:

DirectInput as well as Joystick are IDisposable. The general rules around this are:

• If you create an object which is IDisposable and you own it (your code determines the lifetime and ownership is not transferred to another entity) then you are required to dispose of it after you are finished using it.

• If the IDisposable object you own is a class member then your class should become IDisposable as well disposing of any members in its own Dispose method.

If these rules are followed you are less likely to forget to dispose of objects which require it.

For your code this means that you should implement IDisposable which could just call StopCapture. In addition to joystick you should also dispose of directInput at that point (which means that you probably should move the creation of directInput into StartCapture if you want to start/stop multiple times).

StackExchange Code Review Q#68711, answer score: 5