Review Request: Statistics and Achievements, part 2

Laszlo Papp lpapp at kde.org
Sat Jul 9 08:05:56 CEST 2011 

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


"It's a bit long, but Arjen Hiemstra said this would be the best way." -> Why it is not good idea is that I have a very slow internet connection and every time I open this up, takes me ages to load. Second, it really contains more things that should be separated. For instance, I can take more serious technical reviews in my kdeext player and player lib part. Right now I cannot see just them, I need to filter manually which is a bigger overhead than it could be. Furthermore, I could not really take a quality review on this patch since I did not have enough time and I still spent more 1-2 hours with it. It would really help to have separate review requests in order to provide fast and quality feedbacks for snippets.

I would really suggest in the future not providing god/monolythic diffs for review. For me personally, it is really not nice and a lot of overhead meaning I can easily lose my sake for any review.

Next step: please upload the player things in a separate review and I can go through them. 


creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h
<http://git.reviewboard.kde.org/r/101887/#comment3882>

    public Q_SLOTS



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h
<http://git.reviewboard.kde.org/r/101887/#comment3883>

    Sounds like a signal name. *Done is more like a signal name. do* is more like a slot name. I am not sure when it cannot be applied, but could you try to rename according to this rule ? Maybe instead of writing what happened, telling what will take place ?



creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3884>

    I would personally use QStringList() << QString( "qlonglong" ); but no worries about this because the other files do it this over there.



creator/x-gluon-mimetypes.xml
<http://git.reviewboard.kde.org/r/101887/#comment3903>

    I would prefer alphabetical order according to the mime-type namesince that the main container there.



engine/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101887/#comment3885>

    achievementmanager ? We use plural nowhere in the project even though the managers deal with more things. It would be nice to not break up this consistency. 



engine/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101887/#comment3886>

    No plural, personally.



engine/abstractstatistic.h
<http://git.reviewboard.kde.org/r/101887/#comment3887>

    This breaks the order public/protected/private order. Could you please move back if it is a public slot ?



engine/achievement.h
<http://git.reviewboard.kde.org/r/101887/#comment3902>

    If you take a look at the other engine files, they contain the same idea this way:
    #include "gluon_engine_export.h" 
    
    The idea is simply that behind this: it is the same library, compilation/build process. <...> whispers me that it is from the system, maybe from some other engine, maybe not even game engine, if I try to think as a novice programmer.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3900>

    same as previously.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3888>

    I would personally put this before the member declarations (after the constructor in this specialcase).



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3889>

    No consistency with the rest of the code here. AchievementPrivate::parseSelection is enough without Achievement and it is even shorter. I would prefer that way.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3893>

    Why not ' '. Space is a simple character.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3891>

    Could you avoid the regexp here, please ? For me, it is unmaintainable if I need to touch it. I am not fine at regexpS and I do not use a special regexp on a daily bases. Not that I even want to. I saw your regexp above, but that was super simple. This one is something which would be really pain for me to modify and maintain. Real pain as in much more than needed.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3890>

    string.contains('-') ?



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3892>

    Same, slower than it could be without any gain. Moreover static code checkers, like krazy, will complain about it nicely.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3894>

    Do you need plural here ? If yes, could you use TaskListStatistic ? That is more talkative to me whether it is really intended that way or not. Also, we started using this schema in the player lib, so it would even be consistent with that.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3895>

    const method



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3896>

    const method, just returns something.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3897>

    accomplished what ? Task, Statistic, Achievement, something else ? Could you please give a more talkative name ? 
    Do not spare characters for the proper understanding and maintanibility.



engine/achievement.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3898>

    same as one line above



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3901>

    same as previously for both.



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3905>

    I would call it AchievementManager as previously told you the reason.



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3908>

    There is an interesting mixup here ongoing. Sometimes, you use achievement* for the getters, sometimes just *. I always prefer the long names, but I do not mind it either way, ie.: if you remove the achievement from everywhere here.
    
    Why I prefer the long name is simply that if you add a new getter with foobarName it will be more consistent to use:
    foo barName();
    QString archievementName();
    
    than foo barName();
    QString name();
    
    The class name also implies the name though. I am neutral about which way to keep, personally long name is preferred by me.



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3904>

    I would call it achievementCount since the count already implies, it can be more than just 1. 
    
    On the other hand, it is interesting you are using size in the qDebug statement for this. Actually it is now a bit confusing for me as a third-party person looking at the API (be it public or internal). It can mean the size of something or the counts of something. Please clean it up.



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3906>

    hasDependency as a common naming practice for such cases and also as the comment says ?



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3907>

    isHidden as a common naming schema in such cases and also as the comment says ?



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3909>

    There are two issues here:
    
    1) The usage of the "achieve" base word twice makes it weird for me. I would you passed or something like that.
    
    If you choose using these things without achievement since the class name implies it, it goes away though, like hasAchieved() then only.
    
    2) I would prefer the usage of "has*" here as well as a standard and as the comment says.
    
    If I see well, the achievement class uses only achieved, it is a manager which can do other things, boiler place things as well etc.



