Jump to content
  • Advertisement
Sign in to follow this  
Terlenth

Coding Style: Please Help Improve

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

Hello all, I realize that this is sort of a continuation from the Beginners thread that was started a couple of days ago but the code I would like some help on is not games code so I thought it would be out of place in that forum. I was hoping some of the more experienced coders would be able to take a look at my code and help me improve my style. This is one of my assignments from my C course.
[source lang = "c"]
////////////////////////
//     Greg Powell    //
//       0221498      //
//     Asignment #1   //
////////////////////////
 
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define LEFT_START_POSITION     0
#define RIGHT_START_POSITION    79
#define DRAWTYPE1               1
#define DRAWTYPE2               2
#define DRAWTYPE3               3
#define HORIZONTAL              4
#define VERTICAL                5
#define NO_LENGTH_REQUIRED      0

//Function Declaration

void stairsfromleft (int Length, int Height, int Numberstairs); 			//First type of staircase starting from the left
void stairsfromright (int Length, int Height, int Numberstairs); 			//Second type of staircase starting from the right
void convergingstairs (int Length, int Height, int Numberstairs1, int Numberstairs2); 	//Third type of staircase converging in the middle
int draw (int Length, int Startposition, int Drawtype, int Totallength, int Direction);	//Draw the lines of the stairs

int main (int argc, char * argv[])
{
	int Length;		//Length of stairs
	int Height;		//Height of stairs
	int Numberstairs1;	//Number of stairs from the left
	int Numberstairs2;	//Number of stairs from the right
	
	srand(time(0));		//Seed random numbers

	Length = (rand() % 2) + 3;
	Height = Length / 2;
	Numberstairs1 = (rand() % 5) + 2;
	Numberstairs2 = (rand() % 5) + 2;

	if (argc > 0)
	{
		if (strcmp(argv [1], "-1") == 0)
			stairsfromleft (Length, Height, Numberstairs1);
		if (strcmp(argv [1], "-2") == 0)
			stairsfromright (Length, Height, Numberstairs2);
		if (strcmp(argv [1], "-3") == 0)
			convergingstairs (Length, Height, Numberstairs1, Numberstairs2);
	}
	else
	{
		printf("Please re-run program with one of the following parameters: \n");
		printf("\'-1\' for a staircase from the left \n");
		printf("\'-2\' for a staircase from the right \n");
		printf("\'-3\' for a converging staircase");
	}
}

void stairsfromleft(int Length, int Height, int Numberstairs)
{
	int Staircounter;
	int Heightcounter;
	int Startposition;
	Startposition = LEFT_START_POSITION;
	for (Staircounter = 0; Staircounter < Numberstairs; Staircounter++)
	{
		Startposition = draw(Length, Startposition, DRAWTYPE1, NO_LENGTH_REQUIRED, HORIZONTAL);
		printf("\n");
		for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
		{
			draw(Length, Startposition, DRAWTYPE1 , NO_LENGTH_REQUIRED, VERTICAL);
			printf("\n");
		}
	}
}

void stairsfromright(int Length, int Height, int Numberstairs)
{
	int Staircounter;
	int Heightcounter;
	int Startposition;
	Startposition = RIGHT_START_POSITION;
	for (Staircounter = 0; Staircounter < Numberstairs; Staircounter++)
	{
		Startposition = draw(Length, Startposition, DRAWTYPE2, NO_LENGTH_REQUIRED, HORIZONTAL);
		printf("\n");
		for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
		{
			draw(Length, Startposition, DRAWTYPE1, NO_LENGTH_REQUIRED, VERTICAL);
			printf("\n");
		}
	}
}

