This post was saved / ported from a previous site migration. You may encounter missing images, dead links, and strange formatting. Sorry about that!

Project TreeWars: How to write bad code

Programming C++ TreeWars geometry math

The TreeWars project is a week and a half old (at the time of this writing). It’s come a long way; in its current form, it actually looks pretty neat:



There are now stats, and hit points, and the basic gameplay mechanic is in place. I’ve also laid the groundwork for some more complex gameplay, as well. Of course, I haven’t talked about gameplay all that much yet - we’ll get to that at some point, I’m sure. But what I want to talk about right now is how to write bad code.

Bad code happens. Sometimes, when you’re writing code, you will hit a stumbling block. Encounter a problem where the solution doesn’t jump immediately to mind. And often, you’ll look back on it later and say “what the hell was I thinking?” Well, here is an instructive example of that.

This blunder is actually pretty embarrassing; I really did know better, but that knowledge escaped me at the time. As you’ve probably noticed from the screenshot above, my program deals with drawing circles (called either ’nodes’ or ‘vertices’). If a user clicks to add a new node, my code has to check and see whether it will overlap with an existing node.

So, I had a function that answers the question “is there a node close enough to the point (x,y) that a new node there will overlap?” If you’re good at geometry, you probably already see the right way to do it. But I bet you can’t spot the very very wrong way!

My solution (and I stress that this is a really, really bad idea) was to create a ‘bounding box’ around each existing node, and a bounding box around the area that the new node would occupy, then do some complex logic to see whether the bounding boxes overlap. It looked like this (and if you’re not a programmer, just note the general size and complexity of this code):

[sourcecode language=“cpp” gutter=“false” wraplines=“false”]
bool Graph::vertex_present(int x, int y, int size)
{
   int delta = size / 2;
int x_min = x - delta;
int x_max = x + delta;
int y_min = y - delta;
int y_max = y + delta;

for (list<Vertex>::iterator cursor = vertices.begin();
cursor != vertices.end(); cursor++)
{
Vertex v = (cursor);
if (((x_min >= v.x_min && x_min <= v.x_max) &&
((y_min >= v.y_min && y_min <= v.y_max) ||
(y_max >= v.y_min && y_max <= v.y_max))) ||
((x_max >= v.x_min && x_max <= v.x_max) &&
            ((y_min >= v.y_min && y_min <= v.y_max) ||
(y_max >= v.y_min && y_max <= v.y_max))))
{
return true;
}
}
return false;
}
[/sourcecode]

It’s ugly, and hard to read, and I knew it felt like a hack when I wrote it. But at that moment, nothing better was coming to me. Now, for those of you who don’t enjoy recreational geometry, here’s the right way to do it: you know that the existing node is at (x1,y1) and that it has a radius of r1. The point you’re checking is at (x2,y2) and the new node would have a radius of r2. So, all you do is get the distance between the two points (basic geometry, I could have googled “the distance formula” and about 8 billion websites would have popped up to tell me how much I fail here), and then compare that to r1 + r2. Like so:

[sourcecode language=“cpp” gutter=“false” wraplines=“false”]
bool vertex_present(int x, int y, int r)
{
for (list<Vertex
>::iterator cursor = vertices.begin();
cursor != vertices.end(); cursor++)
{
float dy = y - v->y;
float dx = x - v->x;
if (sqrt(dy
dy + dx*dx) <= v->;r) return true;
}
return false;
}
[/sourcecode]

See? much simpler.

Now, why did it take me 24 hours to figure that out? Well, I wrote the original code when I was pretty tired, and the nature of programming is that once you write a function and it works, it is easy to forget about it. After I needed to calculate distance somewhere else, though, an alarm went off in my head. “Hey,” said the alarm, “You could have used that over in vertex_present()! What is wrong with you?"

Well, alarm, look. Sometimes I just can’t brain as well as other times. But hey, at least it ended up being nice and pretty in the end, so thanks for that! Actually, vertex_present() doesn’t even exist in my code any more; it was refactored into some other function, because I only actually used it in a single spot.

Luckily, I can go back in time and show you how awful my code used to be through the magic of git, which I use to track all of my programming projects. It lets me periodically ‘commit’ the changes I’ve made to my code, and keeps track of everything I’ve done. I can then ‘checkout’ any commit, instantly reverting my code to an old state, and then ‘checkout’ the latest code again when I’m done. It can do a lot more than that, too. If you write code, I highly recommend using git or another version control system.

So, that’s all for today. Next time, we might even talk a little bit about gameplay! With a little luck, the game will be sophisticated enough by then that there will be something to talk about.