[Kde-games-devel] KGameDifficulty API notes and suggestions

[Nicolas Roffet]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part.
Url : http://mail.kde.org/pipermail/kde-games-devel/attachments/20070710/caf2f385/attachment.pgp