KGameDifficulty API notes and suggestions

Hi, fellows!

Nicolas, today I decided to look at KGameDifficulty class
you've
written to see if I can use it in games i maintain.

I have a few suggestions/observations. Perhaps you'll find
them useful


1. First of all I think that taking KXmlGuiWindow* as a
parameter
means hardcoding. I.e. there's no way for user of
KGameDifficulty to
place actions where he/she wants - this is hardcoded in
KGameDifficulty. And I think there will be games that would
want some
custom setup. OTOH, you may think like "hardcoded
place"="standard
look". Perhaps this argument stands. But I personally
prefer more
freedom in where to put actions in my game . So I'd
propose to
somehow redesign the class so it a) doesn't depend on
KXmlGuiWindow
argument b) returns {Q|K}Action which can be plugged where
the game
writer wants. Or maybe this should become
KStandardGameAction? Opinions?

2. Constructor which accepts number of difficulty levels
seems to have
some magic hidden behind it (and it has it indeed ), so it
isn't
understandable from the *.h file how it'll behave. Suppose I
have no access
to .cpp file - here are the questions, immediately arising
in my head:
   - how actions will be named if I pass,say, "3"?
"1","2","3"?
 Something meaningful
("easy","normal","hard")? Then
how big is the
 number i can pass to continue to get meaningful names? What
happens
 if I pass 5 or 6 or 7 as number of difficulty levels? How
about 10? API
 docs don't reflect that  And this
constuctor looks unnatural
 because of that. I would get rid of it completely in favour
of
 QStringList one .

3. No need to specify "const" for bool,int (and
any other non-reference
or built in type) function parameters - they're copied
anyway, so
original value can not change 

4. Function name "createActionsAndMore()" lives
some mistery behind
its name for developer that will work on the code you've
written
.
Perhaps it is better to replace "AndMore" with
what this "More"
means?  Or just
remove it, leaving name like "createActions()".
All
in all, reader doesn't know what AndMore means - so he might
as well
not know about it at all 

5. Is it really needed to put KDEGAMES_EXPORT before each
method
declaration? I really don't know this part, not very
experienced in
lib exports area . If this
is needed, than I should add this to
KGamePopupItem public methods too, right?

I hope you'll find this notes useful.
I can help you with this class as this is something really
common for
many games - well spotted! 
(but then I'd like to remove tab indentation from it - do
you mind this? ;))

What do you, others think about above notes?

Cheers,
Dmitry.
_______________________________________________
kde-games-devel mailing list
kde-games-develkde.org
https://mail.kde.org/mailman/listinfo/kde-games-devel

Hi Dmitry,


Am Dienstag, 10. Juli 2007 15:37:48 schrieben Sie:
> Nicolas, today I decided to look at KGameDifficulty
class 

First of all: Thanks for your feedback!


> 1. First of all I think that taking KXmlGuiWindow* as a
parameter
> means hardcoding. I.e. there's no way for user of
KGameDifficulty to
> place actions where he/she wants - this is hardcoded
in
> KGameDifficulty. And I think there will be games that
would want some
> custom setup. OTOH, you may think like "hardcoded
place"="standard
> look". 

Yes, that's exactly the point. 
And also: I don't see yet a need to have to possibility to
use the 
KGameDifficulty actions at a place not depending on the main
window... Do you 
have ideas what game could need that and why?
If there is a real need, we could add another constructor.
But I would 
reluctantly remove the actual constuctor(s) because I think
it's realy nice 
just to write (for instance) the following 2 lines of code
to define the 
difficulty levels:
	m_difficulty = new KGameDifficulty(this, false, 4);
	connect(m_difficulty, SIGNAL(levelChanged(int)), this, 
SLOT(levelChanged(int)));

Once again: if you need more freedom, we can extend the
API.


> Perhaps this argument stands. But I personally prefer
more  
> freedom in where to put actions in my game . So I'd
propose to
> somehow redesign the class so it a) doesn't depend on
KXmlGuiWindow
> argument b) returns {Q|K}Action which can be plugged
where the game
> writer wants. Or maybe this should become
KStandardGameAction? 

I thought about that also. Something like this
	m_difficulty = KStandardGameAction::difficulty(this, ...,
..., ...);

