Jump to content
  • Advertisement
Sign in to follow this  
Sadness

Question about my programming technic

This topic is 5466 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

This is the start of a game (tetris clone) I've made some times ago. Since I'm still learning, and I want to improve my coding skill, I would like some comment on this little things. Any feedback or critizism is very welcome ! :) PS : Feel free to point me any error I've made.
#include <iostream>
#include <cstdlib>
#include <vector>
#include "main.h"
#include "SDL/SDL.h"
#include "SDL/SDL_image.h"
#include "SDL/SDL_ttf.h"

#ifndef FALSE
const int FALSE = 0;
#endif
#ifndef TRUE
const int TRUE = 1;
#endif

// define the block size (X pixel)
const Uint8 BSize = 20;

// define the coord. where the xy matrix start (220px by 58px)
const Uint8 XMatrix = 220;
const Uint8 YMatrix = 58;

// define the size of the matrix in block
const Uint8 WMatrix = 10;
const Uint8 HMatrix = 20;

using namespace std;


SDL_Surface* Screen = NULL; // The main screen Surface
SDL_Surface* BMatrix = NULL; // Surface where we blit the stock
SDL_Surface* SMatrix = NULL; // A copy of the matrix Screen
SDL_Surface* Background = NULL; // The background image
SDL_Surface* Block[7]; // The block 
SDL_Surface* Temp = NULL; //Temp surface for SDL_DisplayFormat();

SDL_Rect Src_Rect, Dest_Rect;

SDL_Event event;

Uint8* kbarray;

int FPS = 0;
Uint32 TTime=SDL_GetTicks(); // Ticks Time

int UPDATE = 1;

// Matrix or grid. A place to store data about the fallen block
vector< vector<Uint32> > Matrix; // We store the block fallen here
vector< vector<Uint32> > TempMatrix; // Here's the falling block

int main(int argc, char* argv[])
{
  
  if( SDL_Init(SDL_INIT_VIDEO) ==-1)
    {
      cerr << "Can't initialize SDL, " << SDL_GetError() << endl;
      exit(-1);
    }
  
  //Nice little title
  SDL_WM_SetCaption("KKtrix 0.0.1b", NULL);

  if (SDL_InitSubSystem(SDL_INIT_TIMER) ==-1)
    {
      cerr << "Can't initialize SDL timer, " << SDL_GetError() << endl;
    }
  
  
  // Cleaning SDL
  atexit(SDL_Quit);
  
  // Should do a way to change setting on the fly or at least read
  // from a file
  Screen = SDL_SetVideoMode(640, 480, 16, SDL_HWSURFACE);
  if (!Screen)
    {
      cerr << "Can't set videomode, " << SDL_GetError() << endl;
      exit(-1);
    }
  
  // Load the different graphic here
  Background = IMG_Load("graphic/background.jpg");
  if(!Background)
    {
      cerr << "Can't load background image, " << SDL_GetError() << endl;
      exit(-1);
    }
  
  Background = Temp = SDL_DisplayFormat(Background);
  
  // Load in all the Block
  for(int a=1;a<7;a++)
  {
    char temp[100];
    sprintf(temp,"graphic/block%d.jpg",a);
    Block[a-1]=IMG_Load(temp);
    if(!Block[a-1])
    {
      cerr << "Can't load block" << a << ".jpg : " << SDL_GetError() << endl;
    }
    Temp = SDL_DisplayFormat(Block[a-1]);
    Block[a-1] = Temp;    

  }

  SMatrix = IMG_Load("graphic/matrix.jpg");
  if(!SMatrix)
    {
      cerr << "Can't load matrix image, " << SDL_GetError() << endl;
    }
  Temp = SDL_DisplayFormat(SMatrix);
  SMatrix = Temp;
  
  // Since we blit all the block to BMatrix, we keep a clean version
  // Of the Matrix graphic in SMatrix.
  BMatrix = SMatrix;
  BMatrix = Temp = SDL_DisplayFormat(BMatrix);
  
  // Display the background image
  Src_Rect.w=Dest_Rect.w=Background->w;
  Src_Rect.h=Dest_Rect.y=Background->h;
  Src_Rect.x=Src_Rect.y=0;
  Dest_Rect.x=Dest_Rect.y=0;
  SDL_BlitSurface(Background, &Src_Rect, Screen, &Dest_Rect);
  SDL_Flip(Screen);
  
  InitMatrix();
  
  GarbageMatrix(12);
  //Main loop.
  for(;;)
    {
      if(SDL_PollEvent(&event) != 0)
	{
	  if(event.type==SDL_QUIT) break;
	  if(event.type==SDL_KEYDOWN)
	    {
	      kbarray=SDL_GetKeyState(NULL);
	      if(kbarray[SDLK_ESCAPE]==1) break;
	      if(kbarray[SDLK_DOWN]==1) UPDATE = 1;
	    }
	}
      if(SDL_GetTicks()-TTime>1000)
	{
	  cout << FPS << endl;
	  FPS = 0;
	  TTime = SDL_GetTicks();
	}
      else
	{
	  FPS++;
	}
      CheckClearedLine();
      DrawBlock();
      FPS++;
    }
  exit(0);
  
}

