6

In joint with this question. I am having trouble coming up with a good type safe solution to the following seemingly basic problem. I have a class music_playlist that has a list of the songs it should play. Seems pretty simple right, just make an std::list of all the songs in queue, and make it available to the user. However, out of necessity the audio decoding, and the audio rendering happens on separate threads. So the list needs to be mutex protected. Often times the mutex was forgotten by other programmers using my library. Which obviously led to "strange" problems.

So at first I just wrote setters for the class.

struct music{};
class music_playlist{
private:
     std::list<music> playlist;
public:
     void add_song(music &song){playlist.push_back(song);}
     void clear(){playlist.clear();}
     void set_list(std::list<music> &songs){playlist.assign(songs.begin(),songs.end());}
//etc
};

This led to user code like below...

music song1;
music song2;
music song3;
music song4;
music song5;
music song6;
music_playlist playlist1;
playlist1.add_song(song1);
playlist1.add_song(song2);
playlist1.add_song(song3);
playlist1.add_song(song4);
playlist1.add_song(song5);
playlist1.add_song(song6);

//or
music_playlist playlist2;
std::list<music> songs;
songs.push_back(song1);
songs.push_back(song2);
songs.push_back(song3);
songs.push_back(song3);
songs.push_back(song5);
songs.push_back(song6);
playlist2.set_list(songs);

While this works and is very explicit. It is very tedious to type out and it is bug prone due to all the cruft around the actual work. To demonstrate this, I actually intentionally put a bug into the above code, something like this would be easy to make and would probably go through code reviews untouched, while song4 never plays in playlist 2.

From there I went to look into variadic functions.

struct music{};
class music_playlist{
private:
     std::list<music> playlist;
public:
     void set_listA(music *first,...){
     //Not guaranteed to work, but usually does... bleh
         va_list Arguments;
    va_start(Arguments, first);
    if (first) {
        playlist.push_back(first);
    }
    while (first) {
        music * a = va_arg(Arguments, music*);
        if (a) {
            playlist.push_back(a);
        }else {
            break;
        }
    }
    }
    void set_listB(int count,music first,...){
         va_list Arguments;
     va_start(Arguments, first);
     playlist.push_back(first);

    while (--count) {
        music a = va_arg(Arguments, music);
            playlist.push_back(a);
    }
    }
//etc
}; 

Which would lead to a users code like below...

playlist1.set_listA(&song1,&song2,&song3,&song4,&song5,&song6,NULL);
//playlist1.set_listA(&song1,&song2,&song3,&song4,&song5,&song6); runtime error!!
//or
playlist2.set_listB(6,song1,song2,song3,song4,song5,song6);

Now its much easier to see if a song was added twice or not included. However in solution A it will crash if NULL is not at the end of the list, and is not cross platform. In solutionB you have to count the amount of arguments which can lead to several bugs. Also, none of the solutions are type safe, and the user could pass in an unrelated type and wait for the crash at runtime. This didn't seem to be a sustainable solution. So then I came across std::initializer_list. Can't use it several of the compilers I develop for don't have support for it yet. So I tried to emulate it. I ended up with this solution below.

Is this use of the "," operator considered bad form?

Which would lead to a users code looking like this...

struct music_playlist{
    list<music> queue;
//...
};
int main (int argc, const char * argv[])
{
    music_playlist playlist;
    music song1;
    music song2;
    music song3;
    music song4;
    playlist.queue = song1,song2; // The queue now contains song1 and song2
    playlist.queue+= song1,song3,song4; //The queue now contains two song1s and song2-4
    playlist.queue = song2; //the queue now only contains song2
    return 0;
}

This syntax was not confusing to our small testgroup. However, I had serious concerns with abusing operator overloading so severely. So I posted the question above. I wanted to see what programmers that were comparatively experts to our test group thought. Quite alot of programmers did not like it however it seemed better than the above solutions because it would catch most bugs at compile time instead of just at run time. However Tom posted an interesting counter example, that would cause unexpected behavior.

//lets combine differnt playlists
new_playlist.queue =    song1        //the first playlist
                      ,(song3,song4) //the second playlist //opps, I didn't add song 3!
                      , song5;

This sours me to that solution. So do you have any ideas about a better solution?

P.S. None of the above code was compiled, they are just there for example purposes.

Community
  • 1
  • 1
Skyler Saleh
  • 3,961
  • 1
  • 22
  • 35
  • 2
    Honestly, I don't see how single line adding of `song1,song2,song3,song4,song5,song6` is any more better to find errors than multiline? Why would people read over one and not the other? It seems to me if you have variables like that which are not in a list/array you're doing something wrong in the calling code, which is honestly not your problem to solve... – KillianDS Jul 18 '11 at 07:55
  • 1
    All the bugs in your example have nothing to do with concurrency, just poor programming. – littleadv Jul 18 '11 at 07:55
  • @littleadv Which is unfortunately something I have to deal with. As the target market of this library has such broad programming abilities. Some of them are experts in the field with 10+ years of experience while others are involved in writing their first C++ program. The entire design of the library tries to make as many bugs as possible result in compile time errors or be easily detectable, by a simple glance through the source files. This has been the only area, where I have not been able to apply this methodology. – Skyler Saleh Jul 18 '11 at 08:02
  • 1
    @RTS - just makes my point in the answer stronger: assume the customer is dumba** - make sure **your** code is foul-proof. If it requires protection - protect it **yourself**. As to adding the same song twice while missing the other - really not much you can do with that. Having the same song twice seems to me as a viable use-case, so you shouldn't dismiss it as mistake. – littleadv Jul 18 '11 at 08:06
  • 1
    I'm too tired to write a substantial answer so I'll just leave this in here.. instead of using the comma operator, you should really stick to the standard `<<` operator which has many precedents. – David Titarenco Jul 18 '11 at 08:06
  • @KillianDS Its just that you are more likely to notice that when it is not surrounded by a large block of generally irrelevant code. – Skyler Saleh Jul 18 '11 at 08:08
  • I'm with KillianDS: even in a perfect world (C++0x) where you have perfect type-safe varadic functions, how is the single line any less error prone than the longer list? – Nicol Bolas Jul 18 '11 at 08:11

1 Answers1

6

The first question that comes to mind is whether this is a problem, and whether this is a problem worth the solution. Since you are creating an interface, your client will be user code (not people, code). How many times will your clients hardcode playlists in the code versus say, store load them from a file or construct them from their user selections?

Think on the actual value of the feature and compare that with the impact that it will have in how they use your library, and think on how many problems your users might have with the changed interface.

The simplest solution is accepting iterators to construct/reset your list, and then let the users deal with the problem. Of course they can build their own container in the way that you have shown, but they can also use Boost.Assignment to take care of the boiler plate, so their user code will look like:

std::vector<music> songs = boost::assign::list_of()( song1 )( song2 );
play_list.set( songs.begin(), songs.end() );

Or if they are not comfortable with that library the can use a plain old array:

music songs[2] = { song1, song2 };
play_list:set( songs, songs + 2 ); // add your preferred magic to calculate the size
David Rodríguez - dribeas
  • 204,818
  • 23
  • 294
  • 489