Advertisement

Tetris Clone Code Review Request

Started by July 13, 2017 01:56 PM
3 comments, last by Kylotan 7 years, 4 months ago

Hello! I have made a clone of Tetris using SDL libraries for C++. I would love to get some input as to what I have done right and wrong. Also please tell me if im using Github correctly, it confuses me greatly. (I tried to add a new commit to remove the misplaced header file "DungeonCrawlV4.o" but it doesn't seem to have taken place.)

https://github.com/Starfruit64/Tetris-Clone

Quick thoughts (some of which I think I mentioned to you last time we looked at this code):

  • there's no need to have a single, shared SDL_Event for everything to use - it can be a local object inside the function that polls for them.
  • it seems a bit pointless to abstract away the SDL input messages in a program this small, but if you prefer to do this, I'd suggest using enums rather than strings like "a_d" which are more error-prone and also less efficient to pass around. This also allows you to use switch/case instead of the uglier chains of if-elses.
  • I don't recommend wrapping your SDL_PollEvent call inside an input class because SDL events aren't all about player input.
  • Normally, instead of passing a graphics object (e.g. Renderer) to a game logic object (e.g. Board), you do it the other way around. This is because you don't usually want your logic objects to have to worry about details such as how they get rendered.
  • different gamestates are better represented with enums than with integers and comments explaining the integers.
  • you should consider making accessors like 'return_pos' const
  • you should probably add comments in your header files to say what each class does and what each member does
  • explicitly initialise variables when you create them, e.g. to nullptr
  • be consistent with your naming - you seem to use 'tetro' and 'tetri' interchangeably.
  • consider separating your game loop out into clear input/update/render functions. You might need to have some extra mechanism telling you when to render, since (unlike most games) you don't render every frame.
Advertisement
1 hour ago, Kylotan said:

Quick thoughts (some of which I think I mentioned to you last time we looked at this code):

  • there's no need to have a single, shared SDL_Event for everything to use - it can be a local object inside the function that polls for them.
  • it seems a bit pointless to abstract away the SDL input messages in a program this small, but if you prefer to do this, I'd suggest using enums rather than strings like "a_d" which are more error-prone and also less efficient to pass around. This also allows you to use switch/case instead of the uglier chains of if-elses.
  • I don't recommend wrapping your SDL_PollEvent call inside an input class because SDL events aren't all about player input.
  • Normally, instead of passing a graphics object (e.g. Renderer) to a game logic object (e.g. Board), you do it the other way around. This is because you don't usually want your logic objects to have to worry about details such as how they get rendered.
  • different gamestates are better represented with enums than with integers and comments explaining the integers.
  • you should consider making accessors like 'return_pos' const
  • you should probably add comments in your header files to say what each class does and what each member does
  • explicitly initialise variables when you create them, e.g. to nullptr
  • be consistent with your naming - you seem to use 'tetro' and 'tetri' interchangeably.
  • consider separating your game loop out into clear input/update/render functions. You might need to have some extra mechanism telling you when to render, since (unlike most games) you don't render every frame.

About events: should I remove the Input class and have all events handles in Main in a function? Or should I have multiple times where I poll for events, one time for input, one time for window events, etc.?

I recommend handling all the events in your main loop, and passing them on to whichever subsystem - e.g. Input - can handle them.

This topic is closed to new replies.

Advertisement