# 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 NAudio.Midi;

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 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();
}
}
}

• 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? – user2136963 May 22 '15 at 18:14

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);
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
{
// cancellationToken is signalled before the wait period is over.
}
finally
{
midiOut.Send(MidiMessage.StopNote(note, volume, channel).RawData);
}
}


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

## 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?