Help with game development


(Pharap) #101

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.


Consitent Indentation

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.

Here’s your code before:
https://github.com/JohnnydCoder/Little-Kickers/blob/1120efea691f0159d618b5c1f6ed27e944987bce/opponent.h

And here’s what it looks like after I made a number of stylistic changes:
https://github.com/Pharap/Little-Kickers/blob/9563077531f8e26a8e8177d16cf92fe845efbaa2/opponent.h

(I also did the same for maps.h: the original vs my version, but didn’t get round to doing it for any of the others.)

I changed the brace style to Allman style because that’s what I use.
It seems like before you were using a mixture of Allman style and 1TBS.

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.

By changing 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

Next, I noticed a bit of redundancy in line 47 of opponent.h and line 122 of opponent.h.

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).

If opp.hasBall is true then !opp.hasBall is false,
and if opp.hasBall is false then !opp.hasBall is true,
so (opp.hasBall || !opp.hasBall) is always just true.
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:

if (arduboy.everyXFrames(8))

player.image

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.


Casing

In programming, there are four main ways of naming things

  • ‘Pascal’ case - SmallDog
    • Uppercase letter for the first letter of every word, no underscores
  • ‘Camel’ case - smallDog
    • Uppercase letter for the first letter of every word except the first word, no underscores
  • ‘Snake’ case - small_dog
    • Lowercase letters for every letter of every word, words separated with underscores
  • ‘Macro’ case - SMALL_DOG
    • 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 tileswide or gamestate where all the letters are lowercase and there’s nothing separating the words.
It should be tilesWide* and gameState.
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 tilex and mapx too.
You should use tileX and mapX instead.

* 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 screenWidthInTiles or 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.
I.e. write opponent, not opp.

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 findChar or findCharInString.
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.

Instead of:

const char * firstW = strchr("Hello world", 'w');

You could have:

const char * firstW = findChar("Hello world", 'w');

Or:

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.)


(JohnnydCoder) #103

Thanks for helping improve my code!

What is constexpr?


(Pharap) #104

The keyword constexpr has two uses, constexpr variables and constexpr functions.
(constexpr functions are a story for another day.)

constexpr variables are like normal variables in that they hold a value, but that value can never change - i.e. they are constant values.
constexpr variables are what should be used to represent constant values because they have several advantages over other approaches.

E.g. instead of

#define OPP_SPEED 1

a better approach would be

constexpr uint8_t opponentSpeed = 1;

The main benefits of constexpr over macros are:
(Once again, click on an arrow to get more info.)

Better control over the type
  • You can choose the type of the variable
  • The type of a macro is the type of whatever the macro’s expression is

E.g.

// Type is `uint8_t`
constexpr uint8_t opponentSpeed = 1;

// Type is `int` (`decltype(1)`)
#define OPP_SPEED 1

To make the macro a uint8_t you’d have to do:

#define OPP_SPEED static_cast<uint8_t>(1)

Which is a lot more effort.

Better scoping

A variable can be put in a specific scope, like a namespace or a class.
As a result of this, you can have two variables with the same name if they exist in different scopes.

Macros on the other hand don’t have a concept of scope, after they’re defined they ‘infect’ all scopes until they’re undefined.

E.g.

constexpr uint8_t opponentSpeed = 1;

#define OPP_SPEED 1

void someFunction()
{
	// Allowed, different scope
	uint8_t opponentSpeed = 2;

	// ERROR, conflict
	uint8_t OPP_SPEED = 2;
}
Guaranteed to be evaluated at compile time

By their definition, the value/expression used to initialise a constexpr variable must be able to be evaluated at compile time.

This means that they are always evaluated at compile time by the compiler, so what ends up in the program is actually just a single constant value, even if the expression is quite complex.

This is usually true with macros, but it isn’t always.
Ultimately it depends how good the compiler is.

Macros are buggier than constexpr (a.k.a. macros are evil)

Macros are just a bad tool in general because of how they work.
Macros aren’t actually part of the C++ language itself,
they’re part of the preprocessor phase.

Before C++ code is compiled by the compiler, another program called the preprocessor goes through all the code and processes all the macros.

Essentially the preprocessor is just a glorified ‘copy & paste’ machine.

For example, if you used your macro OPP_SPEED,
what goes into the preprocessor is this:

opp.x += OPP_SPEED;

And what comes out is this:

opp.x += 1;

So in my earlier example, in someFunction, what the code looks like after the preprocessor is done with it is this:

constexpr uint8_t opponentSpeed = 1;

void someFunction()
{
	// Allowed, different scope
	uint8_t opponentSpeed = 2;

	// ERROR, conflict
	uint8_t 1 = 2;
}

This illustrates why macros don’t have a concept of scope - they’re not actually part of the language itself, they only exist at the preprocessor phase, after that they’re gone.

Because of this, macros can go badly wrong if you aren’t careful.

For example

#define OPP_X_OFFSET WIDTH - OPP_WIDTH

If I were to then write:

int x = OPP_X_OFFSET / 2;

The preprocessor would translate that to:

int x = 128 - 10 / 2;

So instead of getting (128 - 10) / 2 you get 128 - (10 / 2), which is a hard to spot bug.

If, however, a constexpr variable were used instead, this wouldn’t be a problem:

constexpr uint8_t opponentXOffset = WIDTH - opponentWidth;

And then if I were to write this:

int x = opponentXOffset / 2;

There’s no substitution, opponentXOffset is calculated at compile time (128 - 10), so there’s no bug.

If that’s not enough, there’s some very good examples in the isocpp FAQ:

90% of the time there’s a language feature that will do the job better than a macro.


(JohnnydCoder) #105

Little Kickers has been released! :grinning: