A Note class which plays a note and stops it after specific period of time

This code plays a note for a specific period of time using NAudio.dll 1.3.8.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using NAudio.Midi;
using System.Threading;

namespace SoundVision
{
    class Note
    {
        public Note(MidiOutWrapper midi_output,int number, int duration, int volume=127, int channel=0)
        {
            if (midi_output == null)
                return;
            //midi_output is not null because it's referenced in this constructor and therefore cannot be collected
            //Check for disposed is not to rely on disposed resources in Send function
            lock (midi_output)
            {
                if (!midi_output.disposed)
                    midi_output.Send(MidiMessage.StartNote(number, volume, channel).RawData);

                Timer t = new Timer(_ =>
                {
                    lock (midi_output)
                    {
                        if(!midi_output.disposed)
                            midi_output.Send(MidiMessage.StopNote(number, 0, channel).RawData);
                    }
                },
                null, duration, Timeout.Infinite);
            }

        }
    }
}

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using NAudio.Midi;

namespace SoundVision
{
    class MidiOutWrapper:MidiOut
    {
        public MidiOutWrapper(int device) : base(device)
        {
            disposed = false;
        }

        public bool disposed { get; private set; }
        //kill() is called only during locking of object, so in Note midi_output can't become disposed inside lock
        public void kill()
        {
            disposed = true;
            Reset();
            Close();
        }
    }
}

Why is this a class? It contains no data and has no methods; the only thing it has is a constructor. This would be more suitable as a method.

Let's step back and look at the desired behaviour:


This code plays a note for a specific period of time.

Here is the simplest way I can think of to achieve this:

public static async Task PlayNoteAsync(
    MidiOut midiOut,
    int note,
    TimeSpan duration,
    int volume = 127,
    int channel = 0)
{
    midiOut.Send(MidiMessage.StartNote(note, volume, channel).RawData);
    await Task.Delay(duration).ConfigureAwait(false);
    midiOut.Send(MidiMessage.StopNote(note, volume, channel).RawData);
}

We just:

• Start playing the note

• Wait the desired amount of time

• Stop playing the note

Much of the original code is concerned with whether or not the MidiOut object has been disposed. I don't believe it should be the responsibility of this method to check if the MidiOut has been disposed; I would suggest instead to restructure the client code such that MidiOut is not disposed until the notes have stopped playing (or play has been cancelled, see below). Unfortunately, it's hard to be more specific about how to do this without an example of the client code. I would also recommend removing the MidiOutWrapper class.

You might want to support cancellation with a CancellationToken:

public static async Task PlayNoteAsync(
    MidiOut midiOut,
    int note,
    TimeSpan duration,
    int volume = 127,
    int channel = 0,
    CancellationToken cancellationToken = default(CancellationToken))
{
    // Consider using cancellationToken.ThrowIfCancellationRequested(); here.
    midiOut.Send(MidiMessage.StartNote(note, volume, channel).RawData);

    try
    {
        // Task.Delay will throw a TaskCanceledException if the
        // cancellationToken is signalled before the wait period is over.
        await Task.Delay(duration, cancellationToken).ConfigureAwait(false);
    }
    finally
    {
        midiOut.Send(MidiMessage.StopNote(note, volume, channel).RawData);
    }
}

Finally, I think this is a suitable candidate for being an extension method.

StackExchange Code Review Q#91511, answer score: 4