void InitMatrix()
{
  cout << "Initializing the Matrix" << endl;
  Matrix.resize(HMatrix);
  for(Uint32 i = 0; i<Matrix.size(); i++)
    {
      Matrix.resize(WMatrix);
      for(Uint32 j = 0; j<Matrix.size(); j++)
	{
	  Matrix[j] = 0;
	}
    }
  TempMatrix.resize(HMatrix);
  for(Uint32 i = 0; i<TempMatrix.size(); i++)
    {
      TempMatrix.resize(WMatrix);
      for(Uint32 j = 0; j<TempMatrix.size(); j++)
	{
	  TempMatrix[j] = 0;
	}
    }
}

void ResetMatrix()
{
  for(Uint32 i = 0; i<Matrix.size(); i++)
    {
      for (Uint32 j = 0; j<Matrix.size(); j++)
	{
	  Matrix[j] = 0;
	}
    }
}

//Reverse the order of the scan. Since we want the block
//At the bottom.
void GarbageMatrix(int a)
{
  //cout << "Garbaging the matrix" << endl;
  for(Uint32 i = Matrix.size() -1 ; a != 0; i--)
    {
      for(Uint32 j = 0; j<Matrix.size(); j++)
	{
	  if(rand()%2 == 0)
	    {
	      Matrix[j] = rand()%8;
	    }
	}
      a--;
    }
}

void DrawBlock()
{
  //cout << "Drawing Block" << endl;
  if(UPDATE != 0)
    {
      int x = 0;
      int y = 0;
      SDL_Rect Dest, Src;
      SDL_BlitSurface(SMatrix, NULL, BMatrix, NULL);
      for(Uint32 i = 0; i<Matrix.size(); i++)
	{
	  for(Uint32 j = 0; j<Matrix.size(); j++)
	    {
	      if(Matrix[j] != 0)
		{
		  int b = Matrix[j] - 1;
		  Src.x=Src.y=0;
		  Src.h=Src.w=BSize;
		  Dest.x=x;
		  Dest.y=y;
		  SDL_BlitSurface(Block, &Src, BMatrix, &Dest);
		}
	      x += BSize;
	    }
	  x = 0;
	  y += BSize;
	}
      Src.x=Src.y=0;
      Src.w = WMatrix * BSize;
      Src.h = HMatrix * BSize;
      Dest.x=XMatrix;
      Dest.y=YMatrix;
      SDL_BlitSurface(BMatrix, &Src, Screen, &Dest);
      SDL_Flip(Screen);
      UPDATE = 0;
    }
}

// Function that check each line, if a line have 10 block, it's a full
// one and it have to be cleared( hence the call to DropLine(). We then
// increment Line. And we return this value for score function and msg
// function.
int CheckClearedLine()
{
  //cout << "Check Cleared Line" << endl;
  int Line = 0;
  for(Uint32 i = 0; i<Matrix.size(); i++)
    {
      int b = 0;
      for(Uint32 j = 0; j<Matrix.size(); j++)
	{
	  if(Matrix[j] == 1)
	    {
	      ++b;
	      if(b == 10)
		{
		  DropLine(i);
		  Line++;
		}
	    }
	}
    }
  return Line;
}

