Warped Audio Sample

Question

I was asked this question and got no feedback, so I'd like to ask you for any feedback on my solution, Thanks!

Think of an audio sample as a rubber-band that you want to pin to a musical time ruler. The pins are called Warp Markers. A Warp Marker locks a specific point in the sample (in sample time) to a specific place in a measure (in beat time). You can use any number of Warp Markers to create an arbitrary mapping of the sample’s inherent rhythm to a musical meter. For the sake of this task, please assume the following behavior:

1. An audio clip contains a reference to a sample and a collection of Warp Markers.
2. There is at least one Warp Marker in the clip.
3. Between two Warp Markers, the tempo is constant.
4. The tempo before the first Warp Marker is the same as the tempo after the first Warp Marker.
5. The tempo after the last Warp Marker is specified separately in the input.
6. Beat time is measured in beats, sample time is measured in seconds, and tempo is measured in beats per second. Input description

There are 4 kinds of lines in the input:

1. Warp Marker definition
2. Definition of the tempo after the last marker
3. Sample time to beat time conversion
4. Beat time to sample time conversion

At least one Warp Marker and the tempo after the last Warp Marker should be defined before the first conversion; otherwise, these can appear in any order. Each line consists of a keyword followed by numeric arguments. All numeric values are entered as doubles without units. Warp Marker and tempo definitions affect only the conversions that come later in the input.

marker <beat time> <sample time>
end_tempo <value>
s2b <sample time>
b2s <beat time>

Output description For each of the s2b and b2s lines, the corresponding output time is printed without unit. Example

Input
marker 0.0 0.0
marker 1.0 5.0
end_tempo 10.0
b2s 0.5
s2b 6.0
Output
2.5 11.0

Here's my solution:

//one.h

#include <vector>
#include <string>
#include <map>

using namespace std;

#ifndef SAMPLER_H
#define SAMPLER_H

class Sampler {
    public:
        map<double, double> beats = {};
        map<double, double> samples = {};
        double endTempo;
        void addMarker(double beat, double sample);
        double b2s(double beat);
        double s2b(double sample);
};

#endif

and

// one.cc

#include "one.h"

using namespace std;

void Sampler::addMarker(double beat, double sample) {
    beats[beat] = sample;
    samples[sample] = beat;
}

double Sampler::b2s(double beat) {
    auto marker2 = beats.upper_bound(beat);

    if(marker2 == beats.begin()) marker2 = next(marker2);

    auto marker1 = prev(marker2);

    auto firstBeat = marker1->first;
    auto firstSample = marker1->second;

    double tempo;

    if(marker2 == beats.end()) {
        tempo = endTempo;
    } else {
        auto secondBeat = marker2->first;
        auto secondSample = marker2->second;

        tempo = (secondBeat - firstBeat) / (secondSample - firstSample);
    }

    return firstSample + ((beat - firstBeat) / tempo);
}

double Sampler::s2b(double sample) {
    auto marker2 = samples.upper_bound(sample);
    if(marker2 == samples.begin()) marker2 = next(marker2);
    auto marker1 = prev(marker2);

    auto firstBeat = marker1->second;
    auto firstSample = marker1->first;

    double tempo;

    if(marker2 == samples.end()) {
        tempo = endTempo;
    } else {
        auto secondBeat = marker2->second;
        auto secondSample = marker2->first;
        tempo = (secondBeat - firstBeat) / (secondSample - firstSample);
    }

    return firstBeat + ((sample - firstSample) * tempo);
}

and here's main

#include <iostream>
#include <vector>
#include <fstream>
#include <sstream>
#include "one.h"

int main() {
  Sampler sampler;
  string str;
  while(true) {
    getline(cin, str);
    istringstream ss(str);
    string command;
    getline(ss, command, ' ');

    if(command == "marker") {
        string strBeat;
        string strSample;
        getline(ss, strBeat, ' ');
        getline(ss, strSample, ' ');
        double beat = stod(strBeat);
        double sample = stod(strSample);
        sampler.addMarker(beat, sample);
    } else if(command == "end_tempo") {
        string strTempo;
        getline(ss, strTempo, ' ');
        sampler.endTempo = stod(strTempo);
    } else if(command == "s2b") {
        string s;
        getline(ss, s, ' ');
        cout << sampler.s2b(stod(s)) << endl;
    } else if(command == "b2s") {
        string b;
        getline(ss, b, ' ');
        cout << sampler.b2s(stod(b)) << endl;
    } else {
        break;
    }
  }
}

and heres my tests

#include "one.h"
#include "gtest/gtest.h"
namespace {

class OneMarker : public ::testing::Test {
protected:
     Sampler * sampler;
     virtual void SetUp() {      
         sampler = new Sampler();
         sampler->addMarker(3, 5);
         sampler->endTempo = 10;
     }

