// static class is a tight dependency, hard to mock out PCANBasic.Write(_handle, ref msg);
public object SyncRoot { get; } // .. create a new object() in the constructor
method Open
Since no lock is used, another thread can call the method after we think it is not opened.
if (IsOpen) { return true; } // .. other code that assumes IsOpen == false
We no longer store the handle and use the newly created interface.
IsOpen = TryInitializePCANChannel(out _handle);
IsOpen = _canBusService.Initialize();
What happens if the call was not succesfull?
if(IsOpen) { _canbusListenerThread = new Thread(new ThreadStart(CanbusListener)); _canbusListenerThread.Start(); } // else ??
Invert the condition and trow on failure.
if (!IsOpen)
{
throw new IOException("Failure initializing CAN bus");
}
Use throw to keep the original stacktrace.
catch(Exception ex) { Log.Error("Error opening PCAN connection: {Message}, Error: {Error}", ex.Message, ex); throw ex; }
catch(Exception ex)
{
Log.Error("Error opening PCAN connection: {Message}, Error: {Error}", ex.Message, ex);
throw; // preserves stacktrace
}
refactor Open to: use the lock, _canBusService, throw an exception when initialization was rejected gracefully and propagate the error preserving stacktrace
public bool Open()
{
lock (SyncRoot)
{
if (IsOpen) { return true; }
try
{
IsOpen = _canBusService.Initialize();
if (!IsOpen)
{
throw new IOException("Failure initializing CAN bus");
}
_canbusListenerThread = new Thread(new ThreadStart(CanbusListener));
_canbusListenerThread.Start();
}
catch(Exception ex)
{
Log.Error("Error opening PCAN connection: {Message}, Error: {Error}", ex.Message, ex);
throw;
}
return success;
}
}
method WriteCanMsg
Guard against bad user input.
msg = msg ?? throw new ArgumentNullException(nameof(msg));
Same tips as with Open: use lock, interface, throw
refactor WriteCanMsg to: use the lock, _canBusService, arg checks and propagate the error preserving stacktrace