engine/achievementsmanager.h
<http://git.reviewboard.kde.org/r/101887/#comment3910>

    No plural in my opinion.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3911>

    no plural imo. Please apply it in each case at the methods.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3912>

    I would separate it from the core lines with one empty line. In addition, same as above, just use "..." instead of <...>.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3915>

    Uhh, I do not like it, neither does Krazy and other checkers.
    
    For starter, you do not need to deparent QObjectS since that is done by default in the destructor. 
    
    Second, you can use qDeleteAll from the QtAlgorithms. 



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3914>

    What kind of object list ? Could the variable name be more talkative ?



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3913>

    Since it is a simple getter, you might want to use .at(0) instead of [0]



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3916>

    I would prefer achievementList



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3917>

    I would prefer achievementList



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3919>

    Same as above, not nice not using constref in foreach loops, however there is no even need for external loop here. 
    
    Just use qDeleteAll usage from QtAlgorithms. Again, you do not need to deparent a QObject in any case done by the destructor.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3918>

    I would prefer achievementList



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3920>

    NACK!
    
    Use QMutableListIterator JAVA-like iterator or STL style QList<QObject>::iterator if you would like to modify the value of the elements of the list.
    
    I personally prefer the JAVA style iterator.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3921>

    QMutableListIterator.



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3922>

    QMutableListIterator



engine/achievementsmanager.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3923>

    You are not consistent here: using *property( ".." ) and *property(".."). 
    
    Please use one them only. 



engine/assets/other/achievements/achievementsasset.h
<http://git.reviewboard.kde.org/r/101887/#comment3945>

    "..." (<BR> after the last engine entry)



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3946>

    "..." and empty line.



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3947>

    , d( new AchievementsAssetPrivate() )



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3948>

    Mutable iterator.



engine/assets/other/achievements/achievementsasset.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3949>

    mutable iterator



engine/assets/other/statistics/statisticsasset.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3950>

    "..." and line break.



engine/booleanstatistic.h
<http://git.reviewboard.kde.org/r/101887/#comment3924>

    const



engine/booleanstatistic.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3925>

    const



engine/game.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3926>

    achievementmanager



engine/gameproject.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3927>

    "...achievement.h" and also for the others. Please do not put empty line after the "game.h" include, but put after the last engine related include entry.



engine/gameproject.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3928>

    Sentences, expressions start with capital letter in English.
    
    Also it is mixed here:
    1) //Foobar
    2) // Foobar
    
    Please match them



engine/gameproject.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3929>

    As I previously told you, I do not think it is a good idea. It should be find* instead of search*. Moreover, it should not have the list as an arguement, but return it.
    
    



engine/gameprojectprivate.h
<http://git.reviewboard.kde.org/r/101887/#comment3930>

    find* term is used in Gluon for such situations, also consider the the findChildren method of the QObject class.



engine/gameprojectprivate.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3931>

    Same issue, no empty space before please. Use "achievement.h" simply and put an empty line after that to separate the "local" and "global" includes. 



engine/multiscorestatistic.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3933>

    You can just use if( d->maximumScoreCount )
    
    Count is talkative enough what kind of data type it is.



engine/multiscorestatistic.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3932>

    Why not .at(0) instead of [0] if it is a simple getter ?



engine/projectmetadata.h
<http://git.reviewboard.kde.org/r/101887/#comment3934>

    "..."



engine/projectmetadata.h
<http://git.reviewboard.kde.org/r/101887/#comment3935>

    empty line after this line please



engine/projectmetadata.h
<http://git.reviewboard.kde.org/r/101887/#comment3936>

    Interesting mixup. Sometimes with project/Project, sometimes without it. Could you please polish it ?



engine/projectmetadata.h
<http://git.reviewboard.kde.org/r/101887/#comment3940>

    why not projectName arg ?



engine/projectmetadata.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3938>

    mixup here for me as well



engine/projectmetadata.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3939>

    mixup here for me as well



engine/projectmetadata.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3937>

    mixup here for me as well



engine/projectmetadata.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3941>

    why not project name arg ?



