Help me improve please :)

witnessmenow

Hey there,

I'm looking to improve my programming. I'm in 4th year electronic and computer engineering and i really feel my programming isn't at the standard it should be at.

Here is an assignment that was due for last Friday that i handed up already, and i was wondering if you could tell me where i could improve it.

//#############################
// Alien Invasion 1.0
//
// A 2D game using openGL
//
// Witnessmenow
// 
// 
// 
// 09/01/09
//
#include "stdafx.h"
#include <windows.h>
#include <stdlib.h>
#include <GL/glut.h>
#include <ctime> 

float yAxis = -.75;
float xAxis = 0;
float mainLeft, mainRight, mainTop, mainBottom ;
float middleLeft, middleRight, middleTop, middleBottom;
float gunLeft, gunRight, gunTop, gunBottom ;
float mainSize = 0.1;
float enemySize = 0.0375;
float enemyX = 0.9375, enemyY = 0.9375;
float previousenemyX = 0;
float enemyRight = 0, enemyBottom = 0;
float bulletLeft, bulletRight, bulletTop, bulletBottom;
float bulletX, bulletY = (yAxis + (mainSize/3)+(mainSize/4)+(mainSize/4));
float bulletSize = 0.01;
float laserSize = 0.03;
float laserLeft, laserRight, laserTop, laserBottom;

float playerSpeed = 0.05;
float enemySpeed = 0.004;
float bulletSpeed = 0.08;


int i = 0, j = 0;
int pointValue = 5;
int score = 0;
int selection = 0;
float level = 0;
int laserRefresh = 25;
int laserAmount = 0;
int rows = 5;
int refresh = 50;

void display();
void collision();
void animation();
void KeyDown(unsigned char key, int x, int y);
void reset();
void menu();
void gameOver();
void newLevel();

bool pointCollision = true;
bool back = false;
bool moveDown = false;
bool currentBullet = false;
bool alive[10][5];
bool currentLaser;
bool laser = true;
bool levelUp = false;

int main(int argc, char** argv)

{
    glutInit(&argc,argv);
    glutCreateWindow("Alien Invasion");
	laserAmount = laserRefresh;
	
	for ( j=0; j <10; j++)
	{			
		for (i=0; i <5; i++)
		{
			alive [i][j] = true;
		}
	}
	menu();
    glutDisplayFunc(display);
	glutKeyboardFunc(KeyDown);
    glutMainLoop();
	
}




void reset()
{
	yAxis = -.75;
	xAxis = 0;
	enemyX = 0.9375, enemyY = 0.9375;
	i = 0, j = 0;
	score = 0;
	laser = 10;
	for ( j=0; j <rows; j++)
	{			
		for (i=0; i <5; i++)
		{
			alive [i][j] = true;
		}
	}
	
}

void gameOver()
{
	printf("GAME OVER\n");
	printf("Your score = %d ",score);
	printf("\n\n\n");
	score = 0;
	reset();
	menu();
}

void newLevel()
{
	printf("\nNEW LEVEL\n");
	level = level + 1;
	score = score + 100;
	enemyX = 0.9375, enemyY = 0.9375;
	laserAmount = laserAmount + laserRefresh;
	for ( j=0; j <rows; j++)
	{			
		for (i=0; i <5; i++)
		{
			alive [i][j] = true;
		}
	}
}

void menu()
{
	printf("Welcome to Alien Invasion Please Select an option:\n");
	printf("1) Play Game\n");
	printf("2) About The Game\n");
	printf("3) Settings\n");
	printf("4) Quit\n");
	scanf("%ld",&selection);

	if (selection == 1)
	{
		//
	}
	else if (selection == 2)
	{
		
		printf("\n\nObjective:\n");
		printf("----------\n");
		printf("The object of this game is to shoot all the alien space ships before\nthey reach earth\n\n");
		printf("Controlls:\n");
		printf("----------\n");
		printf("Move Left   = A\n");
		printf("Move Right  = D\n");
		printf("Fire Gun    = W\n");
		printf("Fire Laser  = S\n");
		printf("Reset       = P\n\n");
		printf("Author:\n");
		printf("----------\n");
		printf("Brian Lough - 05530199\n");
		printf("4th ECE\n\n\n\n");
		menu();
	}
	else if (selection == 3)
	{
		printf("\n\nSettings:\n");
		printf("----------\n");
		printf("Here you can tinker with the settings of the game\nThe game will revert to default values the next time it is started\nPlease pick an item you want to modify\n");
		printf("1)Weapons \n");
		printf("2)Your Ship\n");
		printf("3)Enemy\n");
		printf("4)Return to Main Menu\n");
		scanf("%ld",&selection);

		if (selection == 1)
		{
			printf("\n\n Weapons \n");
			printf("----------\n");
			printf("Please note that high bullet speeds make hitting targets unstable\n");
			printf("Enter a value for bullet speed (Default = 0.08):");
			scanf("%f",&bulletSpeed);
			printf("\nEnter a value for the bullet's size (Default = 0.01):");
			scanf("%f",&bulletSize);
			printf("\nEnable laser, type 1 for yes(default = enabled)");
			scanf("%ld",&selection);
			if (selection == 1)
			{
				laser = true;
			}
			else
			{
				laser = false;
			}
			printf("Enter a value for the laser's size (Default = 0.03):");
			scanf("%f",&laserSize);
			printf("Enter a value for the amount the laser refreshs at the start of each level (Default = 25):");
			scanf("%ld",&laserRefresh);
			printf("\n\n");



		}
		else if (selection == 2)
		{
			printf("\n\n Your Ship \n");
			printf("----------\n");
			printf("Enter a value for ship's speed (Default = 0.05):");
			scanf("%f",&playerSpeed);
			printf("\nEnter a value for the ship's size (Default = 0.1):");
			scanf("%f",&mainSize);
			printf("\n\n");
			
		}
		else if (selection == 3)
		{
			printf("\n\n Enemy \n");
			printf("----------\n");
			printf("Enter a value for enemy's speed (Default = 0.004):");
			scanf("%f",&enemySpeed);
			printf("\nEnter a value for the enemy's size (Default = 0.0375):");
			scanf("%f",&enemySize);
			printf("\nEnter the amount of points gained for killing an enemy (Default = 5):");
			scanf("%ld",&pointValue);
			printf("\nEnter the number of rows of enemies that should appear on\neach level (Default = 5, max = 10):");
			scanf("%ld",&rows);
			if (rows > 10)
			{
				rows = 10;
			}
			if (rows < 1)
			{	
				rows = 1;
			}
			printf("\nEnter a screen refresh value\the lower the value the quicker the screen refreshes (Default = 50:");
			scanf("%ld",&refresh);
			printf("\n\n");
		}
		else if (selection == 4)
		{
			menu();
		}
		else
		{
			printf("Invalid Selection.\n\n\n");
			menu();
		}
		
		menu();

	}
	else if (selection == 4)
	{
		PostQuitMessage(0);
	}
	else
	{
		printf("Invalid Selection.\n\n\n");
		menu();
	}
}

void KeyDown(unsigned char key, int x, int y)
{
	switch (key) 
	{
	case 27:		//ESC
		PostQuitMessage(0);
		break;
	case 'a':
		xAxis = xAxis - playerSpeed;
		display();
		break;
	case 'd':
		xAxis = xAxis + playerSpeed;
		display();
		break;
	case 'w':		
		if (currentBullet == false)
		{
			currentBullet = true;
			bulletY = yAxis + (mainSize/3)+(mainSize/4)+(mainSize/4);
		}
		display();
		break;
	case 's':
		if (laser == true)
		{
			if (currentLaser == false)
			{
				currentLaser = true;
			}
			else
			{
				currentLaser = false;
			}
		}
		display();
		break;
	case 'p':
		reset();
		display();
		break;
	}

}

void animation()
{    	

	//Animating objects
	Sleep(refresh);

	previousenemyX = enemyX;
	if (back == false)
	{
		enemyX = enemyX - enemySpeed - (level*0.005);
	}
	else
	{
		enemyX = enemyX + enemySpeed + (level*0.005);
	}
	if (currentBullet == true)
	{
		bulletY = bulletY +  bulletSpeed;
	}
	if (currentLaser == true)
	{
		laserAmount = laserAmount - 2;
		if (laserAmount < 1)
		{	
			currentLaser = false;
		}

		laserTop = laserTop + 0.5;
		
		if (laserTop < 1)
		{
			laserTop = 1;
		}

	}
	
	glutPostRedisplay();
	

	
}

