Advertisement

MY Space Shooter code review

Started by June 29, 2017 03:15 PM
3 comments, last by Kylotan 7 years, 5 months ago

Hello, I'm developing space shooter game and need yours advice on code. What is good/bad and etc. All elements, enemies, UI are placeholders at this moment.

https://github.com/Povilas-Jablonskis/Space-Shooter

Some quick comments:

  • _BaseGameObject is strictly speaking not allowed because your compiler reserves the use of all identifiers that start with an underscore followed by a capital letter
  • Prototypes with no parameter names, e.g. BaseGameObject(int, int, float, float, float, float, float, float, float), are not very good documentation for what the arguments are meant to do.
  • accessors/"getters" should normally be marked as const
  • I would also encapsulate geometric points and vectors into a struct or class, rather than staying with float[2]
  • It's better in a constructor to initialise data in a member initialiser list rather than in the function body, where possible.
  • Write some comments.
  • Overuse of static (e.g. in BulletManager) is usually a bad sign because it implies that you have a global variable 'dressed up' as a static. Consider making these members non-static and ensure the object's lifetime is carefully controlled.
  • Similarly, try to avoid globals. Ideally you wouldn't have any. Certainly there's no need for most of them to exist. For example, most of the globals in main.cpp could exist in a Game class (although making that play nicely with glut might be awkward, I don't know)
  • When passing std::string, consider passing it by const reference instead. This can avoid some unnecessary copying.
  • The map loop in Player::Update() doesn't look like it would compile, never mind work. There's an extra semi-colon in there. Inside the loop you're erasing elements which can invalidate iterators, so the loop could crash. Besides, if you want to clear a map, use clear.
  • 'magic numbers' in the code, like the 6 in "position[0] -= 6.0f" are a bad idea, because the reader doesn't know what they mean. It's even worse when the number is repeated (as it is here) because any future change has to be done in every separate place. Instead, create a constant with a meaningful name somewhere, and refer to that where you currently have the number.
Advertisement
14 hours ago, Kylotan said:

Some quick comments:

  • Overuse of static (e.g. in BulletManager) is usually a bad sign because it implies that you have a global variable 'dressed up' as a static. Consider making these members non-static and ensure the object's lifetime is carefully controlled.
  • Similarly, try to avoid globals. Ideally you wouldn't have any. Certainly there's no need for most of them to exist. For example, mMost of the globals in main.cpp could exist in a Game class (although making that play nicely with glut might be awkward, I don't know)

BulletManager is used to create/update/render bullets. Bullets can be added from player or other class. So I have to create new instance in every class that uses? or just pass reference when updating?

FontLoader is used to load/use fonts in a lot of classes. Should I make just make reference of it in main.cpp?

Renderer is used to render objects. Should I also make instance of it in main.cpp?

 

14 hours ago, Kylotan said:

Some quick comments:

  • 'magic numbers' in the code, like the 6 in "position[0] -= 6.0f" are a bad idea, because the reader doesn't know what they mean. It's even worse when the number is repeated (as it is here) because any future change has to be done in every separate place. Instead, create a constant with a meaningful name somewhere, and refer to that where you currently have the number.

 

What should you offer? To add constants to Player and etc classes?

Regarding statics and globals: passing a reference to something when updating is fine. It's better to be explicit than implicit, and that means it's better to pass something in if you might need it, than to make it globally accessible 'just in case'. Similarly, it's better to make an instance of something and say "this is the instance of X that we will be using" rather than having static functions and data which basically implement a magic shared instance.

Normally, I would have a 'Game' or 'Application' class that can contain widely-used members like BulletManager, FontLoader, and Renderer. You'd create a single one of those and then it can pass whatever is needed to wherever it is needed.

Regarding constants - like any variable, they should be declared where they're needed. If you only need them in that function, declare them there. If you need them throughout a class, declare them in a class. If you need them throughout a project, consider putting them in a new header file and #including that wherever you need.

This topic is closed to new replies.

Advertisement