[Kde-games-devel] KTron's review

[Stas Verberkt]

On dinsdag 10 maart 2009 00:02:57 Albert Astals Cid wrote:
> A Dimarts, 24 de febrer de 2009, Legolas va escriure:
> > Hello games-list,
> >
> > Has been in review for two weeks now. That said, how do you feel about
> > moving KTron/KSnake to the KDEGames module wednesday/thursday?
>
> My comments:
>  * ChangeLog file seems like it could be killed

Probably all text files (README, TODO, ChangeLog) could be killed. As they are 
outdated and I prefer using bugzilla for wishes etc and svn log as a 
changelog.

>  * Given that there are already two games using the fontutils method i
> invented for blinken i think we should think of moving it to libkdegames.

That'd be a good idea.

>  * Do we really have to mention KTron/KSnake in the Game Type? I think that
> they just add confusion to the user

How should it be? They are different game types.

>  * In single player mode i set my name as "foo" but when highscore shows it
> says "kdesvn" (my user name)

Just committed a fix.

>  * It would be nice if Right Player / KSnake label is dynamic, that is,
> says Right Player when on "KTron" modes and "Player Name" when on "KSnake"
> mode, also Left Player line should disappear on "KSnake" mode

Do you mean the text in the shortcuts dialog, the configuration dialog or both?
The configuration dialog has the texts in a .ui file, so I'm not sure how easy I 
can make it dynamic.

>  * It would be good if Right Player / Player Name was filled with
> KUser::FullName if exists or KUser::loginName if it does not

As above. I could also use the given player name, altough the distinction 
between left and right makes it easier to see which player has which controls.

>  * Also Left Player should have a default name (think something catchy!)

Hmm :) You could use things like the player name (in the configuration) or 
Opponent. But I do like the left / right distinction.

>  * As a coding practice, in KDE we usually prefix private class variables
> with m_ so they are easier to recognize when having a quick look at a
> random method.

I'll do a big update as soon as I find the time.

>  * namespace ObstacleType {
>         enum Type {
>                 Bush
>         };
> }
>  makes more sense inside as enum inside class Obstacle

Comitted a fix.

>
> Not sure how much of the code is inherited and how much is new to see how
> that is your "fault" or original's coder :D

I changed a lot of the code (moved things to seperate files, made it a bit 
OOP). So I guess most is my fault ;)

>
> Albert

Thanks for the comments. Could you give me a bit more help / feedback on the 
naming issues?
I'm not completely sure about how to handle them.

Regards,

Stas