HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Beat detection algorithm implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
beatalgorithmimplementationdetection

Problem

What is the quality of the code I've written? Is it easy to read or is it a low-quality piece of code? Also, is there any way I can improve the algorithm itself (beside changing C parameters)?

```
using System;
using System.Threading;
using Un4seen.Bass;
using Un4seen.BassWasapi;

namespace Szab.BeatDetector
{
public enum SensivityLevel
{
VERY_LOW = 120,
LOW = 110,
NORMAL = 100,
HIGH = 80,
VERY_HIGH = 70
};

public sealed class SpectrumBeatDetector
{
#region Fields

// Constants
private const int BANDS = 10;
private const int HISTORY = 50;

// Events
public delegate void BeatDetectedHandler(byte Value);
private event BeatDetectedHandler OnDetected;

// Threading
private Thread _AnalysisThread;

// BASS Process
private WASAPIPROC _WasapiProcess = new WASAPIPROC(SpectrumBeatDetector.Process);

// Analysis settings
private int _SamplingRate;
private int _DeviceCode;
private SensivityLevel _BASSSensivity;
private SensivityLevel _MIDSSensivity;

// Analysis data
private float[] _FFTData = new float[4096];
private double[,] _History = new double[BANDS, HISTORY];

#endregion

#region Setup methods

public SpectrumBeatDetector(int DeviceCode, int SamplingRate = 44100, SensivityLevel BASSSensivity = SensivityLevel.NORMAL, SensivityLevel MIDSSensivity = SensivityLevel.NORMAL)
{
_SamplingRate = SamplingRate;
_BASSSensivity = BASSSensivity;
_MIDSSensivity = MIDSSensivity;
_DeviceCode = DeviceCode;
Init();
}

// BASS initialization method
private void Init()
{
bool result = false;

// Initialize BASS on default device
result = Bass.BASS_Init(0, _SamplingRate, BASSInit.BASS_DEVICE_DEFAULT, IntPtr.Zero);

if (!result)
{
throw new BassInitException(Bass.BASS_ErrorGetCode().ToString());
}

// Initialize WASAPI
result = BassWasapi.BASS_WASAPI_

Solution

Naming

Based on the naming guidelines input parameters should be named using camelCase casing.

Although it isn't mentioned in the guidelines, you should consider to use the same casing for variables which are local to a method.

You have a lot of magic numbers in your code which should be extracted to well named constants. In this way Mr.Maintainer wouldn't have to think to much about them.

If you need a comment to know what a variable is about, then this variable is poorly named.

General

-
Comments should describe why something is done. Let the code itself tell what is done by using meaningful names.

-
commented code is dead code which should be removed

-
leave your variables some space to breathe. if(i

DetectBeat()

You don't need to assign
0 to the C variable, because an assignment changing this value is happening anyhow.

If the condition
if(Energies[i] > (C * avg[i]) && volumelevel > 1) evaluates to true, you can break out of the for loop instead of again checking the for conditions.

By extracting the calculating of the
C parameter into a separate method, the DetectBeat() method will be reduced to its core responsibility.

private const int _Bass = 3;
private const int _Mids = 6;
private double CalculateCParameter(int band)
{
    if (band < _Bass)
    {
        return C = 2.3 * ((double)_BASSSensivity / 100);
    }
    if (band < _Mids)
    {
        return 2.89 * ((double)_MIDSSensivity / 100);
    }
    return C = 3 * ((double)_MIDSSensivity / 100);
}


And by extracting the calculation of the result to a separate method

private byte CalculateResultByBand(int band)
{
    if(band < _Bass)
    {
        return 1;
    }
    if (i < _Mids)
    {
        return 2;
    }
    return 4;
}


the
DetectBeat() method can, after adding a guard clause for volumelevel <= 1 be simplified to

private byte DetectBeat(double[] Energies, int volume)
{

    double[] avg = CalculateAverages();
    byte result = 0;
    double volumelevel = (double)volume / 32768 * 100;   // Volume level in %

    if (volumelevel  (C * avg[i]))   
        {
            result = CalculateResultByBand(i);
            break;
        }
    }

    if(result > 0)
    {
        OnDetected(result);
    }

    return result;
}


Free() versus
IDisposable

Because you are having unmanaged resources you really should implement
IDisposable` to free the unmanaged resources.

Read more about disposing finalizable types here : https://msdn.microsoft.com/en-us/library/b1yfkh5e%28v=vs.110%29.aspx (scroll down)

Code Snippets

private const int _Bass = 3;
private const int _Mids = 6;
private double CalculateCParameter(int band)
{
    if (band < _Bass)
    {
        return C = 2.3 * ((double)_BASSSensivity / 100);
    }
    if (band < _Mids)
    {
        return 2.89 * ((double)_MIDSSensivity / 100);
    }
    return C = 3 * ((double)_MIDSSensivity / 100);
}
private byte CalculateResultByBand(int band)
{
    if(band < _Bass)
    {
        return 1;
    }
    if (i < _Mids)
    {
        return 2;
    }
    return 4;
}
private byte DetectBeat(double[] Energies, int volume)
{

    double[] avg = CalculateAverages();
    byte result = 0;
    double volumelevel = (double)volume / 32768 * 100;   // Volume level in %

    if (volumelevel <= 1) { return result;}

    for (int i = 0; i < BANDS && result == 0; i++)                    
    {
        // Set the C parameter
        double C = CalculateCParameter(i);
        // Compare energies in all bands with C*average
        if(Energies[i] > (C * avg[i]))   
        {
            result = CalculateResultByBand(i);
            break;
        }
    }

    if(result > 0)
    {
        OnDetected(result);
    }

    return result;
}

Context

StackExchange Code Review Q#78589, answer score: 3

Revisions (0)

No revisions yet.