void convergingstairs(int Length, int Height, int Numberstairs1, int Numberstairs2)
{
	int Staircounter;
	int Heightcounter;
	int Totallength;
	int Startposition;
	int Drawtype;
	int Largernumberofstairs;
	int Totalnumstairs;
	
 	Totalnumstairs = Numberstairs1 + Numberstairs2;
	Totallength = (Length) * (Totalnumstairs);
	
	if (Numberstairs1 == Numberstairs2)
	{
		Startposition = LEFT_START_POSITION;
		Drawtype = DRAWTYPE3;
		Largernumberofstairs = Numberstairs1;
		for (Staircounter = 0; Staircounter < Largernumberofstairs; Staircounter++)
		{
			Startposition = draw(Length, Startposition, Drawtype, Totallength, HORIZONTAL);
			printf("\n");
			for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
			{
				draw(Length, Startposition, Drawtype, Totallength, VERTICAL);
				printf("\n");
			}
			Totallength -= Length;
		}
		draw(Length, Startposition, DRAWTYPE1, NO_LENGTH_REQUIRED, HORIZONTAL);
		printf("\n");
	}
	if (Numberstairs1 > Numberstairs2)
	{
		Startposition = LEFT_START_POSITION;
		Drawtype = DRAWTYPE1;
		Largernumberofstairs = Numberstairs1;
		Staircounter = Numberstairs1;
		while (Staircounter != Numberstairs2)
		{	
			Startposition = draw(Length, Startposition, Drawtype, Totallength, HORIZONTAL);
			printf ("\n");
			for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
			{
				draw(Length, Startposition, Drawtype, Totallength, VERTICAL);
				printf("\n");
			}
			Totallength -= Length;
			Staircounter--;
		}
		Totallength += Length * (Numberstairs1 - Numberstairs2);
		for (Staircounter = 0; Staircounter < Numberstairs2; Staircounter++)
		{
			Drawtype = DRAWTYPE3;
			Startposition = draw(Length, Startposition, Drawtype, Totallength, HORIZONTAL);
			printf ("\n");
			for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
			{
				draw(Length, Startposition, Drawtype, Totallength, VERTICAL);
				printf("\n");
			}
			Totallength -= Length;
		}
		draw(Length, Startposition, DRAWTYPE1, NO_LENGTH_REQUIRED, HORIZONTAL);
		printf("\n");
	}	
	if (Numberstairs2 > Numberstairs1)
	{
		Startposition = Totallength + Length;
		Drawtype = DRAWTYPE2;
		Largernumberofstairs = Numberstairs2;
		Staircounter = Numberstairs2;
		while (Staircounter != Numberstairs1)
		{	
			Startposition = draw(Length, Startposition, Drawtype, Totallength, HORIZONTAL);
			printf ("\n");
			for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
			{
				draw(Length, Startposition, DRAWTYPE1, Totallength, VERTICAL);
				printf("\n");
			}
			Totallength -= Length;
			Staircounter--;
		}
		Startposition = LEFT_START_POSITION;
		for (Staircounter = 0; Staircounter < Numberstairs1; Staircounter++)
		{			
			Drawtype = DRAWTYPE3;
			Startposition = draw(Length, Startposition, Drawtype, Totallength, HORIZONTAL);
			printf ("\n");
			for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
			{
				draw(Length, Startposition, Drawtype, Totallength, VERTICAL);
				printf("\n");
			}
			Totallength -= Length;
		}
		draw(Length, Startposition, DRAWTYPE1, NO_LENGTH_REQUIRED, HORIZONTAL);
		printf("\n");
	}	
}