// Simple recursive algo. It goes up in the matrix until it hit the first row
// and move down each line by one row
void DropLine(int i)
{
  //cout << "Doing DropLine" << endl;
  int a = i - 1;
  if(i != 0)
    {
      for(Uint32 j = 0; j<Matrix.size(); j++)
	{
	  Matrix[j] = Matrix[a][j];
	}
      i--;
      DropLine(i);
    }
  else
    {
      for(Uint32 j = 0; j<Matrix.size(); j++)
	{
	  Matrix[0][j] = 0;
	}
      
    }
}

// Function mostly to test other thing or debug
void CompleteLine(int i)
{
  for(Uint32 j = 0; j<Matrix.size(); j++)
    {
      Matrix[j] = 1;
    }
}

// Again for debugging purpose only
void PrintMatrix()
{
  for(Uint32 i = 0; i<Matrix.size(); i++)
    {
      cout << i << ":";
      for(Uint32 j = 0; j<Matrix.size(); j++)
	{
	  cout << Matrix[j] << " ";
	}
      cout << endl;
    }
}


void SwitchMusic();
void PlayMusic(); 
void InitMatrix();
void ResetMatrix();
void GarbageMatrix(int);
void DrawBlock();
int CheckClearedLine();
void DropLine(int);
void CompleteLine(int);
void PrintMatrix();

Share this post


Link to post
Share on other sites
Advertisement
Use bool false and true instead of defining macros. bool is a valid type in C++ (which you are using) and C99. It looks as though you learned C89 first -hence these #defines.

With regard to debug statements, make a const bool debug = true; for when you are debugging and const bool debug = false; when you aren't. Then around your debug statements put:

if(debug)
cout << ...;

This is described in Stephen Dewhurt's C++ Gotchas as Gotcha #27.

You have an awful lot of Globals. This is described as Gotcha #3.
You have a lot of "magic numbers" (where you loop while i < 7 -what does 7 mean?). This is Gotcha #2
You comment excessively:

// define the block size (X pixel)
const Uint8 BSize = 20;

Obviously if you called this variable BlockSize, it would be very obvious that you were defining the block size.

Your code might work fine, but if you want to start making bigger games and working with other people, there is a bit of work to be done.

Which books have you read so far? Some people on these forums might be able to suggest some more books for you to improve your coding with.

Share this post


Link to post
Share on other sites

#include <cstdlib>


Do you need it? Avoid the c-style libraries in modern C++ if you can.


#ifndef FALSE
const int FALSE = 0;
#endif
#ifndef TRUE
const int TRUE = 1;
#endif


OK, but this won't actually 'define' FALSE and TRUE as the preprocessor sees it. May be a problem later on when you have more source files.

Also, unnecessary. C++ provides a 'bool' type.


// define the block size (X pixel)
const Uint8 BSize = 20;

// define the coord. where the xy matrix start (220px by 58px)
const Uint8 XMatrix = 220;
const Uint8 YMatrix = 58;

// define the size of the matrix in block
const Uint8 WMatrix = 10;
const Uint8 HMatrix = 20;




There is probably no good reason to worry about the size of these values, even though you do know that a Uint8 is big enough.


SDL_Surface* Screen = NULL; // The main screen Surface
SDL_Surface* BMatrix = NULL; // Surface where we blit the stock
SDL_Surface* SMatrix = NULL; // A copy of the matrix Screen
SDL_Surface* Background = NULL; // The background image
SDL_Surface* Block[7]; // The block
SDL_Surface* Temp = NULL; //Temp surface for SDL_DisplayFormat();
SDL_Rect Src_Rect, Dest_Rect;

SDL_Event event;

Uint8* kbarray;

int FPS = 0;
Uint32 TTime=SDL_GetTicks(); // Ticks Time

int UPDATE = 1;

// Matrix or grid. A place to store data about the fallen block
vector< vector<Uint32> > Matrix; // We store the block fallen here
vector< vector<Uint32> > TempMatrix; // Here's the falling block




