5
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Also isn't it possible that midi_output becomes null right after checking if (midi_output != null) ut before locking? Or after checking if(!disposed) but before timer creation? \$\endgroup\$ – user2136963 May 22 '15 at 18:14
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

Braces:

Braces are always good to have, although they are not always necessary. In addition, at least one bug made it into production code because of missing braces (the Apple SSL bug).

Spaces:

Be consistent:

public Note(MidiOutWrapper midi_output,int number, int duration, int volume=127, int channel=0)

Timer t=null;
bool disposed = false;

var:

Instead of specifying each type, you can use var:

var disposed = false;

You cannot do var t = null; though, because it doesn't know how much space to give t for when it is assigned.

Naming:

What does t mean? From the context, it is clear it means Timer, but why not name it noteTimer to clearly specify what it is timing.

Typically you use PascalCase for public field/property names, and camelCase for private field/property and variable names:

public bool disposed { get; private set; }

Disposing:

disposed = true;
Reset();
Close();

What happens when Reset() or Close() throws an error and the value isn't really disposed, but disposed is still set to true? What should happen here?

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.