int draw (int Length, int Startposition, int Drawtype, int Totallength, int Direction)
{
	int Returnvalue;
	int Positioncounter;	
	char Space = ' ';
	char Horizontaldash  = '_';
	char Verticaldash  = '|';

	if (Direction == HORIZONTAL)
	{
		switch (Drawtype)
		{
			case DRAWTYPE1:
				for (Positioncounter = 0; Positioncounter < Startposition; Positioncounter++)
					printf ("%c ", Space);
				for (Positioncounter = 0; Positioncounter < Length; Positioncounter++)
				{	
					printf ("%c ", Horizontaldash);
					Startposition++;
				}
				break;
			case DRAWTYPE2:
				for (Positioncounter=Startposition; Positioncounter > Length; Positioncounter--)
					printf("%c ", Space);
				for (Positioncounter = 0; Positioncounter < Length; Positioncounter++)
				{	
					printf ("%c ", Horizontaldash);
					Startposition--;
				}
				break;
			case DRAWTYPE3:
				for (Positioncounter = 0; Positioncounter < Startposition; Positioncounter++)
					printf ("%c ", Space);
				for (Positioncounter = 0; Positioncounter < Length; Positioncounter++)
				{	
					printf ("%c ", Horizontaldash);
					Startposition++;
				}
				for (Positioncounter = Totallength; Positioncounter > Startposition; Positioncounter--)
					printf ("%c ", Space);
				for (Positioncounter = 0; Positioncounter < Length; Positioncounter++)
					printf ("%c ", Horizontaldash);
				break;
		}
		Returnvalue = Startposition;
	}
	if (Direction == VERTICAL)
	{
		switch (Drawtype)		
		{
			case DRAWTYPE1:
				for (Positioncounter = 0; Positioncounter < Startposition; Positioncounter++)
					printf ("%c ", Space);
				printf ("%c ", Verticaldash);
				break;
			case DRAWTYPE3:
				for (Positioncounter = 0; Positioncounter < Startposition; Positioncounter++)
					printf ("%c ", Space);
				printf ("%c ", Verticaldash);
				for (Positioncounter = Totallength; Positioncounter > Startposition + 1; Positioncounter--)
					printf ("%c ", Space);
				printf ("%c ", Verticaldash);
				break;
		}
		Returnvalue = 0;
	}
	return Returnvalue;
}





Thanks you in advance for your time and any help you can provide [Edited by - Terlenth on August 23, 2004 3:33:55 PM]

Share this post


Link to post
Share on other sites
Advertisement
Here's a few comments for you:
1) Use definitions as declarations. By this, I mean avoid prototypes unless you really have to. It makes maintenance a bit easier.
2) 'return' is not a function. It doesn't need '()'s. Also, if all the function returns is '0' then make it a void function.
3) Check the number of arguments passed to main (argc) before accesing the 'argv []' index. The user may have neglected to enter any parameters.
4) Use the 'switch' statement. Replace:

if (drawtype == 1)
{
...
}
if (drawtype == 2)
{
...
}

can be replaced:

switch (drawtype)
{
case 1:
...
break;
case 2:
...
break;
}

5) Put '{}'s after all 'if's, 'for's and 'while's. It can save a lot of headaches caused by not paying attention to what your typing.
6) Meaningful names. 'type1' is not particularly informative.
7) Put breaks in multiple word variable names to make it easier to read: NumberStairs, numberStairs or number_stairs. Choose one and be consistant.
8) Generally, and this is pesonal preference, use uppercase in function names, 'DrawVertical', and lowercase in variables, 'start_position'.

Skizz

Share this post


Link to post
Share on other sites
My coding style runs like a bit like this:

if (isset($_REQUEST['action'])) {
$action = $_REQUEST['action'];
} else {
$action = 'inbox';
}

if ($action=='send') {
if (isset($_POST['sendMessage'])) {

require_once('htmlconv.php');

$subject = create_HTML($_POST['subjectField']);
if ($subject=='') {$subject = '(No Subject)'; }

$body = create_HTML($_POST['messageField']);
if ($body=='') {
echo 'You must enter a message. Please go back and try again!<br><a href="javascript: history.go(-1)">Return to message editor.</a>';
}

} else {
echo 'Sorry, please try again<br>';
echo $returnToInbox;
die();
}

send_Message($_POST['toField'],$_SESSION['user_id'],$subject,$body);

echo '<b>Message sent successfully</b><hr>';
$action = 'inbox';

}

if ($action=='delete') {
if (isset($_REQUEST['message'])) {
$message = $_REQUEST['message'];
if (verify_User($message)) {
mysql_query("DELETE FROM messages WHERE messageID=".$message);
echo '<b>Deleted message</b><hr>';
$action='inbox';
} else {
echo 'You cannot delete that message as it belongs to somebody else.<br>'.$returnToInbox;
}
} else {
echo 'Sorry, the requested action could not be executed.<br><a href="javascript: history.go(-1)">Click here to try again</a>';
die();
}
}




Is the fact that I use:

if (conditional) {
}

and not

if (conditional)
{
}

going to drive people crazy? I find my version more readable, but see more and more people using the second one. Should I switch?

Share this post


Link to post
Share on other sites
Err not sure if this is what you mean, but...

1/ More comments. Definition of the contract of the function and explanation of parameters unless both are *very* obvious (even to those whom have never seen the code before)

2/ Either use lowercase to start your variable names or UPPERCASE. And keep it consistant. Same with Classes and Functions. Constants are often ALL_UPPER_CASE with underscores seperating them.

And a bit of a tip... if you put the constant on the left hand side rather then the right hand side you can prevent things like this:

if (x=1)
when you actually meant
if (x==1)

If you put the constant on the left hand side

if (1=x)

It will give you a compiler error because you can't obviously change the value of 1!


Anyway your code is largely nice to read, because you put your braces on a new line and tab everything out nicely.

Share this post


Link to post
Share on other sites
When C was first conceived, programmers used teletype terminals - no graphics and 25 lines of text. So screen space was at a premium and the:

if (expr) {
}

style was used. These days, we've got bigger, graphical screens so the

if (expr)
{
}

style became popular. I prefer the latter, I like the braces to line up vertically. The important thing I guess is to be consistant.

Skizz

Share this post


Link to post
Share on other sites
Quote:
Original post by Skizz
When C was first conceived, programmers used teletype terminals - no graphics and 25 lines of text. So screen space was at a premium and the:

if (expr) {
}

style was used. These days, we've got bigger, graphical screens so the

if (expr)
{
}

style became popular. I prefer the latter, I like the braces to line up vertically. The important thing I guess is to be consistant.

Skizz


I guess that makes sense [grin]

Share this post


Link to post
Share on other sites
Quote:

1) Use definitions as declarations. By this, I mean avoid prototypes unless you really have to. It makes maintenance a bit easier.

Do you mean use the definitions of the functions as the declarations?

Quote:

2) 'return' is not a function. It doesn't need '()'s. Also, if all the function returns is '0' then make it a void function.

Yeah, I know 'return' isn't a function its just a personal preference when it comes to reading but I can change that pretty easily. As for the returning '0' its something my prof wanted to see, so I had to do it that way.

Quote:

3) Check the number of arguments passed to main (argc) before accesing the 'argv []' index. The user may have neglected to enter any parameters.

Very good point, probably should have done that. Thanks.

Quote:

4) Use the 'switch' statement.

Quite true, should have probably thought of that on my own. Thanks again.

Quote:

5) Put '{}'s after all 'if's, 'for's and 'while's. It can save a lot of headaches caused by not paying attention to what your typing.

Can do. Again its more of a personal preference when reading it that I didn't do that, but as stated before I can easily change that habit.

Quote:

6) Meaningful names. 'type1' is not particularly informative.

Again good point, I am normally pretty good at the meaningful names thing. Probably should take more time when coding and make sure the names are meaningful enough.

Quote:

7) Put breaks in multiple word variable names to make it easier to read: NumberStairs, numberStairs or number_stairs. Choose one and be consistant.

Normally do split multiple word variables up with underscores. And, I will definately try to be more consistant when it comes to the way they are written.

Quote:

8) Generally, and this is pesonal preference, use uppercase in function names, 'DrawVertical', and lowercase in variables, 'start_position'.

That's not a bad idea. Next time I'm coding I will try to do it that way.

Thanks

Edit: If anyone has anything they would like to add please do so. Thanks.

[Edited by - Terlenth on August 20, 2004 9:57:32 AM]

Share this post


Link to post
Share on other sites
The only thing I'd add would be that you should maybe work on refactoring some of your functions. Several of your switch statements and the two type functions do pretty much the same thing with one different parameter. It would be much cleaner and more managable to provide a single function which handles both types of stair and a different type of drawing. Instead of drawHorizontal/Vertical, it'd make more sense to have a single draw() which is passed a vert/horiz paramter to handle the two different cases.

Share this post


Link to post
Share on other sites
Thank you everyone, I'm going to impliment all of your suggestions (or at least most of them). If anyone is interested in the changes just let me know and I'll repost the work once its done (might do that anyway =P).

Thanks again.

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.

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!