void collision()
{
	
	if (currentBullet == true)
	{//Checking if a bullet has hit the enemy 
		if((bulletLeft < enemyRight) && (bulletRight > (-enemyX -enemySize)) && (bulletTop > enemyBottom))
		{
			
			for ( j=0; j <rows; j++)
			{
			
				for (i=0; i <5; i++)
				{
					if ((alive[i][j] == true)&&(bulletRight > ((-enemyX - enemySize) + (i*.2)))&&(bulletLeft < ((-enemyX + enemySize) + (i*.2)) ))
					{									
						if ((bulletBottom < ((enemyY + enemySize) + (j*.2)))&& (bulletTop > ((enemyY - enemySize) + (j*.2))))
						{
							alive[i][j] = false;
							//printf("bump");
							score = score + pointValue;
							currentBullet = false;
							bulletY = gunTop;
						}					
					}
						
				}
				
			}
		}
	}
	//Checking if the laser has hit an enemy
	if (currentLaser == true )
	{
		if(( laserLeft < enemyRight) && ( laserRight > (-enemyX -enemySize)) && ( laserTop > enemyBottom))
		{
			
			for ( j=0; j <rows; j++)
			{
			
				for (i=0; i <5; i++)
				{
					if ((alive[i][j] == true)&&( laserRight > ((-enemyX - enemySize) + (i*.2)))&&( laserLeft < ((-enemyX + enemySize) + (i*.2)) ))
					{									
						if (( gunTop < ((enemyY + enemySize) + (j*.2)))&& ( 1 > ((enemyY - enemySize) + (j*.2))))
						{
							alive[i][j] = false;
							score = score + pointValue;
							
							
						}					
					}

				}
				
			}
		}
	}
	//Checking has the enemy hit the user's ship or the ground
	if(( mainLeft < enemyRight) && ( mainRight > (-enemyX -enemySize)) && ( gunTop > enemyBottom))
	{
		
		for ( j=0; j <rows; j++)
		{
		
			for (i=0; i <5; i++)
			{
				if ((alive[i][j] == true)&&( mainRight > ((-enemyX - enemySize) + (i*.2)))&&( mainLeft < ((-enemyX + enemySize) + (i*.2)) ))
				{									
					if (( mainBottom < ((enemyY + enemySize) + (j*.2)))&& ( gunTop > ((enemyY - enemySize) + (j*.2))))
					{
						
						gameOver();

						
					}
					else if ((enemyY - enemySize) + (j*.2) < -0.80)
					{
						gameOver();
				
					}
			
				}

			}
		}
	}

	//Checking are all enemies in this wave dead
	levelUp = true;
	for ( j=0; j <rows; j++)
	{
		for (i=0; i <5; i++)
		{
			if (alive[i][j] == true)
			{
				levelUp = false;
			}
		}
	}

	if (levelUp == true)
	{
		newLevel();
	}


}


