[WIP] September

I made a simple game for the Pre-Games Jam 5 Warm Up

September Source (CC0 1.0 Universal)

September is a memory game where the buttons you need to press will flash on the screen and you will then need to input them.

It is a bit clunky since I am going about things that aren’t ideal, but have given a lot of great feedback I will carry forward into games into the future.


Notes:

  • This is really a proof of concept and a way to dust off some C++ coding atrophy.
  • I was not able to get it working in ProjectABE, but that is an issue I personally need to troubleshoot.
  • This was a game I made over the course of a day and half.
  • @Pharap will be migrating his comments from Pre-Games Jam 5 Warm Up to this post when he has a moment. This is the feedback I was referring to.
  • I am open to criticism and tips, but before you post I would make sure @Pharap didn’t already cover them. :P
1 Like

I wasn’t really thinking about server backends.
If the code you’re writing is destined for your own servers then you’re free to waste resources to your heart’s content because it’s only ever destined for your own system.

I was thinking more about websites and desktop programs - things that run on and/or are hosted on the client/customer’s computer.

It’s probably better if I elaborate in a PM rather than derailing the topic.
(Which I’ll do later rather than now.)


I’ve decided to just address the things I found and assume I haven’t missed anything…

Note: I’m including the “Good things” section in a poor attempt to soften the blow of the text walls that follow. :P

Good things
  • classes are good
  • enum class is good
  • bool instead of boolean is good
  • Fixed width types are good
Important issues
  • countChars and printCenter are not const-correct. They should accept const char *, not char *.
    • If you’ve got warnings turned on, the compiler should warn you about this
  • Using signed types everywhere, even when negative numbers aren’t needed
    • Some places advise against unsigned integers, but I find that it’s usually just unfounded scaremongering. More to the point though, on AVR signed numbers can sometimes increase code size because they sometimes require extra checks, hence it’s more typical to find uint8_t and uint16_t being used unless negative numbers truly are needed.
  • Rather than intialising Game as Game game = Game(arduboy, sequenceLength);, you should probably prefer Game game(arduboy, sequenceLength) or Game game { arduboy, sequenceLength };.
    • It probably won’t make a difference in this case (assuming the dynamic allocation is removed), and in fact probably won’t ever make a difference in Arduboy code, but in desktop code when expensive objects are in use, using a = (technically called ‘copy initialisation’) can be more expensive than either of the later two approaches (which both invoke ‘direct initialisation’), the difference being that the former creates a temporary object that’s then copied (or moved) to create the variable, whereas the latter only directly initialises the variable without creating any temporaries. See here for more info.
  • The way your code is split between a .h and a .cpp makes it hard to follow in places. You should ideally either define everything in the .h or declare everything in the .h and define everything in the .cpp (with maybe a few exceptions for particularly short functions like getters).
  • Avoid implicit conversions to bool like the one in while(string[i]), it harms readability (and frankly it’s more C-ish than C++-ish). Prefer while(string[i] != '\0'). Similarly for pointers prefer pointer != nullptr and for integers prefer != 0 (as you have done in other places.
  • Nobody should ever really need to use drawSlowXYBitmap, it’s a waste of CPU cycles.
  • Mixing rendering and game logic is typically frowned upon - separation of concerns
  • The switch in generateSequence could be replaced with an array lookup. E.g.
void generateSequence()
{
	static constexpr uint8_t buttons[6]
	{
		UP_BUTTON, RIGHT_BUTTON, DOWN_BUTTON,
		LEFT_BUTTON, A_BUTTON, B_BUTTON,
	};

	for (size_t index = 0; index < sequenceLength; ++index)
	{
		uint8_t buttonIndex = random(6);
		sequence [index] = buttons[buttonIndex];
	}
}
Less important issues
  • You could just use arduboy.everyXFrames(sequenceSpeed) instead of arduboy.frameCount % sequenceSpeed != 0. It should produce the same code anyway (if you can demonstrate that everyXFrames is more expensive then fair enough), and everyXFrames is more widely understood.
  • You could use strlen instead of countChars, though I’m not sure if that’s technically an improvement or not since it probably uses more memory.
  • You don’t need a ; after a function definition. You only need it for type definitions (e.g. class, struct, enum class, union). This is because it’s technically legal to do: struct SomeType { } someVariable;, but most sane C++ programmers would advise against that - it’s an archaic C-ism.
  • All of your public variables should probably be private.
  • You could specify enum class Screen : uint8_t { Title, Board, Win, Loose }; to make Screen use 8 bits instead of the 16 bits it will default to. (An enumeration is backed by the default int if you don’t specify a custom underlying type.)
  • You should probably store buttons in uint8_ts instead of int16_ts.
  • sequenceEndBound could do with a better name.
  • More comments would be nice
  • Some more named constants to clear up the meanings of these ‘magic numbers’ would also be nice
  • displayBoard's logic is a tad strange. I can’t really put my finger on why. (I might just be having a hard time with it because I’m half asleep.)
Other things
  • Ordinarily it would actually be considered bad to have an initialise function. Typically it’s expected that a class’s constructor performs all necessary initialisation and ‘two-step initialisation’ is somewhat frowned upon. I don’t think it’s a particularly big deal in this case, particular as some things (e.g. arduboy.begin()) should never occur in the constructor of a global object anyway, but I thought I’d mention it.
  • It probably makes more sense to call arduboy.display() last in loop. I’m not entirely sure (because I’ve never actually seen it done this way) but I’ve got a feeling it might interfere with arduboy.nextFrame().
  • Technically speaking random(6) will have modulo bias which means some results are slightly more likely to occur than others, but it’s probably not enough to make a noticeable difference. Just bringing it up because modulo bias awareness is important.

There’s two advanced topics I’d ideally like to discuss which are:

  • Using progmem (though you can probably find lots of info about that on the forum)
  • How to store the length of a string literal so you don’t have to waste time calculating the size of a string at runtime (which is what I do in Minesweeper in order to centre the text)

But those are two big cans of worms worthy of more text walls,
so I’ll leave them for now to avoid oversaturating this comment.

1 Like