[Kde-games-devel] Re: [PATCH] Assorted KAtomic fixes

[Andreas Beckermann]

On Thursday 24 April 2003 12:58, Marc Mutz wrote:
> Hi!

Hi
Disclaimer: I am not familar with KAtomic code, I don't even play the game. I 
guess you are the guy with most knowledge about it here, so you should decide 
whether to commit. Anyway some comments.

-    if (feld[xpos][ypos+1] == 0) {
+    if (ypos < 14 && feld[xpos][ypos+1] == 0) {

This really looks like a big hack to me and it occurs several times in your 
code. Am I right that "ypos < 14" is for

> - The Feld class didn't do bounds checking at all, so a level was
>   required to have a wall around it to prevent situations where bounds
>   checking would be needed.
>   -> added bounds checking.
>   (feld.diff)

?
What about a
#define FIELD_SIZE 14
and using
"ypos < FIELD_SIZE"
or similar? That would make the code more readable and maintainable.

I just took a look at the current KAtomic code - these numbers seem to be 
hardcoded everywhere, but I think when you commit changes anyway, you could 
commit them in a readable way.

> - The molecule size was restricted to 10x10, while the field is 15x15
>   tiles big. This causes funny behavior when a molecule is bigger than
>   that (premature success).
>   -> increased max molecule size to 15x15.
>   -> increased minimumSize of molecule preview to 240x200 from 200x200
>   (molek.diff)

I really cannot comment on this :(

> - (libkdegames) kscoredialog unconditionally loaded the LastPlayerName
>   from the config. That leads to the problem that for each newly played
>   level, the preset player name in the score dialog is empty.
>   -> Overwrite the player name only, if LastPlayerName is not empty.

Umm did you change this? I could not find it in the attachments

> OK to commit fixes to both branches and new level to HEAD?

If you think your patch does the things that you listed, then I think it's 
fine. The patch looks fine to me.

> Marc

CU
Andi