void display()
{

	animation();
	collision();
	//Destroying the bullet once it goes above what is displayed so another can be fired
	if (bulletY > 1)
	{
		currentBullet = false;
		bulletY = gunTop;
	}
	//handling what the enemies do once they reach the eddge of the screen
	if (moveDown == false)
	{
		if(enemyRight > 1.0)
		{	
			moveDown = true;
			enemyX = previousenemyX;
			enemyY = enemyY - .2;
			back = true;
		}
		if((-enemyX -enemySize) < -1.0)
		{
			moveDown = true;
			enemyX = previousenemyX;
			enemyY = enemyY - .2;
			back = false;
		}
	}
	else
	{
		moveDown = false;
	}

	//Keeping the user inside the boundries of the screen
	if(xAxis > 1.0 - mainSize)
	{	
		xAxis = 1.0 - mainSize;
	}
	if(xAxis < -1.0 + mainSize)
	{	
		xAxis = -1.0  + mainSize;
	}
	
	
	//Main Body
	mainLeft = -mainSize + xAxis;
	mainRight = mainSize + xAxis;
	mainTop = (mainSize/3) + yAxis;
	mainBottom = (-mainSize/3) + yAxis;

	//Middle Body
	middleLeft = (-mainSize/1.5) + xAxis;
	middleRight = (mainSize/1.5) + xAxis;
	middleTop = ((mainSize/5) + (yAxis + ((mainSize/3)+(mainSize/5))));
	middleBottom = ((-mainSize/5) + (yAxis+ ((mainSize/3)+(mainSize/5))));

	//Gun Body

	gunLeft = (-mainSize/10) + xAxis;
	gunRight = (mainSize/10) + xAxis;
	gunTop = mainSize/4 + (yAxis + ((mainSize/3)+(mainSize/4))+(mainSize/4));
	gunBottom = -mainSize/4 + (yAxis + (mainSize/3)+(mainSize/4)+(mainSize/4));

	//Bullet
	bulletLeft = -bulletSize + bulletX;
	bulletRight = bulletSize + bulletX;
	bulletTop = bulletSize + bulletY;
	bulletBottom = -bulletSize + bulletY;

	//Laser
	laserLeft = -laserSize + xAxis;
	laserRight = laserSize + xAxis;
	laserTop = laserTop;
	laserBottom = gunTop;

	collision();

    glClear(GL_COLOR_BUFFER_BIT);
	
	//#################
	//Drawing the User controlled object
	//#################

	glBegin(GL_POLYGON);
	glColor3f(0,1,0);
	glVertex2f(mainLeft , mainTop);
    glVertex2f(mainLeft , mainBottom);
    glVertex2f(mainRight , mainBottom);
    glVertex2f(mainRight , mainTop );
	glEnd();
    glFlush();
	//Draw Middle
	glBegin(GL_POLYGON);
	glColor3f(0,1,0);
	glVertex2f(middleLeft , middleTop);
    glVertex2f(middleLeft , middleBottom);
    glVertex2f(middleRight , middleBottom);
    glVertex2f(middleRight , middleTop );
	glEnd();
    glFlush();
	//Draw Gun
	glBegin(GL_POLYGON);
	glColor3f(0,1,0);
	glVertex2f(gunLeft , gunTop);
    glVertex2f(gunLeft , gunBottom);
    glVertex2f(gunRight , gunBottom);
    glVertex2f(gunRight , gunTop );
	glEnd();
    glFlush();


	//##############
	//Drawing Enemy
	//##############
	for ( j=0; j <rows; j++)
	{
		for (i=0; i <5; i++)
        {
			if (alive [i][j] == true )
			{
				glBegin(GL_POLYGON);
				glColor3f(1,0,0);
				glVertex2f((-(enemyX - enemySize) + (i*.2)), ((enemyY + enemySize) + (j*.2)));
				glVertex2f((-(enemyX - enemySize) + (i*.2)), ((enemyY - enemySize) + (j*.2)));
				glVertex2f((-(enemyX + enemySize) + (i*.2)), ((enemyY - enemySize) + (j*.2)));
				glVertex2f((-(enemyX + enemySize) + (i*.2)), ((enemyY + enemySize) + (j*.2)));
				glEnd();
				glFlush();
			}
			enemyRight = i*.2 + ((-enemyX) + enemySize);
        }
		enemyBottom = -j*.2 + enemyY - enemySize;
	}

	//############
	//Drawing Bullet
	//############
	
	if (currentBullet == true)
	{
		glBegin(GL_POLYGON);
		glColor3f(0,0,1);
		glVertex2f(bulletLeft , bulletTop);
		glVertex2f(bulletLeft , bulletBottom);
		glVertex2f(bulletRight , bulletBottom);
		glVertex2f(bulletRight , bulletTop );
		glEnd();
		glFlush();
	}
	else
	{
		bulletX = xAxis;
	}

	//#############
	//Drawing laser :)
	//#############
	if (currentLaser == true)
	{
		glBegin(GL_POLYGON);
		glColor3f(0,0,1);
		glVertex2f(laserLeft , laserTop);
		glVertex2f(laserLeft , gunTop);
		glVertex2f(laserRight , gunTop);
		glVertex2f(laserRight , laserTop );
		glEnd();
		glFlush();
	}
	else
	{
		laserTop = gunTop;
	}


	collision();
	
}

Any advice would be much appreciated

codecrunchers

not sure if this is helpful, but something like this in which the components have state may be more suited to an OO language i.e. C++ - this would allow you to reuse parts of your code, and extend the functionality with more ease.

Alan

toiletduck

Heh, didn't go for the VRML eh?

