Magic Numbers: Need to Remove

[antfarmar]

The code has several areas in which magic numbers are used due to laziness. Never a good idea. It will cause stupid time-wasting bugs later on since the magic number is most likely dependent on/a function of some other variable.

[ghost]

The most glaring place I could spot was AsteroidBaseBehaviour.SpawnRandomPositionwhich has a constant radius when testing for overlap.

AsteroidBigBehaviour has a literal for how many small asteroids to spawn. I doubt it'll cause bugs though.

Yes, there are some other literals like scale variance etc, but I hardly believe it will cause bugs. Perhaps expose it as a slider if you need to frequently adjust the scale to avoid rewriting code.

[antfarmar]

The most glaring place I could spot was AsteroidBaseBehaviour.SpawnRandomPositionwhich has a constant radius when testing for overlap.

Good eye. That's the code that made me open this issue frankly. The rest aren't so bad, as you already noted. (i.e. initial spawn counts) But since this is Unity, I should probably be taking advantage of the Inspector for those types of variables. They'd be perfect candidates for a [Range] attribute.

[ghost]

I decided to add another comment, but for a different reason. I know you do this mostly for yourself, therefore I feel this is secondary and more reflective on why (or why not) you'd remove magic numbers. You may choose to ignore to "fix" some of your numbers once you understand the reason the programming community dislikes magic numbers, or ignore to if you find no reasonable need other than "ceremonially doing so, because someone said something, even though I don't understand why". Not all magic numbers need to be variable, and not all magic numbers need to have named symbols.

---

Discussion continued

... There are more literals in your code, but from my point of view they ought not to cause bugs. However, for other reasons - communication between developers - creating a constant to give the literal a name that describes intent, could be helpful. For code like this:

m_AsteroidBigPool = new ObjectPool(m_AsteroidBigPrefab, m_AsteroidParent, 10, 20);

It is not clear to someone new, what 10 and 20 means. It's pretty clear we're creating a pool that accepts a prefab, a parent, but 10 and 20 is not so clear.

const int startCount = 10;
const int maxCount = 20;
// shortened variable name to fit on screen
bigPool = new ObjectPool(m_AsteroidBigPrefab, m_AsteroidParent, startCount, maxCount);

This might make it clearer, but also more verbose (we're adding information, not reducing information).

Another option could be to use named parameters. It's fine to use in Start because a lot of stuff already is happening but using named parameters at performance critical sections - you might want to be careful. I haven't experienced any performance problems myself, but the article states some. At least in the case where the parameters are re-positioned.

// shortened variable name to fit on screen
bigPool = new ObjectPool(m_AsteroidBigPrefab, m_AsteroidParent, startCount: 10, maxCount: 20);

But in most cases, you can rely on your IDE to tell you the parameter names over a mouse over, so you may want to consider who this helps. Probably people reading through it at a glance, people who don't use modern IDEs and newbies who don't know how to use their IDE, people reading it on paper, on presentations, in code reviews, etc. Picture yourself in their shoes and decide if the code is clear enough or not. Or ask someone "is it perfectly clear what this does?".

But it could also be useful to express why 10 was chosen as the startCount etc. Why 10? Why not 15? Why not 100? Why should the developer care about how many instances should be created initially?

// Does it help? Not academically, but practically. 
// If not, what's the point?
const int typicalNumAsteroidsInGame = 10;
const int destroyExcess = 20;

// shortened variable name to fit on screen
bigPool = new ObjectPool(prefab, parent, typicalNumAsteroidsInGame, destroyExcess);

I guess you could try to keep in the back of your mind;

• Who am I solving the problem for?
  • The compiler?
  • Unity?
  • Myself?
  • The public, who never have seen my code before?
  • People attending a conference I am presenting at?
  • Someone I want to hire me?
• Why is it a problem? (Is it a problem?)
  • Do I have to do it, to make the program run?
  • Do other code depend on the literal?
  • Am I getting lost in my own code?
  • Am I causing bugs that would been easier to fix?
  • Do I want people to pay interest in my code?
  • Will people find their way around the code easily?
  • Will people get my presentation?
  • Without lengthy questions at end?
  • Will I remember what the third integer literal someone asked about represents?
  • Am I wasting time on things I shouldn't need to waste time on?
  • Would employers value programmers who write code the employer can understand?
• Which solution applies best, given above?

The list is not exhaustive, but the idea is; know your reason for removing literals. Don't remove them for the sake of removing them, I guess I want to say.

At least, that's my two cents.

// How about this?
// Easily fit horizontally.

pool = new ObjectPool(prefab);
pool.container = asteroids.transform; // Now optional, and has a name
pool.excess = 20;                     // Now optional, and has a name
pool.Preallocate(10);                 // Now optional, and has a name

[antfarmar]

Thanks for the thoughtful comment. I appreciate it. You bring up a lot of good points to keep in mind, as well as solutions.

Who am I solving the problem for? Why is it a problem? (Is it a problem?)

I guess readable code benefits everybody. Especially in a team. But if you're too good at it, you'll lose some job security since it's so easy to understand :smile:. Code commenting and now even GitHub seem to help quite a lot with these issues too. You can document almost anything easily now.

But most importantly I think it should benefit yourself (the coder). Even if it's a personal project, and you end up dropping it for awhile, when you come back to it you want it to be in good shape since you're going to forget many parts of it, or why you did it:

[ghost]

But if you're too good at it, you'll lose some job security since it's so easy to understand

It's funny we live in a world where being "worse" is an asset. At least from the bright side, for the next job interview it wouldn't hurt to say "I was overqualified for the role". :)

[antfarmar]

Replaced magic number in AsteroidBehaviour.cs to a named variable: float collisionSphereRadius = 5f;

May later reopen issue to expose pool counts in the Inspector with [Range] attributes as well.