Async insecurities

I feel like I'm overdoing the async await stuff. I'm just learning. Maybe this isn't the best instance to be using this stuff.

I have a system with sensors that detect the completion of some action. I decided to approach this solution using asynchronous programming since I still need to communicate with other controllers as well as track the progress in changing states. Here is a snippet of the mess I made.

Is this a good instance to use this or should I revert back to tasks and events?

private async void StartTest_Click(object sender, RoutedEventArgs e)
        {

            if (AdamCont == null)
            { return; }

            Abort = new CancellationTokenSource();

            int reached = await AdamCont.RunTestConfigReached(Abort.Token);

            if (reached == 0)
            {
                LightControl(SignalLight.GreenLight);                   
                Model.TestState = TestCycleState.NotInTest;
            }
            else if (reached > 0)
            {
                LightControl(SignalLight.RedLight);                   
                Model.TestState = TestCycleState.NotInTest;
               // OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
                //Now failedPosition can be called directly.
                FailedPosition( new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
            }
            else
            {
                LightControl(SignalLight.AmberLight);                   
                Model.TestState = TestCycleState.NotInTest;
            }

        }

The call is waiting on this sequence of operations.

```
public async Task RunTestConfigReached(CancellationToken abortToken)
{
//extend seal bar
this.SetDIOState(true, DOChannels_5045_1.SEAL_CLAMP_DOWN);
this.SetDIOState(false, DOChannels_5

The first thing that needs to be addressed is the indentation and the spacing, because that's the real issue here.

You should line your braces with the start of the line above it, like so :

if()
{
}

Instead of :

if(reached == 0)
   {
    // Yay it completed.
   }

While we're at it, this code is weird :

if(reached == 0)
{
    // Yay it completed.
}
else if(reached > 0)
{
    // Boo it didn't.
    OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed",reached)));
}

What happens when reached is below zero? Did it completed or not? Either way, have an empty content for your if is weird, you should remove it, since you do nothing in it.

if(reached > 0)
{
    //Didn't succeed.
    OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed",reached)));
}

async void is usually bad. Consider returning a async Task, because otherwise the one that launches the async task won't be able to "follow" it. If the "launcher" doesn't want to follow it, that's his problem, but you must give him the opportunity to do so.

Instead of :

if (reached == 0)
{
    LightControl(SignalLight.GreenLight);                   
    Model.TestState = TestCycleState.NotInTest;
}
else if (reached > 0)
{
    LightControl(SignalLight.RedLight);                   
    Model.TestState = TestCycleState.NotInTest;
   // OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
    //Now failedPosition can be called directly.
    FailedPosition( new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
}
else
{
    LightControl(SignalLight.AmberLight);                   
    Model.TestState = TestCycleState.NotInTest;
}

Why don't you just do :

if (reached <= 0)
{
    LightControl(SignalLight.GreenLight);                   
    Model.TestState = TestCycleState.NotInTest;
}
else
{
    LightControl(SignalLight.RedLight);                   
    Model.TestState = TestCycleState.NotInTest;
   // OnFailed(this, new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
    //Now failedPosition can be called directly.
    FailedPosition( new FailedPositionEventArgs(Model.TestState, String.Format("Step {0} in test configuration failed", reached)));
}

It means the same thing, there's no code duplication and less conditional instruction if/else.

StackExchange Code Review Q#111834, answer score: 8