Hi,
I started my project a few years ago as a hobby and I've been learning as I go. I'm currently in the process of fixing various bugs, and I'm really starting to notice that a lot of my old code is quite poorly written. Back then I didn't really know any better, but since then I've learnt a thing or two and well...some of what I did really sucks. I even think the way I'm currently fixing some of my bugs now is kind of poor, kind of like a jug filled with holes and I'm plugging one or two here and there at a time with chewing gum.
I'm really tempted to re-write parts of my old code, but it kind of seems overwhelming...even discouraging with over 40k of code (a lot of it is prob bloated crap that semi-works or re-copied chunks I should have put in a function). The newer, more recent stuff is OK I think, but a lot of the older stuff (with newer code written on top of it or merged into it) makes me cringe at the thought of "re-writting"...I dont even know if its possible at this stage since some of it is "inter-twined". :cry:
How do "real programmers" approach a problem like this? I remember reading an article on "Bad Coding Practices To Avoid", and I'm noticing I've pretty much violated them all when I started out in the beginning. How do programmers start fixing "mediocre or bad code", and is it normal to really dread this? For example, I've got a lot of else-if statements in my rep exec for special situations that probably only happen once to fix a specific bug, and when I encounter more, I use even more else if's...yikes. Its like piling crap onto even more crap.
Also, I think one of the main problems I have now in fixing bugs is that I create other bugs that I only discover sometimes much later...then when I finally find and fix those new ones I inadvertently break something else...which again I only sometimes notice a loong time later on! Argggg!
How do you guys do it? :embarrassed:
In the cases like the one you described, when the code is so bad that fixing existing bugs becomes a torture, there's no other choice but to rewrite the program, this way or another.
The radical path is to just rewrite everything from scratch, using older code as a reference. If you fear you won't manage, or loose enthusiasm even quicker that way, you'll have to do what is called "refactoring".
General rule here is: stop introducing new features, and stop looking for bugs too, that will distract you. Stop making "quick fixes" and "hacks". Stop duplicating code.
Your final aim should be to make a code clean enough to let you see exactly what it does on a quick glance. This will significantly increase bug-finding efficiency after you're done with this stage.
I do not see your code, so I can't tell what your code looks like: whether you have lots of small-to-medium-sized functions, or maybe you have few giant functions of 500-lines each. I'll try to explain generic approach.
Start with splitting the code by modules. AGS is not the best for that, because of how it implements module dependency, but you can try. When possible, keep script module size below 1000 lines (the number may depend on what you feel comfortable to work with). Group functions by what they do (and what objects they work with). If you can't/don't want to put them in different modules, put "divisors" between them for better visual discretion, such as:
//==================================================================
// FUNCTIONS THAT DO STUFF WITH CHARACTERS below this line
//==================================================================
Just start with the very beginning of your script and scroll down. Put a comment for every function, telling what it does (maybe in short). When you commented, like, 20 functions, check them again to see if you can divide them by 2 or more groups somehow. If the group of functions is very distinctive, create a new script module and put them there.
When you did like 50 functions (the numbers are random, see what you are comfortable with), make a pause and spend time on cleaning their code:
Do some functions have duplicate code? Pick it out into new function.
Is the function too large (more than the size of your screen)? Split it in parts, let the original functions have only function calls, for example:
void function_part1()
{
<...>
}
void function_part2()
{
<...>
}
void function_part3()
{
<...>
}
void original_function()
{
function_part1();
function_part2();
function_part3();
}
Do not be afraid to make small functions for simple things. Rather force yourself to do so. If you'll later find out that this causes some troubles, fix it, but not before it really causes a problem.
Try to pick out algorythms that have much in common, even though they may not be identical. Use parameters to create generic algorythm that can run all your cases.
A simple example: imagine you create overlays in code, every with different background and text, drawn with different color:
DynamicSprite *spr = DynamicSprite.Create(100, 100);
DrawingSurface *d = spr.GetDrawingSurface();
d.DrawImage(0, 0, 123);
d.DrawingColor = 12;
d.DrawString(10, 10, eFontNormal, "bla bla bla");
d.Release();
Overlay *o = Overlay.CreateGraphical(spr.Graphic);
You can put the code like that into a function with parameters:
Overlay *CreateMySpecialOverlay(int image, int text_color, FontType font, String text)
{
DynamicSprite *spr = DynamicSprite.Create(Game.SpriteWidth[image], Game.SpriteHeight[image]);
DrawingSurface *d = spr.GetDrawingSurface();
d.DrawImage(0, 0, image);
d.DrawingColor = text_color;
d.DrawString(10, 10, font, text);
d.Release();
return Overlay.CreateGraphical(spr.Graphic);
}
You may later find out that the function is too generic, and on many occasions you have to put the very same parameters again and again. In such case, create a new "helper" function to use in such cases, for example:
Overlay *CreateMySpecialOverlayOfType1(String text)
{
return CreateMySpecialOverlay(123, 12, eFontNormal, text);
}
In a nutshell: try to generalize your code as much as possible: have a set of "universal" functions with many parameters that do the core things, and a set of "helper" functions with little parameters that call "universal" ones with certain often-used parameters. Again: do not be afraid to have many of these, if you find out some "helper" ones are superfluos, you can always delete them later.
On other hand, even if the "helper" function is used only once in your code, that still makes sense.
Another very important thing is to systematize the code. As you progress cleaning up your code, try to make it so that game resources are directly worked with from within one function only (or limited group of functions).
For example, assuming there's a GUI that have some info displayed on itself. Do not set that info from various parts of code. Instead make a function, or a set of functions (or even a custom struct, if you do know how to work with these) that will work with that GUI, and call these functions from all places where you previously worked with that gui directly.
This is important, because if at some point you will decide to fix/change something related to this gui, you will only have to change limited number of functions.
After you cleared a formidable part of your code, you may notice that all code that works in particular area (like all audio, or all gui code) has been cleaned and grouped together. Then make a pause and try to fix bugs in this cleaned code. Then return to clearing rest of code.
Quote from: Crimson Wizard on Sun 27/10/2013 16:08:41A simple example: imagine you create overlays in code, every with different background and text, drawn with different color:
DynamicSprite *spr = DynamicSprite.Create(100, 100);
DrawingSurface *d = spr.GetDrawingSurface();
d.DrawImage(0, 0, 123);
d.DrawingColor = 12;
d.DrawString(10, 10, eFontNormal, "bla bla bla");
d.Release();
Overlay *o = Overlay.CreateGraphical(spr.Graphic);
You can put the code like that into a function with parameters:
Overlay *CreateMySpecialOverlay(int image, int text_color, FontType font, String text)
{
DynamicSprite *spr = DynamicSprite.Create(Game.SpriteWidth[image], Game.SpriteHeight[image]);
DrawingSurface *d = spr.GetDrawingSurface();
d.DrawImage(0, 0, image);
d.DrawingColor = text_color;
d.DrawString(10, 10, font, text);
d.Release();
return Overlay.CreateGraphical(spr.Graphic);
}
AGS handles it, but it will still complain that you're not deleting that DynamicSprite (at least in the log files). You
should handle it yourself. There's also no reason to use the DrawingSurface functions for drawing the background, as that's probably slower than just creating a copy of the sprite directly:
Overlay *CreateMySpecialOverlay(int image, int text_color, FontType font, String text)
{
DynamicSprite *spr = DynamicSprite.CreateFromExistingSprite(image);
DrawingSurface *d = spr.GetDrawingSurface();
d.DrawingColor = text_color;
d.DrawString(10, 10, font, text);
d.Release();
Overlay *result = Overlay.CreateGraphical(spr.Graphic);
spr.Delete();
return result;
}That might seem a bit nit-picky, but if you're offering advice on cleaning up code... :P
Quote from: Crimson Wizard on Sun 27/10/2013 16:08:41Start with splitting the code by modules. AGS is not the best for that, because of how it implements module dependency, but you can try.
I actually disagree with this. While I admit that it is a limitation of the script compiler (and has nothing to do with the AGScript language itself), this actually forces the developer to
think about the relationships between various bits of code. Even the A calls B calls A problem can be solved by the fact that the compiler allows recursive function calls -- so long as you hadn't already devised an infinite recursive loop in the first place then you shouldn't run into problems creating, as you say, a more universal function that handles A and B, calls itself recursively as needed, and can be accessed by more specialized helpers.
Short of A-B-A recursion, there's not really any reason why a script function should have to call a function that is defined after it.
Like I said, it would be an improvement if the compiler could link functions regardless of where they were defined, but I think the difficulty of modularizing code was still vastly overstated.
Wow, thanks Crimson! I copied your post to my desktop. Im going through my rep-exec right now, since it is the messiest (a lot of duplicated code, too many else-ifs, etc). By the way, is there a faster way to find duplicate chunks of code in AGS, seems I can only check 1 line at a time, not a paragraph.
Just wanted to pop back in to say thanks Crimson, this is a lot easier than I thought it would be, thanks to your tutorial. I'm currently breaking up big chunks of code into smaller modules now and making "helper functions". I can finally "see the light", hehe!
Quote from: monkey_05_06 on Mon 28/10/2013 01:58:22
Short of A-B-A recursion, there's not really any reason why a script function should have to call a function that is defined after it.
Hey Monkey,
I rencently got a stack overflow error...I looked it up on Google and in the AGS forums and it seems those errors can be related to recursive calls? What would be the best way for me check my code to see if I have those, and where?
IIRC, the only time I've ever seen a stack overflow in AGS is when I was using recursion, so that seems a likely culprit, yes. I guess the first thing to ask though, are you really aware of what recursion is (in programming)?
A recursive function is simply one that calls itself. You have to be very careful when doing this, especially in AGS. I don't remember the exact limit, but I think if a function calls itself more than a dozen or so times that you'll get a stack overflow.
int ack(int m, int n)
{
if (m == 0) return n + 1;
if (n == 0) return ack(m - 1, 1); // else if ((m > 0) && (n == 0))
if ((m > 0) && (n > 0)) return ack(m - 1, ack(m, n - 1)); // else if...
AbortGame("Ackermann function undefined for negative values."); // else...
}
This is the Ackermann function (http://en.wikipedia.org/wiki/Ackermann_function), which is a fairly standard representation of recursive programming. In AGS that's likely to explode very quickly, but Wikipedia has a table of values (http://en.wikipedia.org/wiki/Ackermann_function#Table_of_values) to show the expected output. What's important to recognize about this function is that it has very specific checks to end the function prematurely based on the parameters given. This is true of all recursive functions, otherwise you would have infinite recursion.
So, if you're getting a stack overflow error, then you'd want to look for similar cases in your code where a function is calling itself (see how the "ack" function calls itself?). In fact, I just tested it, and when the engine aborts (if running from the debugger) it should take you directly to the line where the recursive call is happening. Should be pretty simple to figure out where. Your next steps would be to determine if you really need recursion, and if you do, to better define your exit cases to stop it calling itself recursively so often.
I'm almost 100% sure if I have recursion somewhere, it's by mistake since I didn't even know what that was until now.
I'm trying to reproduce the error, when it first popped-up I closed the window too soon so I dont know exactly where/when it happened. As soon as I can reproduce it, I'll post more info.
Ok so far I can't seem to reproduce that stack overflow error...I'll keep trying though. I wonder if while refactoring I might have fixed it my mistake.
I've got another question with these 2 bool functions here:
bool Mouse::InsideBounds(int iMinX, int iMaxX, int iMinY, int iMaxY)
{
return ((mouse.x >= iMinX && mouse.x <= iMaxX) && (mouse.y >= iMinY && mouse.y <= iMaxY));
}
bool InsideBounds(this Mouse*, int iMinX, int iMaxX, int iMinY, int iMaxY)
{
return ((mouse.x >= iMinX && mouse.x <= iMaxX) && (mouse.y >= iMinY && mouse.y <= iMaxY));
}
What is the "better" way to write that function? What are the two :: called, and do they act the same way as if I were to write "this Mouse*" (extender)? Seems I get the same results, but is one of them "faster" or something?
Unless there's a way to make a static extender function which I do not know, these two functions are two implementation of exactly the thing...
I made an experiment and found that you can declare:
import bool InsideBounds(this Mouse*, int iMinX, int iMaxX, int iMinY, int iMaxY);
and then define two function bodies like you have, only in different script modules. Not sure which function will be chosen at runtime though.
Again, these functions should be exactly the same thing, and neither is faster. You should just leave the one you like more, and remove another.
QuoteWhat are the two :: called, and do they act the same way as if I were to write "this Mouse*"
'::' means "the member of type", in your case - the member of Mouse type. There will be a difference to "this Mouse*" if the first function was static (because static function does not get "this" pointer as first parameter), but I believe it is not the case.
When you write it as "Mouse::" for the non-static member function, the "this" parameter will be added implicitly by compiler, so you do not have to type it yourself.
Right, once a function has been
declared as an extender it can be
defined using Name::Scope or extender syntax. The two definitions are equivalent in this case (assuming the same function bodies). In the case of different scripts both defining the function, AFAIK AGS will always reference the
first defined when resolving function calls. This was an issue with SSH's BackwardsCompatibility module because he was trying to do some bound or sanity checking that the engine actually wasn't doing, but his imports were just referencing the built-in function definitions (the old-style functions are still defined regardless of "Enforce object-based scripting", they just aren't imported/declared).
Quote from: Knox on Sun 01/12/2013 21:07:29I wonder if while refactoring I might have fixed it my mistake.
I'd actually say it was more likely that you introduced it by mistake. ;) Recursion isn't something that you're going to have a lot of practical use for (at least, not in using AGS to make an adventure game).
Quote from: monkey_05_06 on Mon 02/12/2013 02:29:04
I'd actually say it was more likely that you introduced it by mistake. ;)
Haha yeah, that too! Thanks for all your help guys, I'm learning a lot! :=