Sign in to follow this  

Coding Style: Please Help Improve

This topic is 4860 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
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
Not commenting on actual code:

It's certainly much prettier then mine...

The only thing I can suggest is to tab out prototypes; at least the comments.

For me at least, I find prototypes much easier to read and just cleaner when they're in columns:



void proto1 (int,int,float); // some comments
int proto2bigname (); // more comments
struct foo proto3 (void *); // blah blah



or



void proto1(int,int,float); // some comments
int proto2bigname(); // more comments
struct foo proto3(void *); // blah blah

Share this post


Link to post
Share on other sites
Quote:
Original post by Skizz
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.
7) Put breaks in multiple word variable names to make it easier
I totally agree apart from 5 and 7.
Leaving out {} after all 'if's etc has NEVER caused me any headaches.
And it's easy to accidentally type a space instead of an underscore and not notice it.

benryves: That coding style is fairly common. I for one use it all the time. Don't switch back, I sure aren't going to.
Otherwise by the time you put comments in, your functions still take more than a screenful - aaargh.

Terlenth: Doing variable initialisations like
 	int Startposition = 0;
is much nicer than doing the same in two lines.
			totallength = totallength - Length;
can also be simplified to:
			totallength -= Length;

And
			Staircounter = Staircounter - 1;
becomes
			Staircounter--;

Share this post


Link to post
Share on other sites
The only thing I would add is to use constants for the different drawtypes instead of hard-coding the values 1, 2, and 3 everywhere. You could use enum, or define constants, or use #define (your preference) - but the main thing is to give them meaningful, self-documenting names.

Share this post


Link to post
Share on other sites
I just thought I would mention this. I like prototypes. In fact I usually format them according to who calls who, and even have comments in place of functions who prototypes have already been defined. For example:


void FunctionA(void)
void FunctionB(void)

void FunctionC(void)
// FunctionB



This allows for a nice index of all functions in the file at the top of your source file. I even go as far as to order the definitions of the functions in the leveled order that is:


void FunctionA(void)
{
}


void FunctionC(void)
{
}

void FunctionB(void)
{
}



This sort of thing often allows me to access my functions quickly without the aid of a fancy editor, and in my oppinion just looks nicer.

Then again, as I have said, this is what I do, and I have not seen anyone else actually do this. :D

Share this post


Link to post
Share on other sites
*bump*

I'm just going to bump this once just in case someone who missed it after the update has something they may be able to add. If not then I'll just leave it at that.

Thanks again

Share this post


Link to post
Share on other sites
I have nothing to add to what's already been said. I just want to tell Terlenth that I respect that you took active initiative to improve your coding style. That initiative will land you a wonderful job one day. Good luck.

Share this post


Link to post
Share on other sites
I'm hoping so =p

I hope that what I'm doing here will help me with my school and help me develop my coding skill so that I can hopefully get a good job.

Share this post


Link to post
Share on other sites
Either separate different words in function names (and variables) with underscores or use java style capitalization from second word on. It really doesn't matter what you do, but it would really help the readability of your code.

functions that have names like superlongfunctionname aren't as pleasent on the eyes as:
super_long_function_name
superLongFunctionName
SuperLongFunctionName
...

Share this post


Link to post
Share on other sites
Here's some recommendations for you:
Quote:

////////////////////////
// Greg Powell //
// 0221498 //
// Asignment #1 //
////////////////////////

This file header is way too complex. Having a border on the right is much more trouble than it's worth. Whenever you change a line to be longer you have to extend the entire border, making it a hassle.
Quote:

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);

You should probably pick a better naming convention for functions. Analllowercasenamingconvention takes more effort to decipher than something like StairsFromLeft or stairsFromLeft. Also, you should almost always use verbs for function names. Also, the draw function doesn't specify what it's drawing in the name. If it's meant to draw the scene you should probably rename it to 'drawScene' or 'drawWorld.'
Quote:

int Length; //Length of stairs
int Height; //Height of stairs

You need to rethink these variable names. Renaming them so that they have 'Stair' in their names is much more readable than commenting it. Also, you should comment the type of units you are using.
Quote:

int Numberstairs1; //Number of stairs from the left
int Numberstairs2; //Number of stairs from the right

You should never put numbers in variable names. If they truly should be numbered, then you should put them in an array. You should probably reword those comments, they're fairly hard to understand. Also, you should think about using another naming convention for variable names.
Quote:

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

I see a lot of magic numbers here. Consider replacing them with named constants.
Quote:

for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
{
draw(Length, Startposition, DRAWTYPE1 , NO_LENGTH_REQUIRED, VERTICAL);
printf("\n");
}

I see duplicate code. You should put this into a seperate function.
Quote:

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");
}
}

This function should be broken up into other functions. Generally speaking, if you can't fit the whole function on the screen, it needs to be broken up. Also, >3 levels of nesting decreases the readability of a function. Whenever I program I try and keep the nesting level less than 3.
Quote:
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;
}