That's a lot of globals. :/ Also, the '7' as a magic number stands out. I assume that's supposed to be the number of distinct Tetris block types. Give that a name (and a const int).

The SDL init stuff from main could probably be wrapped up in a class. You might for example give it a static 'init' method, and then have the instances basically be wrappers for those SDL surfaces. Just brainstorming though. I don't know SDL either.

Some of those could be local to main(), too. For example, TTime. Noone else needs to know about that, you're just using it to calculate FPS.


// Load in all the Block
for(int a=1;a<7;a++)
{
char temp[100];
sprintf(temp,"graphic/block%d.jpg",a);
Block[a-1]=IMG_Load(temp);
if(!Block[a-1])
{
cerr << "Can't load block" << a << ".jpg : " << SDL_GetError() << endl;
}
Temp = SDL_DisplayFormat(Block[a-1]);
Block[a-1] = Temp;

}




Ugh ugh ugh. Please become more comfortable with the 0-based indexing, and don't fight it. Here, make a start at 0, and then you can get rid of the -1 subtraction. (You'll have to rename your .jpg's too.)

Here I see what you're using cstdlib for, with the image names. Here is how it's done in modern C++: (this is probably buggy, someone will correct it I'm sure)


#include <sstream>

for(int a=0;a<NUM_BLOCKTYPES;a++) {
sstream temp("graphic/block");
temp << a << ".jpg";
// grab a std::string from the stringstream,
// then grab the char * buffer from that
// and feed it to SDL
Block[a]=IMG_Load(temp.str().c_str());
// ... do the rest
}




Oh, if that error condition occurs, you might want to bail out.


InitMatrix();

GarbageMatrix(12);


Fine for now, but I would think you would want to allow for fewer than 12 lines of "garbage". Not everyone is expert :) Also, you could combine those two functions, I'll get to that later...


for(;;)
{
if(SDL_PollEvent(&event) != 0)
{
if(event.type==SDL_QUIT) break;
if(event.type==SDL_KEYDOWN)
{
kbarray=SDL_GetKeyState(NULL);
if(kbarray[SDLK_ESCAPE]==1) break;
if(kbarray[SDLK_DOWN]==1) UPDATE = 1;
}
}




The way the UPDATE global is used is rather ugly. Use a local variable, and pass it as a parameter to DrawBlock.


if(SDL_GetTicks()-TTime>1000)
{
cout << FPS << endl;
FPS = 0;
TTime = SDL_GetTicks();
}
else
{
FPS++;
}
CheckClearedLine();
DrawBlock();
FPS++;
}
exit(0);
}




Oops, you're incrementing FPS twice on most iterations, and getting a false impression of the speed :)


void InitMatrix()
{
cout << "Initializing the Matrix" << endl;
Matrix.resize(HMatrix);
for(Uint32 i = 0; i<Matrix.size(); i++)
{
Matrix.resize(WMatrix);
for(Uint32 j = 0; j<Matrix.size(); j++)
{
Matrix[j] = 0;
}
}
TempMatrix.resize(HMatrix);
for(Uint32 i = 0; i<TempMatrix.size(); i++)
{
TempMatrix.resize(WMatrix);
for(Uint32 j = 0; j<TempMatrix.size(); j++)
{
TempMatrix[j] = 0;
}
}
}




Duplication is bad. You do need to initialize two different matrices, and there's probably nothing to be gained from collapsing them into the same array (o_O), but you can do without repeating the loop structure. Also, as promised, you can fit in the "garbaging" here:


void InitMatrix(int garbageAmount) {
cout << "Initializing the Matrix" << endl;
Matrix.resize(HMatrix);
TempMatrix.resize(HMatrix);
// The compiler may or may not be able to do the optimization
// of calculating Matrix.size() only once. But there's no real
// reason to do it at all; we know what height and width we
// want...
for(Uint32 i = 0; i<HMatrix; i++) {
Matrix.resize(WMatrix);
TempMatrix.resize(WMatrix);
for(Uint32 j = 0; j<WMatrix; j++) {
if((WMatrix - j <= garbageAmount) &&
(rand()%2 == 0)) {
// Check if we're in the bottom n-many lines; if so,
// check if we want garbage.
// There are better (more random) ways to get the
// random value than with integer %, but I'll let it
//slide...
Matrix[j] = rand()%8;
} else {
Matrix[j] = 0;
// If we're too near the top, or the coin toss failed,
// then initialize as empty.
}
TempMatrix[j] = 0;
// don't put any garbage in the temp matrix.
}
}
}