bpmurray

Global variables should not be used unless you have to - try to follow the OO concepts of keeping stuff together, so each function can be reused - you pass in the variables as parameters. For example, i and j are used as indexes in for-loops, and should be local to the functions.

Use shortcuts like += and ++ since they make the code more readable.

Use manifest constants rather than literals, i.e. use #define.

You could use doubles rather than floats (consistent size), but that's not really an issue.

However, EVERY function should have a block of comments above it to explain what it does and why. Long functions should have blocks of comments to explain what it's doing. And there should be comments on most lines.

Webmonkey

Hows it going B. Just few things noticed...

* In the menu function id use a do while loop and switch statements. Easier to read.

* (alive [j] == true) same as if(alive[j]) so...

* You are doing OpenGL C programming, not C++ - no such thing as true/false in C. Can #define those though. Will work most time since a C++ compiler you using.

* calling glColor3f(0,1,0) again and again when using same colour is inefficient. Call it once before the glBegin(GL_POLYGON) and draw away and when want color to change, call again. Remember OpenGL is a state machine as such so once set it stays

* no need for a glFlush() until end, afaik it is inefficient to call it multiple times as code in between flushes is quite small and will have no bother sending whole function to GPU fast enough. I notice you've flushes all over the gaf, only do it once at end of display function.

* Use OpenGL types such as GLdouble GLint instead of floats etc. This will make your code cross platform independent as different architectures may have a different underling type for GLDouble - you replying on casting.

* If you have only one statement in an if or else you don't need to surround with braces {} but some recommend you do as it makes code easier to read.


* Your animation function is set up wrong. What you should do is in the main method where you set up other callbacks like displayFunc, keyboardFunc set up a glutTimerFunc callback passing refresh in as the time so basically. glutTimerFunc(refresh,animation,0); and set up animation signature/header as void animation(int value) and inside the animation function as last line call itself recursively again the refresh time later. I.E:

void animation(int value)
{
//Remove sleep!
//Rest code
glutTimerFunc(refresh,animation,0);
}

* If I remember correctly, Sleep() is part of the windows.h library. Not good to be depending on that, only work on windows!

* multi line comments can be done using /* */ but technically these were never part of standard C library but most compilers won't have a problem with them.
/* ==============
Drawing bullet
==============
*/

* Your code should not reply as you not returning "return 0" at end of main

* No such thing as Bool in C

* Use #defines to specify constants above rather than global variables

* Do NOT USE GLOBALS unless really needed (callbacks etc) - for example, those int i's and int j's - not good having them up as globals!

* If you want to do proper OpenGL you better set up proper buffers etc so:
glutInitDisplayMode(GLUT_DOUBLE | GLUT_RGB);

If you doing quick animation and it flickering etc, you better off using GLUT_DOUBLE instead of GLUT_SINGLE but you will have to do a glutSwapBuffers() after you finish drawing (end of display()) so that you swap the drawing buffer with the drawn one if that makes sense. Anyways will look much better if you have animation!

fergalr

Just the first thing I noticed - you might want to remove your name and student number from the source?
Apologies if you know this and don't care - just thought I'd point it out in case you missed it.

witnessmenow

Thanks for the replies everyone, I''ll take a better look at implementing them when i get some time at the weekend

EDIT: RE globals:
I take your point with the i's and j's but are they ok for say my "mainSize" (which the user can change the value and it is used in nearly all sections of the code)?

toiletduck wrote: »

Heh, didn't go for the VRML eh?

I took this on to improve my programing, I don't see myself ever using VRML again, c++/c on the other hand might come up once or twice . Many/Any in IT do a openGL game? Only one other in ECE did and believe it or not i had to fix it for him :eek:

fergalr wrote: »

Just the first thing I noticed - you might want to remove your name and student number from the source?
Apologies if you know this and don't care - just thought I'd point it out in case you missed it.

Your right i did miss it :pac: Ah sure what harm, im sure no one can steal my identity with that info

toiletduck

witnessmenow wrote: »

I took this on to improve my programing, I don't see myself ever using VRML again, c++/c on the other hand might come up once or twice . Many/Any in IT do a openGL game? Only one other in ECE did and believe it or not i had to fix it for him :eek:

Ha! Well I did the course last year, I don't recall anyone going with the OpenGL.