Last week’s report indicated that I didn’t get as much done as I would have liked. I managed to add a bunch of new random dialogs for weekends, but I didn’t get to grumpy client dialogs, which I pushed into the next sprint.
Sprint 5: More personality injections; streamline waiting for rain
Last week’s sprint plan was:
- Give each neighbor unique text as unhappy clients
- Streamline “Wait for Rain” option
In the last report, I said:
So this week’s sprint I will definitely get to the unhappy client dialog. I have some marketing screenshots to update, partly due to the copyright date, and I want to streamline the way you wait for rain to end so you can get back to raking as it is currently a bit clunky.
I did 2.25 hours of game development, but I did not actually work on any of the above.
So what did I do? Well, last week’s report also said:
I am kicking my Past Self for not spending more time on automated tests so I didn’t have to verify that I didn’t break anything each time I make a change. I’m slowly improving the code as I go along so that it is easier for Future Self, but Past Self is a short-sighted jerk.
This codebase is a few years old, and it has gotten fairly unwieldy. When I work on a 48-hour game development competition such as Ludum Dare, it’s fine for almost all of my game code to be in a single file. It is quick and dirty, and so long as I am mindful and focused, as I would be in a concentrated weekend development effort, I can make a game and then be done with it.
But when I have a commercial project that I am maintaining for years, something I come back to once in awhile, I need to break it up into well-tested pieces so that I don’t need to worry about forgetting how it works.
So I invested in my code quality last week. The rest of this report gets fairly technical, so feel free to skip to the end for my plans for the next sprint.
Code quality and its improvement
To give an idea of what I am dealing with, I have a class called InGameState which handles all of the game play logic and rendering. The class itself only has six functions:
virtual void onEnter(); virtual void update(int timeElapsed); virtual GBLib::IRenderCommand * draw(); virtual void onExit(); virtual void onSuspend(); virtual void onUnsuspend();
This class is the workhorse of pretty much all of my games. That is, when you start a new game, the Main Menu state changes to the In Game state. It calls onEnter(), which is where things get initialized, including loading saved games or creating default values for variables.
Then update() and draw() get called over and over until the player exits, which calls onExit(), and the state machine calls onEnter() on whatever other state, either the main menu state again or the exit state in preparation for shutting down the game.
But as I said, I like to throw as much as I can into a single file when I work on a quick game project. If you looked at InGameState.cpp from nine months ago, it was 4,148 lines of code, and my work in the last month has added another 100 lines of code or so.
In a well-written codebase, I’d argue that most class implementations shouldn’t be more than a few hundred lines of code.
Now onSuspend() and onUnsuspend() are meant to help with pausing and unpausing, usually by delegating to a different state, but in this game, they merely set a boolean value to true and false respectively, so they aren’t very interesting. So how are over 4,000 lines of code distributed among four functions?
Well, they aren’t really. There may be four major class member functions, but each one is implemented using inline code as well as non-member functions strewn throughout the file.
I don’t know offhand how many of those non-member functions are in the code base. There’s a lot. But I know my bias is towards lots of small, well-named functions. But this codebase is also a few years old, so some of those functions are not as small as I would like.
So why didn’t I add them to InGameState.h and make them part of the class? Well, as with my protoytpe projects, it is a lot easier to delete all of the .cpp file and copy the header into a new project than it is to remember which functions to keep and which ones are project-specific.
Why didn’t I create a project-specific class and leave InGameState as a generic class to be used across projects? Because I didn’t.
In any case, there is so much code in this class that handles everything from initializing variables, dialog management, time and date changes, weather updates, input processing, screen updates and changes, and more.
An example: generateNeighborhoodScreenBG()
There’s an entire function dedicated to figuring out how to render a particular part of the neighborhood.
This function is over 50 lines long. What may not be obvious is that it takes what is called a NeighborhoodScreen, iterates over the tiles at the x and y positions, and adds the sprite that corresponds with the tile to the spriteWidget.
Ostensibly, that is ALL it does. But when you look at the code, it looks like it is doing a lot more. In fact, most of the code was actually figuring out how to put sidewalks down based on where streets are located.
void generateNeighborhoodScreenBG(SpriteWidget * spriteWidget, NeighborhoodScreen * screen)
{
for (int x = 0; x < MAX_NEIGHBORHOOD_TILE_WIDTH; ++x)
{
for (int y = 0; y < MAX_NEIGHBORHOOD_TILE_HEIGHT; ++y)
{
int tileType(screen->tiles.at(x).at(y));
SpriteWidget * tileWidget = new SpriteWidget(TileTypesToSprites[tileType], Point(x * TILE_WIDTH, y * TILE_HEIGHT, ZORDER::BG));
if (STREET_TILE == tileType)
{
if (y != 0 && screen->tiles.at(x).at(y-1) != STREET_TILE)
{
tileWidget->add(new SpriteWidget(SideWalkHorizontalSprite, Point(0, 0, ZORDER::FG)));
}
else
{
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(0, 0, ZORDER::FG)));
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(TILE_WIDTH - 27, 0, ZORDER::FG)));
}
if (y != MAX_NEIGHBORHOOD_TILE_HEIGHT - 1 && screen->tiles.at(x).at(y + 1) != STREET_TILE)
{
tileWidget->add(new SpriteWidget(SideWalkHorizontalSprite, Point(0, TILE_HEIGHT - 27, ZORDER::FG)));
}
else
{
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(0, TILE_HEIGHT - 27, ZORDER::FG)));
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(TILE_WIDTH - 27, TILE_HEIGHT - 27, ZORDER::FG)));
}
if (x != 0 && screen->tiles.at(x - 1).at(y) != STREET_TILE)
{
tileWidget->add(new SpriteWidget(SideWalkVerticalSprite, Point(0, 0, ZORDER::FG)));
}
else
{
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(0, 0, ZORDER::FG)));
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(0, TILE_HEIGHT - 27, ZORDER::FG)));
}
if (x != MAX_NEIGHBORHOOD_TILE_WIDTH - 1 && screen->tiles.at(x + 1).at(y) != STREET_TILE)
{
tileWidget->add(new SpriteWidget(SideWalkVerticalSprite, Point(TILE_WIDTH - 27, 0, ZORDER::FG)));
}
else
{
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(TILE_WIDTH - 27, 0, ZORDER::FG)));
tileWidget->add(new SpriteWidget(SideWalkCornerSprite, Point(TILE_WIDTH - 27, TILE_HEIGHT - 27, ZORDER::FG)));
}
}
spriteWidget->add(tileWidget);
}
}
}
There isn’t anything wrong with this function. It does what is intended. Well, presumably. Since I have no tests around it, who knows what that intention was originally or if it continues to work when I’m changing code or even sneezing near it. But let’s assume with maybe only a little anxiety that the code works.
What I hate about it is that this code mixes levels of abstraction. I’m iterating through tile types and adding corresponding tile sprites to some parent sprite. But in one special case, I suddenly need to figure out how to add child sprites to the tile sprite. It’s important, but not so important that it takes up the lion’s share of the function! It just makes the entire thing harder to comprehend. So, a quick “refactor” changes the above code to:
void generateNeighborhoodScreenBG(SpriteWidget * spriteWidget, NeighborhoodScreen * screen)
{
for (int x = 0; x < MAX_NEIGHBORHOOD_TILE_WIDTH; ++x)
{
for (int y = 0; y < MAX_NEIGHBORHOOD_TILE_HEIGHT; ++y)
{
int tileType(screen->tiles.at(x).at(y));
SpriteWidget * tileWidget = new SpriteWidget(TileTypesToSprites[tileType], Point(x * TILE_WIDTH, y * TILE_HEIGHT, ZORDER::BG));
if (STREET_TILE == tileType)
{
addSidewalksToTileWidget(tileWidget, screen, x, y);
}
spriteWidget->add(tileWidget);
}
}
}
I simply extracted that giant collection of if/else chunks into a separate function called addSidewalksToTileWidget().
Now, you’ll note I said “refactor” above. Why the quotes? Well, as said, there are no tests around most of these non-member functions. I have no idea if I subtly broke something with this change. I think it looks fine, and when I play test the game, it still seems to be rendering sidewalks on streets as expected, but does it do the exact same thing as it did before? I have no way of verifying it. So it’s not really refactoring, because refactoring requires that I know with some confidence that I didn’t change the behavior when I changed the structure of the code.
You might also wonder what the above change bought me. Did I not just add more lines of code to InGameState.cpp?
Yes, yes I did. But as I said, the original function mixed layers of abstraction and was hard to read. It is now easier to read and understand. Future Me will be so pleased that Current Me didn’t get in his way when it comes to future updates.
Another example: getTimeText()
Ok, how about one that actually shrunk InGameState AND resulted in an increase in testing, giving me more confidence that the code works and will continue to work correctly in the future?
getTimeText() takes a TimeOfDay object and produces a string that I can render in the clock at the top left of the screen.
TimeOfDay handles dates, but for the purposes of this function, know that it has two important members: hour and minute. TimeOfDay uses 24 hours, and so based on the hour, getTimeText() needs to show a 12-hour clock and let you know if it is AM or PM.
The minute isn’t an actual count of minutes. Time passes in Toytles: Leaf Raking in 10 minute increments, so TimeOfDay’s minute can have values 0 through 5. So getTimeText() will need to convert minute to one of the 10 minute increments to render.
For example, if getTimeText() takes a TimeOfDay object with an hour of 14 and a minute of 3, it will return the string “2:30 PM”.
The TimeOfDay struct was already in a separate file called Gamedata.h, mixed with other miscellaneous classes. But the following code was in InGameState.cpp:
std::string getTimeText(const TimeOfDay & timeOfDay)
{
std::stringstream timeText;
int currentHour = (timeOfDay.hour == 0) ? 12 : timeOfDay.hour;
std::string periodString = (timeOfDay.hour >= 12 ) ? "PM" : "AM";
if (currentHour > 12)
{
timeText << currentHour - 12;
}
else
{
timeText << currentHour;
}
if (timeOfDay.minute == 0)
{
timeText << ":00";
}
else
{
timeText << ":" << timeOfDay.minute * 10;
}
timeText << " " << periodString;
return timeText.str();
}
Since it is so intimately tied to TimeOfDay, I decided to make it a member, and I wanted to be able to test this class, so I put it in its own .h and .cpp files.
There was a bit of hesitation on my part to add this function as a member of TimeOfDay. The struct is just a value, and getTimeText() outputs a particular rendering of that value in a certain way. How it gets rendered should be independent of what it represents, much like how CSS can let you style the same HTML differently based on your needs. What if a future project needs a 24-hour clock rendered? Or what if I don’t want to render text but will instead render an analog clock?
So good design tells me that getTimeText() is more of a rendering helper than an intrinsic part of the TimeOfDay struct, and therefore it should be separate.
But incremental and iterative change tells me that I can move it into the struct, then add tests to it, as well as other TimeOfDay functionality that wasn’t related to rendering, and I can worry about creating separate renderer utilities in the future. Future Me might wish he had them, but he’ll be appreciative that the improved test quality allows him the option to create them much more easily.
I have a function that generates the store’s hours that makes use of getTimeText(). It changed from
hoursOfOperationText << getTimeText(openingTime) << " - " << getTimeText(closingTime);
to
hoursOfOperationText << openingTime.getTimeText() << " - " << closingTime.getTimeText();
It’s not a big difference, which is part of the point. I wanted it to be an easy change that improved things, a simple step forward.
I don’t like that TimeOfDay is in charge of figuring out how to render its time, but for now, I’ll take well-tested, refactorable code over trying to fix all of the design problems at once without a test suite to catch breaking changes.
Oh, and TimeofDay.cpp is 63 lines of code. It’s super easy to read and understand.
I frankly don’t have the hours to spend trying to remember everything I might break while making a large change, so I make smaller changes that I can handle in a single session between the day job and taking care of the kids.
Also, I went on vacation
I meant to work on actual features to continue to inject personality into the project, but I spent almost 2 hours writing a blog post and an email newsletter to announce the most recent update to Toytles: Leaf Raking, and the remainder of my time on changes like the ones above to make future changes easier and safer.
And then I left town for a birthday vacation at a log cabin in northeastern Iowa. It’s easy to social distance when you are living in a forest with no AC. I had a great time with my family.
But that vacation is also why I haven’t written up this report for Monday like I planned. Next week’s report should be on the regularly scheduled day.
Next time, less random work
So my previous sprint got cut short, and my current sprint started late. I should probably commit to less getting done as a result.
I should ALSO stop myself from working on random “improvements” unrelated to what I committed to working on. I want more automated tests to give me confidence in what I am changing. I want to make the code easier to work within. I want to make Future Me happy.
But I also need to recognize that I didn’t get anything player-facing accomplished. I didn’t even address any bugs. I just restructured some code. There’s no actual value in doing so. In fact, Future Me might not even find any improvement in his situation from the changes I did make since I made them without any thought toward what he was going to work on next.
On the one hand, I’m the boss. I can give myself time to work on nothing but code quality improvement. No one is going to give me a hard time about slowing down my next release. On a whim, I can decide to work on a completely different project if I wanted, or not work on a project at all if I need the break.
But on the other hand, I’m the boss, and I need to keep my eye on continually adding value for players. No one else is responsible for ensuring that what I’m working on is the highest priority, and I need to hold myself to a higher standard. I need to be more deliberate.
You might think that I am chastising myself, but I’m not. I’m just taking this opportunity to analyze how the past sprint went and how I would like to change things going forward to improve future sprints. Future Me is going to have the benefit of hindsight, as well as a project that has steadily been improving in quality both from a game play perspective as well as a codebase perspective.
So going forward, code quality improvements should always correlate to work that adds value to the game in the eyes of my customers.
InGameState.cpp has a lot of low-hanging fruit along the lines of long functions I can shorten and classes I can pull out into their own files, so the temptation is going to be to tackle these all at once. I love refactoring code, even if I am just “refactoring” code carefully without the benefit of tests. And ultimately I’d like to get most of the code covered by tests, which requires pulling out more and more into separate modules.
But I’m also running an indie game development business. No one is going to be convinced to purchase a game because its internal code is easier for the developer to work with. There is value in Future Me having an easier time supporting and updating this and other projects, but Future Me is smart and capable enough to make the changes he needs when he needs them. Current Me will make the changes I need as I need them today.
I’ll waste less time on guessing what might be useful and actually work on what I see would be useful now.
—
Want to learn when I release updates to Toytles: Leaf Raking or about future games I am creating? Sign up for the GBGames Curiosities newsletter, and get the 24-page, full color PDF of the Toytles: Leaf Raking Player’s Guide for free!
One reply on “Toytles: Leaf Raking Progress Report – Refactoring, Testing, and Vacationing”
[…] Toytles: Leaf Raking Progress Report – Refactoring, Testing, and Vacationing […]