engine/projectmetadata.cpp
<http://git.reviewboard.kde.org/r/101887/#comment3942>

    Yes, that is what I told you previously.



engine/tasksstatistic.h
<http://git.reviewboard.kde.org/r/101887/#comment3943>

    set/markAccomplished ?



engine/tasksstatistic.h
<http://git.reviewboard.kde.org/r/101887/#comment3944>

    set/markNotAccomplished accordingly, but the "not" sounds weird. I will think about it later if I find the time


- Laszlo


On July 8, 2011, 11:17 p.m., Felix Rohrbach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101887/
> -----------------------------------------------------------
> 
> (Updated July 8, 2011, 11:17 p.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> This is the second part of my SoK project, bringing statistics and achievements to gluon. While the first part included mainly the statistics and the databases to save them, this part includes the achievements and a way to show them in gluon player applications (atm there's only an implementation for the KDE Extended player), but also brings improvements to the statistics. It's a bit long, but Arjen Hiemstra said this would be the best way. And don't forget the second page of the diff ;)
> 
> 
> Diffs
> -----
> 
>   creator/plugins/docks/propertiesdock/CMakeLists.txt 05c5242 
>   creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.h PRE-CREATION 
>   creator/plugins/docks/propertiesdock/propertywidgetitems/longlongpropertywidgetitem.cpp PRE-CREATION 
>   creator/x-gluon-mimetypes.xml 378876d 
>   engine/CMakeLists.txt e8764f5 
>   engine/abstractstatistic.h 494032f 
>   engine/achievement.h PRE-CREATION 
>   engine/achievement.cpp PRE-CREATION 
>   engine/achievementsmanager.h PRE-CREATION 
>   engine/achievementsmanager.cpp PRE-CREATION 
>   engine/assets/CMakeLists.txt b8c087b 
>   engine/assets/other/achievements/CMakeLists.txt PRE-CREATION 
>   engine/assets/other/achievements/achievementsasset.h PRE-CREATION 
>   engine/assets/other/achievements/achievementsasset.cpp PRE-CREATION 
>   engine/assets/other/statistics/statisticsasset.h a1d1995 
>   engine/assets/other/statistics/statisticsasset.cpp a8d6372 
>   engine/booleanstatistic.h PRE-CREATION 
>   engine/booleanstatistic.cpp PRE-CREATION 
>   engine/game.cpp d5fc279 
>   engine/gameproject.h 805d193 
>   engine/gameproject.cpp 8d84024 
>   engine/gameprojectprivate.h 38c442a 
>   engine/gameprojectprivate.cpp 08424b1 
>   engine/gluon_engine_export.h 17fc00c 
>   engine/multiscorestatistic.h PRE-CREATION 
>   engine/multiscorestatistic.cpp PRE-CREATION 
>   engine/projectmetadata.h PRE-CREATION 
>   engine/projectmetadata.cpp PRE-CREATION 
>   engine/statistic.h e29b2fe 
>   engine/tasksstatistic.h PRE-CREATION 
>   engine/tasksstatistic.cpp PRE-CREATION 
>   player/examples/apocalypse.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/ball.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/invaders.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/jumpnbump.gluon/game.gluonmeta PRE-CREATION 
>   player/examples/pong.gluon/game.gluonmeta PRE-CREATION 
>   player/kdeext/CMakeLists.txt 40da561 
>   player/kdeext/delegates/achievementdelegate.h PRE-CREATION 
>   player/kdeext/delegates/achievementdelegate.cpp PRE-CREATION 
>   player/kdeext/gamedetailsoverlay.h 99923a2 
>   player/kdeext/gamedetailsoverlay.cpp b6f525c 
>   player/kdeext/gamesoverlay.cpp d468186 
>   player/lib/CMakeLists.txt 5957748 
>   player/lib/models/achievementsmodel.h PRE-CREATION 
>   player/lib/models/achievementsmodel.cpp PRE-CREATION 
>   player/lib/models/commentsmodel.cpp 5a0bff0 
>   player/lib/models/gameitemsmodel.h c907f7b 
>   player/lib/models/gameitemsmodel.cpp 82f7202 
>   player/lib/models/gameviewitem.h 6a7f4ca 
>   player/lib/models/gameviewitem.cpp 67f0566 
> 
> Diff: http://git.reviewboard.kde.org/r/101887/diff
> 
> 
> Testing
> -------
> 
> It compiles and I tried to test every feature I implemented, but I didn't always tried every combination, so there might be still some bugs.
> 
> 
> Thanks,
> 
> Felix
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/gluon/attachments/20110709/9cc2e2b6/attachment-0001.htm

More information about the Gluon mailing list