And this would be in fact *much better* as this class should
be a singleton, 
when I think about that... -> So I'll update the code
with a real singleton 
pattern.  (I missed
this by coding but now it's clear to me).


> Opinions? 

Other opinions?


> 2. Constructor which accepts number of difficulty
levels seems to have
> some magic hidden behind it (and it has it indeed ), so it
isn't
> understandable from the *.h file how it'll behave.
Suppose I have no access
> to .cpp file - here are the questions, immediately
arising in my head:
>    - how actions will be named if I pass,say,
"3"? "1","2","3"?
>  Something meaningful
("easy","normal","hard")? Then
how big is the
>  number i can pass to continue to get meaningful names?
What happens
>  if I pass 5 or 6 or 7 as number of difficulty levels?
How about 10? API
>  docs don't reflect that  

OK, this a lack of documentation in the H file. You're
right. I want to 
improve this. It should also be possible to define more than
9 levels (with 
no limits).


>  And this constuctor looks unnatural 
>  because of that. I would get rid of it completely in
favour of
>  QStringList one .

But the nice think about it, is that the appellations are
*standard* for all 
games using them. (And translators just need to translate
them once). And 
often (80% of the cases?), you just need standard
appellations 
like "Easy", "Medium", "Hard",
...
And if you wants to use other appellations, it's possible
anyway with the 
custom constructor.


> 3. No need to specify "const" for bool,int
(and any other non-reference
> or built in type) function parameters - they're copied
anyway, so
> original value can not change 

Thanks for this useful comment.  I'll clean
up the code and pay attention at 
this in the future.


> 4. Function name "createActionsAndMore()"
lives some mistery behind
> its name for developer that will work on the code
you've written
>
> .
Perhaps it is better to replace "AndMore" with
what this "More"
>
> means?  Or just
remove it, leaving name like "createActions()".
All
> in all, reader doesn't know what AndMore means - so he
might as well
> not know about it at all 

OK, I know what you mean. In fact, it's the common part of
the constructors... 
I'll try to rename it. (It's a private class anyway, but
you're right, the 
name of it should be comprehensible.)


> 5. Is it really needed to put KDEGAMES_EXPORT before
each method
> declaration? I really don't know this part, not very
experienced in
> lib exports area . If this
is needed, than I should add this to
> KGamePopupItem public methods too, right?

No idea. I saw this in other lib classes so I added this
here too, but I have 
no idea what for exactly. I'm not experienced (yet) with
libraries... Does 
anybody on the list can help us? 


> I hope you'll find this notes useful.

Yes, they are. Thanks a lot.


> I can help you with this class as this is something
really common for
> many games - well spotted! 

It was also spotted by Johann.  The idea
behind that was to standardize this 
instead of reimplementing it in every game, after having
seen a nice 
implementation in bovo.


> (but then I'd like to remove tab indentation from it -
do you mind this?
> ;))

I personally prefer tabs. (There are the well-known pro and
contra 
arguments...) That's why I used tabs in these new files. (In
existent files, 
I try to follow the indentation rules...)
But if you *really* want to remove them, I could survive...
I would be happy, if you contribute to the code, so I don't
want to bother 
you.

Thanks!

-- 
Nicolas

_______________________________________________
kde-games-devel mailing list
kde-games-develkde.org
https://mail.kde.org/mailman/listinfo/kde-games-devel

On Tuesday 10 July 2007 23:54, Nicolas Roffet wrote:
> First of all: Thanks for your feedback!
You're welcome 

> Yes, that's exactly the point. 
> And also: I don't see yet a need to have to possibility
to use the
> KGameDifficulty actions at a place not depending on the
main window... Do
> you have ideas what game could need that and why?
Ah, I missed that you can change insertion place through
mygameui.rc file. Ok 
then 

> If there is a real need, we could add another
constructor. But I would
> reluctantly remove the actual constuctor(s) because I
think it's realy nice
> just to write (for instance) the following 2 lines of
code to define the
> difficulty levels:
> 	m_difficulty = new KGameDifficulty(this, false, 4);
> 	connect(m_difficulty, SIGNAL(levelChanged(int)),
this,
> SLOT(levelChanged(int)));
Well, how useful to see 4 for the reader of the code? I got
your point, so 
perhaps we should replace that int by enum? Then it will be
clearer. For 
example something like this:

enum DifficultyLevelsCount
{
     ThreeDifficultyLevels, // easy, normal, hard
     FourDifficultyLevels, /// etc
     FiveDifficultyLevels
// etc until how many we support
}

Yes, that's basicly the same, and perhaps naming isn't that
great, but IMO it 
is much more clearer to read this:
new KGameDifficulty( this, false,
KGameDifficulty::ThreeDifficultyLevels);

Moreover, this way will put clear restriction on how much
predefined levels we 
support - so passing arbitrary int such as 99 won't work
(without brute force 
of spell^W casts, of course).
And such code is pretty self documenting 

