[glob2-devel] the mess in GameGUI.cpp (was: Ugly code in glob2 hg)

[Joe Wells]

Stéphane Magnenat <address@hidden> writes:

> I've had a quick look at src/GameGUI.cpp to look for the scroll bug, at I've 
> discovered with horror that there is a lot of key related code that is not 
> styled according to glob2's coding rules.

I'm happy to use whatever whitespace rules there are provided the
rules are actually written down somewhere!  I could not find the
whitespace rules written anywhere.

> This code is very hard to read and 
> thus very likely to hide bugs. I've fixed it, but please take care of doing 
> good looking code.
>
> Furthermore, this key code is full of "if" and "case", which shows a bad 
> coding practice. This should be improved with tables and indexes.

> Also, indexing actions with strings is bad for readability and efficiency.
>
> Oh and did I mentioned it is fully uncommented?

By the way, whenever I made changes, I continued the pre-existing
(bad) practice of how the key code was written (lots of “if” and
“switch” statements, indexing actions by strings, etc.).  I completely
agree that the key code should be converted to using tables, but I
decided against doing this conversion myself, because it would mean
touching lots of additional code that was not involved in my changes
(for example, I would have had to change the code in another file that
stores user-defined key bindings in the configuration file).

I have added lots of comments for most my changes, but as you have
noticed lots of the previous code is uncommented.

> In summary, I don't have time to find the bug now

Just look for the place where the code tests that certain modifiers
are not pressed while holding the arrow keys down.  (And the bug is
actually in libsdl, which gets confused about which modifiers are
pressed.  If the arrow keys are not working for you, it is because
libsdl thinks you are holding down Alt or some other modifier key.)

> in such a mess.

The entire GameGUI.cpp file is a horrible mess.  For example, in
addition to the complaints you made above, there are magic constants
everywhere.  Just one example of this is all the places where numbers
are shifted by 5 or 6 bits to convert between game cell positions and
pixel positions.  And the number 1024 (the size of the unit and
building arrays) is all over the place in this and other files
(sometimes in the form of a bitmask 0x3FF).  And the pixel width of
the control panel on the right is hard-coded everywhere, as well as
the positions of all of the buttons in the panel.  And there are lots
of other similar issues.  Why aren't these magic numbers (5, 6, 1024,
etc.) abstracted away in (inline) methods or at least constant
definitions?

-- 
Joe