Jump to content
  • Advertisement
povilaslt2

MY Space Shooter code review

This topic is 472 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Advertisement

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.

Share this post


Link to post
Share on other sites
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?

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!