This function also needs to be broken up.

I also recommend reading Code Complete; it has a truckload of information regarding readability and style. It does center around object oriented languages, but most of the stuff on procedures still apply.

Share this post


Link to post
Share on other sites
Just have some other questions dealing with what you had mentioned.

Quote:

Quote:

int Length; //Length of stairs
int Height; //Height of stairs

You need to rethink these variable names. Renaming them so that they have 'Stair' in their names is much more readable than commenting it. Also, you should comment the type of units you are using.

That makes sense, I can change that. As for units it was I believe just spaces in command promt so I don't really know what I'd call those as units.
Quote:

Quote:

int Numberstairs1; //Number of stairs from the left
int Numberstairs2; //Number of stairs from the right

You should never put numbers in variable names. If they truly should be numbered, then you should put them in an array. You should probably reword those comments, they're fairly hard to understand. Also, you should think about using another naming convention for variable names.

The variables probably should have read NumberOfStaits1, etc. because these variable are supposed to represent the number of stairs in each staircase instance. As for the comments being confusing I don't quite see it, could you please explain to me why they are confusing?

Quote:

Quote:

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

I see a lot of magic numbers here. Consider replacing them with named constants.

I don't really understand why you are saying these are magic numbers, or how I'd make those into named constants. They are just supposed to represent the math required to get it... so I'm a bit confused as how to change that part, maybe you could again let me know how you would do it?

Quote:

Quote:

for (Heightcounter = 0; Heightcounter < Height; Heightcounter++)
{
draw(Length, Startposition, DRAWTYPE1 , NO_LENGTH_REQUIRED, VERTICAL);
printf("\n");
}

I see duplicate code. You should put this into a seperate function.

I guess that would work. The only way I can see that working is if I create about 3 or so functions to replace those loops...

Quote:

This function should be broken up into other functions. Generally speaking, if you can't fit the whole function on the screen, it needs to be broken up. Also, >3 levels of nesting decreases the readability of a function. Whenever I program I try and keep the nesting level less than 3.

Could you give me an example maybe? Becuase, I don't quite see again how I would be able to do so cleanly, or without causing problems.

Quote:

This function also needs to be broken up.

Again I don't quite see how I could do so. I see some ways that might allow me to break it up (like the way I did it in the beginning where I had two draw functions). But I don't quite see how I would do so otherwise.

Share this post


Link to post
Share on other sites
All thou consistent & clear coding conventions is important, i would say learning how to design & structure programs through abstraction by modulzing your code even more important.

For example the main concept in your problem domain is steps & stairs wouldn't it be nice & more natural to actually work with steps & stairs instead of integers & floats?

You seem to be working in C so i would advise you look up on structures, these let you add new user-defined types to the pre-existing built-in ones such as integer.

Basically what you do is think about what concepts are involved with the problem, the nouns in your system these will become C structures & what attributes does this type have, then you think about what operations you need to perform on these types, the verbs in your system.

So a very simplified example is you have stairs in your system the useful attributes in the context of this system are the width & height per step plus the total number of steps, and you wont to "draw the stairs" to the shell/console prompt but not modify the state of stairs its only a reading operation.

Something like this is in C code:



typedef struct _stairs {

int step_width;
int step_height;
int num_of_steps;
} stairs;


void draw_stairs(const stairs* const my_stairs);



get what i'm saying?

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
Yeah, I've used structures before. I've always had problems with them mind you. But that's probably just me not implimenting certain aspects of code. I'll try to see how I can impliment that here.

Thanks

Share this post


Link to post
Share on other sites

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;
}



There is a lot of duplication here. That's bad.
My first comment is that I wouldn't bother with the "Space" etc. constants, and just use plain printfs. You aren't really gaining extra readability this way, and while you have the advantage here that you only need to change the drawing symbols in one place if you want to change them - hopefully after some adjustments there will only be one place anyway ;)

First, consider the drawType == HORIZONTAL case. Notice you have a whole bunch of loops that draw some specific character sequence. The first thing I'm going to do is change it to just calculate how many of each to draw, and then have a single set of loops for the drawing:


int firstSpaces = 0;
int firstDashes = 0;
int secondSpaces = 0;
int secondDashes = 0;
switch (Drawtype)
{
case DRAWTYPE1:
firstSpaces = Startposition;
firstDashes = Length;
returnValue = Length;
// As another comment, I find it's generally a bad idea to use
// the input parameters as variables, you'll get confused.
// Also, notice I took an increment out of the loop;
// as it stood, Startposition always matched Positioncounter
// within the loop, so I know what it will be at the end. :)
// Well. I'm assuming you don't have negative lengths and weird
// things like that. :/
break;
case DRAWTYPE2:
firstSpaces = Startposition - Length;
firstDashes = Length;
Returnvalue = Startposition - Length;
break;
case DRAWTYPE3:
firstSpaces = Startposition;
firstDashes = Length;
Startposition += Length;
// Ok, now I see what you're doing with the Startposition. :/
secondSpaces = Totallength - Startposition;
secondDashes = Length;
returnValue = Startposition;
}
// OK, now draw.
for (Positioncounter = 0; PositionCounter < firstSpaces; PositionCounter++) {
printf(" ");
}
for (Positioncounter = 0; PositionCounter < firstDashes; PositionCounter++) {
printf("- ");
for (Positioncounter = 0; PositionCounter < secondSpaces; PositionCounter++) {
printf(" ");
}
for (Positioncounter = 0; PositionCounter < secondDashes; PositionCounter++) {
printf("- ");
}



You'll probably need to debug that, especially because you probably *do* have to trim negative Lengths and other things along those lines :/ and because I probably flipped some signs somewhere. Anyway.

Ok, that's all well and good, but it still looks pretty long. We cut the eight for loops down to four, but now the duplication is even more obvious. And we also don't use two of the loops most of the time (they execute zero times).

But we can see the pattern more clearly now; we're drawing pairs of (some number of " ") followed by (some number of "- "). Let's extract that into a new function. That will also help with the criticism of this method being too long, and will let us avoid a bunch of these temporary variables.


switch (Drawtype)
{
case DRAWTYPE1:
drawSpacesAndDashes(Startposition, Length);
Returnvalue = Length;
break;
case DRAWTYPE2:
drawSpacesAndDashes(Startposition - Length, Length);
Returnvalue = Startposition - Length;
break;
case DRAWTYPE3:
drawSpacesAndDashes(Startposition, Length);
// Now I'm going to do a couple of transformations so that we
// don't actually modify Startposition. First, substitute
// (Startposition + Length) for Startposition in the original
// lines, and do algebraic simplification:
// secondSpaces = Totallength - Startposition - Length;
// secondDashes = Length;
// Returnvalue = Startposition + Length;
// And now write the actual lines:
drawSpacesAndDashes(Totallength - Startposition - Length, Length);
Returnvalue = Startposition + Length;
}

// And now we have to write that function, at the top level:
void drawSpacesAndDashes(int spaces, int dashes) {
int i;
for (i = 0; i < spaces; i++) {
printf(" ");
}
for (i = 0; i < dashes; i++) {
printf("- ");
}
}



Similarly you can make transformations to the VERTICAL case. What would you like to change about drawSpacesAndDashes to make things easier now? :)

The next beast to tame is convergingstairs(). The first thing I'd suggest is to kill 'Largernumberofstairs', which is only used once, in a situation where it's equal to Numberstairs1 (and also Numberstairs2 ;/ ). Anyway, it doesn't really tell us anything that we don't already know from the equality tests, so get rid of it. We can also get rid of Drawtype, since it's always known what it will be when we use it. (If that makes you uneasy, ask yourself why you *didn't* make a variable "HorizontalOrVertical".)

Now I look through the function and notice that *every* call to draw() is followed by a printf("\n");. I use my text editor to do some searching, and confirm that this is the case through the whole program. Ok, so let's add the newline as part of the drawing call. This makes sense, since the function logically draws a line of stairs, and a line ends with \n.

Ok, now we're set to look for more duplication again. Do you see similarities between how the different cases (relative value of Numberstairs1 vs Numberstairs2) are handled in convergingstairs()? One thing I see is that you have the same call draw(Length, Startposition, DRAWTYPE1, NO_LENGTH_REQUIRED, HORIZONTAL); at the end of each case. So pull that out; just do it once at the end of the function.

Maybe it will help clarify things if you convert those while loops into the equivalent for loops. It will also help if you can copy out those blocks of source code and look at them side-by-side (the best way to do that depends on your editor).

Once that's cleaned up a bit, do you see similarities between how stairsfromleft() and stairsfromright() work? How can you combine those?

Keep making little changes, following the techniques I used in the example at the beginning - and be sure to test to make sure you didn't mess anything up. And each time you improve one little thing, you'll start to see other things you can fix - it's amazing how it works out, really. All you really need to know are two basic rules:

1) Duplication smells bad; look for similar-looking things and try to find the way to generalize what it is you're doing. It's better to set up parameters for a loop in several ways, and call the loop only in one spot, than to do a new loop every time. Better yet to extract new functions, if you can get good use out of them. On the flip side, if a function only turns out to be needed in one spot, consider inlining it.
2) "if" and "switch" smell bad; it's better if you can use some mathematical formula to directly transform the value you're comparing into the one you want. They also tend to hide duplication; see rule (1).

All the rest is problem-solving, basically. Just remember, short is clear (unless we're talking about the names you give to things :) ) and Control-V is not your friend.

Refactoring. It does a codebase good.

Share this post


Link to post
Share on other sites

This topic is 4860 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.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this