I'd even go further (wrt qt lib guidelines), and replaced
bool in constructor 
with simple enum too:
enum { RestartOnChange, NoRestartOnChange } // naming from
head

Then
new KGameDifficulty( this,
                                 
KGameDifficulty::NoRestartOnChange,
                                 
KGameDifficulty::ThreeDifficultyLevels);

is even more understandable for code reader (while being
more lengthy, yes)

> I thought about that also. Something like this
> 	m_difficulty = KStandardGameAction::difficulty(this,
..., ..., ...);
>
> And this would be in fact *much better* as this class
should be a
> singleton, when I think about that... -> So I'll
update the code with a
> real singleton pattern.  (I missed
this by coding but now it's clear to
> me).
Perhaps you're right. But I see no problem with using
"new KGD" as well 

> But the nice think about it, is that the appellations
are *standard* for
> all games using them. (And translators just need to
translate them once).
> And often (80% of the cases?), you just need standard
appellations
> like "Easy", "Medium",
"Hard", ...
Yes, I agree here. But see above 

> Yes, they are. Thanks a lot.
You're welcome. That's great we can cooperate in such a nice
way to create 
something really useful by learning from eachother 

> I personally prefer tabs. (There are the well-known pro
and contra
> arguments...) That's why I used tabs in these new
files. (In existent
> files, I try to follow the indentation rules...)
Well, I think it should be discussed in another thread.
We briefly chatted with Mauricio about this issue on irc and
agreed that we 
should agree (heh) on at least:
- using tab vs spaces in kdegames
- level of indentation we should all use

This is for new code going into kdegames. Old code should be
adopded while(if) 
editing.

It seems natural to pick these items from kdelibs coding
guidelines.

But let's discuss that in another thread - i.e. if someone
will have comments 
on this, please create new thead.

> But if you *really* want to remove them, I could
survive...
Well, and I can survive with tabs if *you* really want them
. So I
guess we 
should just decide on some kdegames-wide policy.

> I would be happy, if you contribute to the code, so I
don't want to bother
> you.
But keep an eye on my (possible) commits in case something
might go wrong .

Cheers,
Dmitry.
_______________________________________________
kde-games-devel mailing list
kde-games-develkde.org
https://mail.kde.org/mailman/listinfo/kde-games-devel

Hi Dmitry,


Am Dienstag, 10. Juli 2007 23:36:41 schrieben Sie:
> Ah, I missed that you can change insertion place
through mygameui.rc file.
> Ok then 

Well. OK for now, but on the long-term, I think we should do
like for the 
KStandardGameAction s and add a line in the following file:
http://websvn.kde.org/trunk/KDE/kdelibs
/kdeui/ui_standards.rc?view=markup

But we have to discuss more about this before doing
anything.


> Well, how useful to see 4 for the reader of the code? I
got your point, so
> perhaps we should replace that int by enum? Then it
will be clearer. For
> example something like this:
>
> enum DifficultyLevelsCount
> {
>      ThreeDifficultyLevels, // easy, normal, hard
>      FourDifficultyLevels, /// etc
>      FiveDifficultyLevels
> // etc until how many we support
> }
>
> Yes, that's basicly the same, and perhaps naming isn't
that great, but IMO
> it is much more clearer to read this:
> new KGameDifficulty( this, false,
KGameDifficulty::ThreeDifficultyLevels);
>
> Moreover, this way will put clear restriction on how
much predefined levels
> we support - so passing arbitrary int such as 99 won't
work (without brute
> force of spell^W casts, of course).
> And such code is pretty self documenting 
>
> I'd even go further (wrt qt lib guidelines), and
replaced bool in
> constructor with simple enum too:
> enum { RestartOnChange, NoRestartOnChange } // naming
from head
>
> Then
> new KGameDifficulty( this,
>                                  
KGameDifficulty::NoRestartOnChange,
>                                  
KGameDifficulty::ThreeDifficultyLevels);
>
> is even more understandable for code reader (while
being more lengthy, yes)

This is a really good point and I tried to consider this by
the rewrite of the 
class. (It's now in SVN).


> That's great we can cooperate in such a nice way to
create
> something really useful by learning from eachother 

Yes it is! 


> > I personally prefer tabs. 
>
> Well, I think it should be discussed in another
thread.

OK.


Next step would be now to try to use this API in other
games. KBlackBox was 
here a kind of proof of concept. Dmitry, Mauricio, is it OK
if I do this in 
KMines now?

-- 
Nicolas

_______________________________________________
kde-games-devel mailing list
kde-games-develkde.org
https://mail.kde.org/mailman/listinfo/kde-games-devel