Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: a simple Jukebox class posted on the wiki - feedback request  (Read 5646 times)

0 Members and 1 Guest are viewing this topic.

Jesper Juhl

  • Hero Member
  • *****
  • Posts: 1405
    • View Profile
    • Email
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  :)
« Last Edit: May 23, 2014, 06:19:14 pm by Jesper Juhl »

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6267
  • Thor Developer
    • View Profile
    • Bromeon
Re: a simple Jukebox class posted on the wiki - feedback request
« Reply #1 on: May 23, 2014, 09:06:22 pm »
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?
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

FRex

  • Hero Member
  • *****
  • Posts: 1839
    • View Profile
    • My GitHub Page
    • Email
Re: a simple Jukebox class posted on the wiki - feedback request
« Reply #2 on: May 23, 2014, 09:32:05 pm »
Quote
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
ZipSavings, script to count rar/7z/zip savings: https://goo.gl/vvBj5M
LuaConsole: https://goo.gl/X4kRUk
FoxRaycaster: https://goo.gl/27nVS8
Small Games - Heart, Routing and Snek: https://goo.gl/15ZGWE https://goo.gl/k5gwWD https://goo.gl/4nKPnT
Botes - a notes app in Pascal: https://goo.gl/bzTqsi

Jesper Juhl

  • Hero Member
  • *****
  • Posts: 1405
    • View Profile
    • Email
Re: a simple Jukebox class posted on the wiki - feedback request
« Reply #3 on: May 26, 2014, 11:33:19 pm »
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.
« Last Edit: May 26, 2014, 11:41:25 pm by Jesper Juhl »

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6267
  • Thor Developer
    • View Profile
    • Bromeon
Re: a simple Jukebox class posted on the wiki - feedback request
« Reply #4 on: May 27, 2014, 12:29:05 pm »
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);
        }
}
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

Jesper Juhl

  • Hero Member
  • *****
  • Posts: 1405
    • View Profile
    • Email
Re: a simple Jukebox class posted on the wiki - feedback request
« Reply #5 on: June 07, 2014, 10:42:24 pm »
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.

 

anything