Reorganising/refactoring of code

[oln]

Some of the classes/files are currently atrociously long. I think we should consider splitting some of them up to make the code more organised and readable.
I have some suggestions, though we should discuss this thoroughly to decide on a good setup.

Very large classes:

• ExampleFrameListener - nearly 4000 lines long
  • Could move the render queue processing to a separate class
  • Console command processing could also be split out
  • The render queue switch statement should be split to use a function for most of the cases instead of putting all the code in one huge function.
  • The class should mainly handle the mapping of events to actions
  • Not sure about the camera and text rendering stuff
• GameMap - ~2500 lines
  
  • Not sure about this one. There is at least a bunch of helper functions that can be split out. Possibly also the pathfinding.
• Creature - ~2300 lines
  
  • Again not sure. There are also some helper functions here.
  • Huge switch statement in doTurn, see above

There is also the use of a bunch of global variables defined in globals.h. I suggest we try to move some of the use of them into parameters or member vars instead.
I also think we should merge ExampleApplication and MapEditor, (MapEditor inherits exampleapplication). I don't see the point of having a split class like this. Also ExampleApplication has all code in the header file, which is not ideal.

EDIT: Also, rename Example*, and possibly MapEditor.

[MCMic]

Please start by renaming Example* classes :-D

[oln]

Yes, I forgot that point

[StefanP.MUC]

- Yes, renaming the whole "Example" stuff should be the first (this is also a todo in the general cleanup thread).

- The huge switch statements with the hundreds of loops should be splitted into functions and then refactored. I bet there are some loops/functions that could be merged without breaking the game logic -> better performance, decreases bug-chance by having less code, increasing readability.

- Pathfinding should definitly be something on its own.

- Console stuff should be separated. Makes the removing easier later on - should at some point in the future be replaced with CEGUI controls, like an input and output textbox.

[TheAncientGoat]

It's awesome that you guys are forging ahead with the codebase, but I do worry that without Andrew around, things might be changed that he had reasons for doing... But yeah, can't wait forever, and throwing away momentum is bad

[oln]

Well we're just cleaning it up, not really changing it.

[StefanP.MUC]

Yes, sometimes I'd like some more feedback/explanation from andrew, too. But on the other hand: it's open source and version controlled.

[svenskmand]

Andrew last visited the forums 26 Mar 2011, 02:17 so he have had the chance to stop you, and I am sure he has read this thread too. So I am sure he does not see a problem with you work. But yes I also agree that refactoring is usually best done by the guy who knows the code the best. But if Andrew does not have time to work on it, then I think it is very nice that you guys will do it

[oln]

Currently working on splitting the render request code into a separate class.

[StefanP.MUC]

By the way, the doTurn() in Creature.cpp uses goto. I think this should be done without goto when the function gets refactored.

[Bodsda]

Goto statemets are evil, and should be purged from our codebase.

Bodsda

[oln]

Mostly finished working my way through the switch statement now. I noticed two oddities, which look like bugs, but I could be wrong:

I assume this sould be cast to animatedObject and not creature:

{l Code}: {l Select All Code}

AnimatedObject *curAnimatedObject = NULL;
...
case RenderRequest::setObjectAnimationState:
                curAnimatedObject = (Creature*) curReq->p;

tempX and tempY are used before they are set. (Or set by a previous request in the same queue, they do not otherwise have set values at this point).
Looks wrong.

{l Code}: {l Select All Code}

 fieldItr = curField->begin();
 while (fieldItr != curField->end())
 {
          tempSS.str("");
          tempSS << "Field_" << curField->name << "_" << tempX << "_"
                  << tempY;
 
          tempX = fieldItr->first.first;
          tempY = fieldItr->first.second;
                    tempDouble2 = fieldItr->second;

[StefanP.MUC]

(edit: Fixed it)Yes, the first looks wrong.

The second could be right, at least in terms of code logic (if there'd be an unitialized usage of variables the compiler would give an error). But maybe it's semantically wrong, the code gives out the old values.

(edit: fixed a lot) There are a lot of temp variables that either are used for multiple task in the same scope or aren't needed in the whole of the code (stuff like creating a temp variable and in the next line returning it, instead of simply returning the value or just-in-case-variables that aren't used). I'll remove some of the obviously unneeded temp variables.

I said this also in another thread, but nobody gave feedback: There are some points in the code where it would make a difference if you'd used postfix or prefix increments. The current code has postfix which passes the old value before incrementing, but the code looks as it should actually use the new value (what would be prefix increment).
GameMap.cpp, line 2532
MapLight.cpp, line 17
MissileObject.cpp, line 30
RoomObject.cpp, line 18

[oln]

(edit: Fixed it)Yes, the first looks wrong.
(edit: fixed a lot) There are a lot of temp variables that either are used for multiple task in the same scope or aren't needed in the whole of the code (stuff like creating a temp variable and in the next line returning it, instead of simply returning the value or just-in-case-variables that aren't used). I'll remove some of the obviously unneeded temp variables.

Yes, there was a lot of this in the renderrequest switch statement. I've fixed it in the new class I made. It's on the repo, but not finished yet. Will try to get it working today.

[StefanP.MUC]

I'm currently working on getting rid of Globals.h (I already killed Defines.h by merging it into Globals.h and reduced the resulting file by moving a lot of stuff into members variables).

I had a problem with GameMap, however. I wanted to make it a singleton but this seemed to make some problems because of ProtectedObject (what exactly does this do?).

[oln]

I think it's supposed to make it thread safe or something. What I was thinking about doing earlier was sending is as a parameter where it's used, not sure what solution is the best.
EDIT: Yeah, it's basically a wrapper around an object that protects access with semaphores.

[StefanP.MUC]

Im currently splitting up the ODFrameListener a bit more, by moving all the Mouse and Keyboard stuff into its own InputManager class. Then we'll also have a central place to change and define the input events and don't have to mess around with the 3k lines FrameListener.

And I think, after that I'll try to make some kind of a "Navigation class" that holds all the camera movements and also defines some "limits" (currently we can scroll out into infinity, that shouldn't be possible). Currently the button events have several tasks all mixed together, that's not very good.

If everything works well the 3k lines FrameListener should then be 3 classes with less than 1k lines each.

[StefanP.MUC]

OK, pushed the refactoring, worked quicker than expected.

The goal to get all three around 1k lines of code wasn't possible. But the FrameListener is now below 2k. All the "help texts" and nested if's still consume a lot of space in there.

The InputManager has around 1k lines, but this can be cleaned up more, because it contains a lot of "direct code" (and I think also duplicate code) instead of calls to functions in a better places. The CameraManager has ~200 lines of code and is pretty clean.

Now it should also be a lot easier to implement mouse wheel scrolling, a long wanted feature. maybe I look into it tomorow.

[svenskmand]

Nice work Stefan