a simple Jukebox class posted on the wiki - feedback request

[Jesper Juhl]

Hi everyone

I recently posted a small Jukebox class to the wiki ( https://github.com/SFML/SFML/wiki/Source%3A-Jukebox ).
It works fine for my own purposes and I hope it can be a good example for others. But I'm curious as to what other people think about it and would apreciate some feedback.
So, what do you think? Do you like it? Do you hate it? Are there bugs I've missed?  Any and all criticism, comment, feedback welcome 

[Nexus]

Hey, looks like a very useful tool

The code looks really good: RAII, const correctness, expressive and well-separated functions, good documentation. So I've only found minor things

• I personally wouldn't use map::emplace if I didn't exploit its semantics; map::insert should do the job. When you emplace the object, you can omit std::make_pair.
• The Jukebox constructor should be explicit
• I personally find n == 0 more expressive than !n
• skip(0) stops the sound without replaying it, is that intended?
• std::size_t is fine instead of std::deque<std::pair<std::string, sf::Music *>>::size_type. These typedefs are only useful for custom allocators.
• Instead of std::advance(it, m_current) you could use std::next(it, m_current) which directly returns a new iterator that can be used in a temporary expression.
• Some code duplication in shuffle and shuffleRemaining could be avoided by a private method that shuffles an iterator range.
• For the methods that return sizes, I'd probably return unsigned int or std::size_t
• Your brace style is interesting (same line for blocks, new line for functions)
• You check a lot of preconditions in unrelated functions, e.g. the m_current index. Can't you assume it to be correct and only have assertions in the functions that actually change it?

[FRex]

Your brace style is interesting (same line for blocks, new line for functions)

This is a C++ style called Stroustrup style (which comes from C K&R style) as noted at the top of the header.
http://en.wikipedia.org/wiki/Indent_style#Variant:_Stroustrup
http://www.stroustrup.com/bs_faq2.html#layout-style

[Jesper Juhl]

Thank you for the kind words about the code Nexus.

Regarding your specific comments:

Yes, of course the constructor should be explicit, silly of me, thank you for pointing it out.

You are right that the std::make_pair is not needed when using emplace on a map - the whole point is perfect forwarding of arguments - fixed.

Regarding "n == 0" vs "!n" my oppinion is that it depends greatly on context. If I'm doing something like "if (foo == 0) {} else if (foo == 2) {} else if (foo == 4)" then I would never use "!n" for the first case as that would just confuse and make a mess of things. However, when I'm only doing a single test and that test is explicitly "is this thing zero or something that is not zero" then I find that (to me at least) "if (!n)" reads more easily as "if (not this thing)" and I find that more expressive - matter of taste I guess.

skip(0) was not intended to stop playback. It is supposed to just restart the current song. That's a bug and I've fixed it locally. Thank you for spotting that.

I agree that size_t is fine and using the vectors size_type doesn't buy me anything here. I've fixed that.

regarding std::advance vs std::next; you are right, I could use std::next in some places where I currently use std::advance. But, I'm pretty sure the compiler will generate the same code for something like

auto it = begin(m_playlist);
std::advance(it, m_current);
m_playlist.erase(it);

as for

m_playlist.erase(std::next(begin(m_playlist), m_current));

and I just happen to find the first form more readable (which I value more than compactness of expression; all else being equal).

There's some code duplication in the shuffle functions - nice catch, I hadn't noticed that when I wrote it. I'll have to fix that. Thanks.

About your "returning sizes" comment. I very much prefer to use "int" everywhere unless there's a very specific reason not to (integer arithmetic is so much easier when operating in plains signed int's - less need to remember all the integer promotion rules, sign extension rules, signed/unsigned conversion rules, fewer casts etc). But, in this case I believe you have a point with size_t since that type is specifically declared by the standard to be a type capable of holding the size of the largest possible object (not necessarily the largest possible integer though) and what I'm dealing with is the size of a container (an object) so size_t is the natural type to use. I'll fix that.

My brace style is heavily influenced by the style used in the Linux kernel, which I've had my head deeply embedded in for close to 20 years by now. It's also the style used by Bjarne Stroustrup in his books as well as the style used by my current employer in their codebase. That combined influence has caused me to adapt it for my own code as well and I kind of like it. I like the fact that the opening brace of a function does not clutter up the line containing the function signature and it also makes individual functions a bit more visually separated. Like it or not, it's a matter of personal taste

Regarding assert's. I could stick to just checking function pre/post conditions in the functions that modify them. But I've been bitten too many times (often by myself) when code has been refactored and suddenly something which should always hold in a function no longer does so. So I've become rather pedantic when it comes to assertions; I'll usually check whatever must be true for the world to be sane; especially since it won't cost anything in a release build but it has often saved my ass during development/maintenance over the years. Basically my rule is "if something cannot be or must be, then assert that it is so".

Thanks a lot for your review. I've updated parts of the code already (locally) and will update the remaining bits over the next couple of days. Then I'll update the code on the wiki, once I've tested it properly.

[Nexus]

Concerning integral types: yes, the status quo of C++ is quite a pain, even the standard is not very consistent. I'd probably just try to be consistent and use as few different types as possible (e.g. int and unsigned int). Typically, std::size_t is the least annoying choice when it's used for sizes or indices, since the STL uses it in many (but not all) places, thus the code leads to fewer conversion warnings.

About std::next(): yes, of course leave it if you find it more readable. It's sometimes handy in expressions, such as a nested loop with all possible pairs of elements:

for (auto first = c.begin(); first != c.end(); ++first)
{
        for (auto second = std::next(first); second != c.end(); ++second)
        {
                doSomething(*first, *second);
        }
}

[Jesper Juhl]

Ok, it took me a while to find the time to update the code, but I've done so now. The most recent version is now on the wiki 
Again, thanks for the feedback.