Review Request: Achievements 6: AchievementsManager

[Dan Leinir Turthra Jensen]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/102034/#review4940
-----------------------------------------------------------


Other than that... ;)


engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/102034/#comment4308>

    You're not, as far as i can see, using savable anywhere in the header, so this should probably not be included.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/102034/#comment4309>

    Never delete a QObject like this, always use QObject::deleteLater() - avoids strange crashes later on.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/102034/#comment4310>

    yeah, alright, this one works, because you control the lifetime of the object so tightly ;)



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/102034/#comment4311>

    It is worth noting that operator[] (and .at()) are not bound checked. As such, you can cause a crash here from any game code, which we'd really want to avoid.
    
    Since this is as it is, it would likely make sense to check for index range explicitly and return a QString(), rather than switch to .value() here. That way we are being entirely explicit about what's going on.


- Dan Leinir Turthra


On July 21, 2011, 5:32 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102034/
> -----------------------------------------------------------
> 
> (Updated July 21, 2011, 5:32 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> This class saves the achievements information into a separate file, so that achievements can be shown without opening the whole project.
> 
> 
> Diffs
> -----
> 
>   engine/CMakeLists.txt 59acf45 
>   engine/achievementsmanager.h PRE-CREATION 
>   engine/achievementsmanager.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/102034/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Felix
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/gluon/attachments/20110721/d016db78/attachment.htm