Review Request: Achievements 6: AchievementsManager

[Laszlo Papp]

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



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

    camelCase part in the file name ?



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

    1) qWarning() << "Invalid index:" << index; would be enough in my opinion.
    
    2) There is no documentation what the return value is for invalid index and what will happen. It should be documented either it is crash or QString().
    
    3) Just for book keeping: we were discussing this question on IRC last night.
    
    My first impression was that segmentation fault could be caused by Game Designers somehow, but I have been told this method is used from Player applications. 
    
    Hence I would refactor my opinion about ithis:
    
    If the API used according to the documentation, it makes it slower unneccesarily, mainly in a loop or more loops, embedded loops and so forth. Each API user would be implicitly forced for unneccesary checks even if they are sure what they are doing.
    
    If it is not used correctly, I prefer how QList and std::list works because I think a talkative segfault is easier to be caught than a silent return value. For a silent return value, you even need to do checks to detect the problem is there. Meanwhile it is stright-forward with a segmentation fault and backtrace usage (or valgrind on Linux). 
    
    For instance, I develop player applications where I know what index I pass. As for me the API would slow compared to that what it could be.
    
    Anybody having doubts about the index argument can make a check externally in his/her application.
    
    I guess those are the reasons behind the QList and std list API as well. That is just my two cents though. :)


- Laszlo


On July 24, 2011, 4:43 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102034/
> -----------------------------------------------------------
> 
> (Updated July 24, 2011, 4:43 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/20110725/4e4e32bc/attachment-0001.htm