What's next... some more drawing, yadda yadda. I just wanted to make a comment that having a return at the beginning of the function is fine; I prefer


void foo(bool reallyFoo) {
if (!reallyFoo) return;
// lots of stuff
}


instead of


void foo(bool reallyFoo) {
if (reallyFoo) {
// lots of stuff
// all of which has to be indented one more level.
}
}


Anyway. Next up is CheckClearedLine.

There is a cleaner way to do the inner loop. That '10' looks suspicious; what you really want to do is check that *all* blocks in the line are full, and you don't really want to worry about how many blocks that is. Thus:


bool flag = false;
for (int i = 0; i < HMatrix; i++) {
for (int j = 0; j < WMatrix; j++) {
if(Matrix[j]) {
// i.e., if it's not zero. Notice that you allow for
// values 0-7 because of how you do your garbage filling.
// I assume they're supposed to represent colours? :s
flag = true;
break; // optional; a tiny bit faster, a tiny bit uglier.
}
}
if (flag) {
// found a complete line.
}
flag = false; // reset for next row.
}




Last up is DropLine. (the rest is debugging stuff and not worth picking at, not that I see anything wrong with it anyway. :) ) Again you have redundant looping; the problem here is failing to isolate the check on 'i' to where it matters.


void DropLine(int i) {
//'a' was used only in one place, so I got rid of it. It
// doesn't really clarify things.
for(Uint32 j = 0; j<WMatrix; j++) {
Matrix[j] = (i == 0) ? 0 : Matrix[i-1][j];
// I used the ternary operator because the basic *procedure*
// is the same for both cases of the if; just the *value*
// changes. There are people who will yell at you for using
// it basically at all, but I think this sort of use is
// quite neat.
// However, having to check i is still kind of ugly. Check
// the vector ".at()" member function, I think you can supply
// a default value.
// Or failing that, you could add a dummy row to the Matrix
// which is always empty...
}
if (i != 0) DropLine(i-1); // do the recursion.
}




Oh, it's easy enough to write that non-recursively, too, but it's probably just as well that you take the chance to familiarize yourself with recursion :)

BTW, you probably noticed I mentioned avoiding redundant loops a few times. You probabaly also noticed that a lot of the algorithms you have to work with deal with looping through the vector. If you're feeling adventurous, you might want to look into the <algorithm> header and see what it can do for you ;)

(P.S. The board's inability to keep me logged in is getting REALLY annoying :( )

Share this post


Link to post
Share on other sites
Quote:
Original post by flangazor
Which books have you read so far? Some people on these forums might be able to suggest some more books for you to improve your coding with.


I have an old C pocket book, I got some basic understanding of programmation with it. And I own the Bjarne stroustrup C++ Language, special edition. Else I've read a lot of thing on internet. Checked some application source code, but most of them are more advanced stuff so.

But thanks for the input, I'll try to look at that (Even if some things you said are not clear presently, and thanks for the book suggestion, I should get it soon.

Quote:
Original post by Zahlman
Here I see what you're using cstdlib for, with the image names. Here is how it's done in modern C++: (this is probably buggy, someone will correct it I'm sure)

#include <sstream>

for(int a=0;a<NUM_BLOCKTYPES;a++) {
sstream temp("graphic/block");
temp << a << ".jpg";
// grab a std::string from the stringstream,
// then grab the char * buffer from that
// and feed it to SDL
Block[a]=IMG_Load(temp.str().c_str());
// ... do the rest
}



Thanks, I did search for a way to do it in C++, even asked on a newsgroupd dedicated to c++ programming, and I didn't have a answer other than to use the C lib. Thanks for the input too.

:)

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

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

GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry.

Sign me up!