     virtual void TearDown() {
        delete sampler;
     }
};

TEST_F(OneMarker, Before) {
    EXPECT_EQ(4.6, sampler->b2s(-1));
    EXPECT_EQ(-57, sampler->s2b(-1));
}

TEST_F(OneMarker, After) {
    EXPECT_EQ(5.7, sampler->b2s(10));
    EXPECT_EQ(53, sampler->s2b(10));
}

class TwoMarkers : public ::testing::Test {
protected:
     Sampler * sampler;
     virtual void SetUp() {      
         sampler = new Sampler();
         sampler->addMarker(0, 0);
         sampler->addMarker(1, 5);
         sampler->endTempo = 10;
     }

     virtual void TearDown() {
        delete sampler;  
     }
};

TEST_F(TwoMarkers, Before) {
    EXPECT_EQ(-5.0, sampler->b2s(-1));
    EXPECT_EQ(-0.2, sampler->s2b(-1));
}

TEST_F(TwoMarkers, Between) {
    EXPECT_EQ(2.5, sampler->b2s(0.5));
    EXPECT_EQ(11.0, sampler->s2b(6.0));
}

TEST_F(TwoMarkers, After) {
    EXPECT_EQ(5.9, sampler->b2s(10));
    EXPECT_EQ(51, sampler->s2b(10));
}

TEST_F(TwoMarkers, Mutation) {
    EXPECT_EQ(5.9, sampler->b2s(10));
    EXPECT_EQ(51, sampler->s2b(10));
    sampler->addMarker(3,6);
    EXPECT_EQ(6.7, sampler->b2s(10));
    EXPECT_EQ(43, sampler->s2b(10));
}
}

Answer 1

Don't abuse using namespace std

Putting using namespace std at the top of every program is a bad habit that you'd do well to avoid. It's particularly bad to put that into a header file because it pollutes the global namespace.

Use appropriate #includes

The header file contains these two lines:

#include <vector>
#include <string>

However, neither is actually required for the interface. Also main does not need <fstream> but needs <string> but that is not included.

Use better naming

The file named one.h is not well named. The include guard name suggests that it is actually named sampler.h which is indeed a better name.

Prefer private to public where practical

The Sampler class has data members beats and samples as public members. Rather than do it that way, it would be better to keep it private because the class relies on an invariant -- namely that each entry in beats has a corresponding entry in samples. Such invariants cannot easily be enforced if the data members are public.

Use include guards correctly

The one.h file does have an include guard, which is good, but doesn't have it as the first thing in the file. This can waste preprocessing time as contrasted with the usual (and recommended) method of putting the include guard as the very first non-comment line in the header.

Use const where practical

The b2s and s2b functions don't alter the underlying object and so should be declared const.

Reconsider the structures

It's unclear to me what, if anything, you intend to do with the beats and samples stored in the Sampler class, but the current method of having parallel std::map structures seems less than optimal. If, as I infer, the context is music, both the sample and beat are increasing functions. This suggests that they could easily be contained in a single std::set<BeatTime> where BeatTime is a simple private struct containing both doubles.

With those changes, here's what the class declaration looks like:

class Sampler {
        struct BeatTime { 
            double beat, sample; 
            bool operator<(const BeatTime& rhs) const {
                return beat < rhs.beat;
            }
        };
        std::set<BeatTime> beats;
        std::set<BeatTime>::iterator sample_upper_bound(double sample) const;
    public:
        double endTempo;
        void addMarker(double beat, double sample);
        double b2s(double beat) const;
        double s2b(double sample) const;
};

Now the addMarker function is very simple:

void Sampler::addMarker(double beat, double sample) {
    beats.emplace(BeatTime{beat, sample});
}

And the b2s code is simplified:

double Sampler::b2s(double beat) const {
    auto marker2 = beats.upper_bound(BeatTime{beat, 0});
    double tempo{endTempo};
    if(marker2 == beats.begin()) {
        marker2 = next(marker2);
    }
    auto marker1 = prev(marker2);
    if(marker2 != beats.end()) {
        tempo = (marker2->beat - marker1->beat) / (marker2->sample - marker1->sample);
    }
    return marker1->sample + ((beat - marker1->beat) / tempo);
}

The s2b code is almost identical except that, of course, we can't use upper_bound directly. Instead, it uses this:

std::set<Sampler::BeatTime>::iterator Sampler::sample_upper_bound(double sample) const {
    auto ret{beats.begin()};
    for ( ; ret != beats.end() && ret->sample <= sample; ++ret) 
    {}   
    return ret;
}

This makes the search linear rather than logarithmic, but it's unlikely to make a huge practical difference until the set becomes very large.