Review Request: Achievements 11: Dependencies

Laszlo Papp lpapp at kde.org
Thu Sep 15 11:49:45 UTC 2011


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


I do not have technically anything obvious against this patch, I think it is fine for a quick glance. My comment is mostly about that how to improve the documentation writing (not mandatory, you could push without that since it works if there are no other obligations to the patch. Nice documentation would just be ideal :).


engine/achievement.h
<http://git.reviewboard.kde.org/r/102612/#comment5778>

    Small method documentation ? I guess simple getter/setter will not change too much, nor is an overhead to put something small therein. :p



engine/achievement.h
<http://git.reviewboard.kde.org/r/102612/#comment5777>

    Here is an example for a good documentation in my opinion:
    
                /** 
                 * Take a name in the format accepted by the GluonObject name property, and transform
                 * it into the format accepted by the objectName property. This function is useful for
                 * when you wish to search the hierarchy for objects by name.
                 *
                 * @param   name    The GluonObject name you wish transformed
                 * @return  The sanitized name
                 * @see name
                 */
    
    1) Descriptions - short description without question mark. :)
    2) Paramater (in more lines in case of more parameters if needed)
    3) Return - return value accordingly
    4) See (this can for instance be a getter for a setter, or vica versa and the like)
    
    I guess the "Reimplemented from ..." parts are exceptions of it, though.
    
    Could you please apply these rules on this patch for starter (at least where we know it is going to change a lot)



engine/achievement.h
<http://git.reviewboard.kde.org/r/102612/#comment5780>

    Probably description (without a question mark) and return line. Maybe hasDependency into the "See..." line and then vica versa ( that is optional I guess :) ?



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/102612/#comment5779>

    Probably descript/arguement/return lines



player/lib/models/achievementsmodel.cpp
<http://git.reviewboard.kde.org/r/102612/#comment5781>

    I would personally split this long line into 2 lines starting the second line with the "&&" part.


- Laszlo


On Sept. 14, 2011, 12:28 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102612/
> -----------------------------------------------------------
> 
> (Updated Sept. 14, 2011, 12:28 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> Add support for a dependency for achievements.
> 
> 
> Diffs
> -----
> 
>   engine/achievement.h 552ad01 
>   engine/achievement.cpp 0d3984a 
>   engine/achievementsmanager.h 5c7c93d 
>   engine/achievementsmanager.cpp 0cdc513 
>   player/lib/models/achievementsmodel.cpp fe1065c 
> 
> Diff: http://git.reviewboard.kde.org/r/102612/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Felix
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/gluon/attachments/20110915/a195c12b/attachment-0001.html>


More information about the Gluon mailing list