[Kde-games-devel] kAtomic, Bug 154756, Patch

[Dmitry Suzdalev]

On Tuesday 08 April 2008 01:04:20 gnushi at web.de wrote:
> Hi Gamers!
>
> Well, I hope I'm now on the right list as kde-edu told me to write here.
> ;-)
Yep. You wrote to the right place ;)

> There were two issues with katomic, which I fixed yesterday:
Very nice!

> As this is my first patch to KDE, please feel free to tell me if I missed
> anything.
Yep, I have a couple of notes on board :)

1. You named actions "prevLevelAct", "nextLevelAct". In KAtomic the convention is to 
use "m_" prefix for member variables (you can tell that by looking at other vars in 
header ;)), so please stick to that. So these actions should be named m_prevLevelAct, 
m_nextLevelAct.
Reasons to name vars like that - it can easily be seen which var is member-var and 
which is local to some function.
The basic rule when hacking on KDE (as well as other OSS projects) is to use 
conventions that are already there :)

2. Same with "timer" -> should be m_timer.

3. When you do timer->stop() I think it is better to check if it's running. Like 
if( m_timer->isActive() )
   m_timer->stop()

4. I'd suggest updateActionsForLevel(int) instead of updateActions(int) :)

5. When I see this:
     connect(m_gameWid, SIGNAL(levelChanged(int)), SLOT(updateLevelName()));
     connect(m_gameWid, SIGNAL(levelChanged(int)), SLOT(updateActions(int)));

I'm starting to think: why not to unite these two slots into one? :)
For example you can make some "levelChanged(int)" slot, and in that slot call

updateLevelName();
updateActionsForLevel(level);

Not a big issue, but this way we'll have one place for reacting on level change, 
looks more convenient

6. This:
+  if (level == 1)
+      prevLevelAct->setDisabled(true);
+  else
+      prevLevelAct->setDisabled(false);

can become just: prevLevelAct->setDisabled( level == 1 );
as well as this:
+  if (level == maxLevel)
+      nextLevelAct->setDisabled(true);
+  else
+      nextLevelAct->setDisabled(false);

can become: nextLevelAct->setDisabled( level == maxLevel )

7. No need to #include <QTimer> in header - it is needed only in *.cpp
So move #include <QTimer> to gamewigdet.cpp, and insert a forward declaration in 
gamewiget.h. (you can see how it is already done there for KAtomicHighscores class)
That's a good habit to have: put only needed #includes in .h files, and forward 
declarations for others. This can save you many dependensies between files in future 
and therefore greatle reduce compilation time.

8. Instead of disconnecting signal in hack mode, i'd suggest to simply do

if( m_allowAnyLevelSwitch )
   return;

in updateActions(int) function.

Reason: if it happens once that code will be changed in a manner that it will do 
connect(..., levelChanged(int), ..., updateActions(int) ) *after* enableHackMode(), 
this disconnect will do nothing :) And if you insert a check above into 
updateActions() it will always work


I think that's all :)
If I'm wrong somewhere, feel free to tell me :)
If you've questions, i'd be willing to answer them.

Another thing: what do you think about introducing possibility for user to enable or 
disable automatic level switching? For example, we can introduce a menu entry in 
settings menu, like "Enable Auto Level Switch" and make it a checkable action.
I think it would be nice :) This is something to think after your patch gets applied.

Thanks for your work! :)

Cheers,
Dmitry.