I did start having a go at improving it myself.
I haven’t read everything, but I’ll go through a few of the improvements that I’ve spotted so far.
Sorry this is a bit long, but I like being thorough with my explanations.
Like I say, the biggest improvement would be to just fix the indenting and settle on which brace style you want to use (you seem to be mixing multiple styles).
That alone makes the code much easier to read.
And here’s what it looks like after I made a number of stylistic changes:
I changed the indents to tabs instead of spaces (by default the Arduino IDE uses two spaces per indent, but there’s a hidden setting to change that).
(There’s a big debate over which is better. Unsurprisingly I prefer tabs over spaces)
(If you think the the amount of indenting seems a bit much, don’t worry about that. When you use tabs the level of indenting is specified by the editor/viewer, and for some reason GitHub defaults to 8 spaces for tabs, which I think is far too many. Usually I set my editor to 4 spaces per tab.)
When it comes down to it, consistency is more important than whether you use tabs or spaces and which brace style you use.
Pick a style, stick with it.
(Though honestly I still think all non-Allman brace styles are ugly.)
Use the smallest appropriate type
You could use
uint8_t instead of
int in some places.
int uses 16 bits and represents both positive and negative numbers (i.e. it’s ‘signed’).
uint8_t uses 8 bits and only represents positive numbers (i.e. it’s ‘unsigned’).
So by using
uint8_t when you can it saves memory.
There’s a few small cases where it’s less important but still worth doing, like here.
But where it would have a bigger impact is on your map.
world from an array of
int to an array of
uint8_t you would save about 112 bytes of RAM, which is a lot of memory.
Avoid logical redandancy
if (opp.hasBall && arduboy.everyXFrames(8) || !opp.hasBall && arduboy.everyXFrames(8))
Before I start, I’ll get rid of the ambiguity by adding some brackets:
if ((opp.hasBall && arduboy.everyXFrames(8)) || (!opp.hasBall && arduboy.everyXFrames(8)))
Now there’s no confusion over what it means.
One of the things I often recommend to people who are trying to improve their code is to read their code out loud as if it were English.
This line basically says “if the opponent has the ball and it’s the 8th frame or the opponent doesn’t have the ball and it’s the 8th frame”.
If you break it down, there are two redundancies in that sentence.
Firstly, it says “it’s the 8th frame” twice (i.e.
arduboy.everyXFrames(8)), when it could just say it once.
if (arduboy.everyXFrames(8) && (opp.hasBall || !opp.hasBall))
And I guarantee you, this means exactly the same thing as before (logically speaking).
So that’s one redundancy gone.
Now, redundancy number 2 is a bit more obvious.
“if the opponent has the ball… or the opponent doesn’t have the ball”
The opponent either has the ball or they don’t, there’s no inbetween (at least, not as far as the code is concerned).
(opp.hasBall || !opp.hasBall) is always just
The opponent always does or doesn’t have the ball (there’s no “well he/she sort of has the ball”).
So that brings it down to just:
player.image isn’t needed. It doesn’t actually do anything.
I’ve found it used in precisely one place (here) and in that one place it’s not actually doing anything.
playerImages is in PROGMEM, so trying to read from
playerImages without using
pgm_read_byte (or a similar function) will just give you junk.
In programming, there are four main ways of naming things
- ‘Pascal’ case -
- Uppercase letter for the first letter of every word, no underscores
- ‘Camel’ case -
- Uppercase letter for the first letter of every word except the first word, no underscores
- ‘Snake’ case -
- Lowercase letters for every letter of every word, words separated with underscores
- ‘Macro’ case -
- Uppercase letters for every letter of every word, words separated with underscores
Sometimes have different names for these,
but those are the definitions I see used most often.
Typically in C++ macro case is reserved for macros only, but after that people have their own different styles and it’s anything goes.
I find that for most people writing on Arduboy (or for Arduino devices in general) the preferred style is PascalCase for class names and camelCase for everything else (though a few people do different things).
The important thing here is not to write stuff like
gamestate where all the letters are lowercase and there’s nothing separating the words.
It should be
The main reason for this is that it’s harder to read,
but it’s also because certain words can be ambiguous.
It’s not a very ‘child friendly’ example, but let’s just say there’s a reason Experts Exchange is now at
www.experts-exchange.com and not
www.expertsexchange.com like it used to be.
That rule goes for things like
You should use
* Although actually
tilesWide could do with a better name, because that doesn’t actually tell you what the variable represents.
Also ‘wide’ isn’t a noun, and variables should be nouns.
So really this should be something like
tilesPerScreenWidth or something like that.
Avoid shortening words
I’m sure some people will disagree with me,
but I think you should always avoid shortening words.
A lot of people complain “it takes longer to type”,
but I think that’s a small price to pay to improve readability.
In the early days of programming using shorter words was more acceptable because computers didn’t have as much RAM so it actually made a difference, but these days there’s no excuse.
Life would be so much easier if C’s strchr was called
You’d know roughly what it did without having to check the documentation, and your code would look like it’s doing what it actually does.
const char * firstW = strchr("Hello world", 'w');
You could have:
const char * firstW = findChar("Hello world", 'w');
const char * firstW = findCharInString("Hello world", 'w');
Anyway, I’ll stop there for now,
I don’t want to overload you with information.
(I’d still like to explain about using
constexpr instead of
#define at some point, but that can wait for now.)