Review Request: Add Category and NewGame classes

[Laszlo Papp]

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



player/lib/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101816/#comment3678>

    Mmm, do not be supprized about further merge conflicts since I will go through the codebase and fix these issues. It will hopefully not be a big conflict solving task, though. :)



player/lib/CMakeLists.txt
<http://git.reviewboard.kde.org/r/101816/#comment3665>

    I would call it creategamejob (or addgamejob). cpp according to the  class description. "New" is a weird term in general in case of adding/appending (at least I saw it nowhere yet I am happening to be). After reading the small description of the class and checking out the code, it is not a general NewGame manager, but just a creator job.
    
    It would take an effect on the moc include name, the header include name, classname, etc. everywhere it happends to be like that.



player/lib/categorylistjob.h
<http://git.reviewboard.kde.org/r/101816/#comment3676>

    fetchFinished ?



player/lib/categorylistjob.h
<http://git.reviewboard.kde.org/r/101816/#comment3677>

    fetchFailed ?



player/lib/categorylistjob.h
<http://git.reviewboard.kde.org/r/101816/#comment3664>

    You did not actually use the discussed private slot way for now



player/lib/categorylistjob.cpp
<http://git.reviewboard.kde.org/r/101816/#comment3675>

    I know you are using your private method properly and you initialize the provider properly in the public ctor, but some static code analyzer does not like it.
    
    I think it is no harm to initialize for null there even if it is not needed.



player/lib/categorylistjob.cpp
<http://git.reviewboard.kde.org/r/101816/#comment3674>

    fetchFinished ?



player/lib/categorylistjob.cpp
<http://git.reviewboard.kde.org/r/101816/#comment3673>

    fetchFailed ?



player/lib/newgamejob.h
<http://git.reviewboard.kde.org/r/101816/#comment3666>

    You do not use the discussed slot code here either.



player/lib/newgamejob.cpp
<http://git.reviewboard.kde.org/r/101816/#comment3668>

    same as above



player/lib/newgamejob.cpp
<http://git.reviewboard.kde.org/r/101816/#comment3672>

    I think it is not necccesary to use the "New" term in the method name.



player/lib/newgamejob.cpp
<http://git.reviewboard.kde.org/r/101816/#comment3669>

    add/createGameFinished ?



player/lib/newgamejob.cpp
<http://git.reviewboard.kde.org/r/101816/#comment3670>

    add/createGameFailed ?


Just a side note: Do not forget to put "REVIEW: 101816" into the commit, once it reaches that stage. :)

- Laszlo


On July 4, 2011, 7:31 a.m., Shantanu Tushar Jha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101816/
> -----------------------------------------------------------
> 
> (Updated July 4, 2011, 7:31 a.m.)
> 
> 
> Review request for Gluon.
> 
> 
> Summary
> -------
> 
> Category's job is to fetch list of categories from the server.
> NewGame takes a game name and a category, and then creates a new game on the server, returning the ID of newly created game.
> 
> 
> Diffs
> -----
> 
>   player/lib/CMakeLists.txt 53c0f0a 
>   player/lib/categorylistjob.h PRE-CREATION 
>   player/lib/categorylistjob.cpp PRE-CREATION 
>   player/lib/newgamejob.h PRE-CREATION 
>   player/lib/newgamejob.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/101816/diff
> 
> 
> Testing
> -------
> 
> Both classes function as expected when used.
> 
> 
> Thanks,
> 
> Shantanu Tushar
> 
>

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