how horrible is my programming style!

I have been dabbling with c++ for a while as a hobby as i would like to eventually program simple 2d games. Unfortunately being self taught combined with the fact that my maths is crap means that i basicly suck at this stuff.

Anyway i know that this program is retarded and that the style and structure is probably abismal . Never the less i am over the moon with it.

All it does is generate 2 random numbers and then stage by stage go through the process of working out the answer, using a carry if needed.

As other beginners to c++ will probably agree at the moment for me its not about solving the problem in style but just solving no matter how ugly it looks.

Code:

//// Demonstrating subtraction with workings////

#include<iostream>
#include<iomanip>
#include<time.h>
#include<stdlib.h>
using namespace std;

void Subtraction(int num1, int num2);
int numLength(int num);
void NumInArray(int nums[], int num, int numLen);
void ShowWorking(int nums1[], int num1Len, int nums2[], int num2Len);
void OutputArray(int nums[], int arrayLength);
void Pause(void);


int main(void)
{
	
	srand((unsigned int)time(NULL));

	for(;;)
	{
		cout << "NEW SUM!!!!!\n";
		int num1 = rand() % 1000 + 1;
		int num2 = rand() % 1000 + 1;

		if(num1 < num2)
		{
			int temp = num1;
			num1 = num2;
			num2 = temp;
		}

		Subtraction(num1, num2);
	}

	return(0);
}

void Subtraction(int num1, int num2)
{
	int num1Len = numLength(num1);
	int num2Len = numLength(num2);

	int nums1[16] = {0};
	int nums2[16] = {0};

	NumInArray(nums1, num1, num1Len);
	NumInArray(nums2, num2, num2Len);
	
	ShowWorking(nums1, num1Len, nums2, num2Len);
}
	

void NumInArray(int nums[], int num, int numLen)
{
	int mult = 10;

	for(int i = 0; i < numLen; i++)
	{
		nums[i] = (num % mult) / (mult / 10);
		mult *= 10;
	}
}


int numLength(int num)
{
	int numLen = 0;
	int mult = 1;

	do
	{
		mult *= 10;
		numLen++;
	}while(num / mult != 0);

	return(numLen);
}

void ShowWorking(int nums1[], int num1Len, int nums2[], int num2Len)
{
	int answer[16] = {0};

	OutputArray(nums1, num1Len);
	OutputArray(nums2, num1Len);
	cout << "-----\n";
	OutputArray(answer, num1Len);
	cout << '\n';

	for(int i = 0; i < num1Len; i++)
	{
		int count = 1;

		if(nums1[i] >= nums2[i])
		{
			answer[i] = nums1[i] - nums2[i];
		}
		else	// Nick one from next door
		{
			if(nums1[i+1] > 0)
			{
				nums1[i+1] -= 1;
				nums1[i] += 10;
			}
			else		// If there isnt one next door go further a field
			{
				do
				{
					count++;
				}while(nums1[i + count] == 0);
				
				cout << "count " << count << '\n';
				nums1[i] += 10;
				nums1[i+count] -= 1;

				for(int j = 1; j < count ; j++)
				{
					nums1[i+j] += 9;
				}
			}
			answer[i] = nums1[i] - nums2[i];
		}
		
		Pause();

		OutputArray(nums1, num1Len);
		OutputArray(nums2, num1Len);
		cout << "-----\n";
		OutputArray(answer, num1Len);
		cout << '\n';
	}

}

void OutputArray(int nums[], int arrayLength)
{
	for(int i = arrayLength - 1; i >= 0; i--)
	{
		
		cout << nums[i];
	}
	cout << "\n";
}

void Pause(void)
{
	for(int i = 0; i < 50000; i++)
	{
		for(int j = 0; j < 30000; j++)
		{
		}
	}
}

if anyone can point out any subtle changes i would be interested.

Thanks
Mark S.

What's the purpose of Pause();? If it's solely to pause the program, I would recommend a Sleep(); function, in <windows.h> enter the number of miliseconds to pause the program for.

First off, it's not really that bad for the size of the program and your skill level.
One thing I would suggest would be commenting when you have closing braces after more than 4 or so lines of code.

Code:

for(int i = 0; i < num1Len; i++)
	{
		int count = 1;

		if(nums1[i] >= nums2[i])
		{
			answer[i] = nums1[i] - nums2[i];
		}
		else	// Nick one from next door
		{
			if(nums1[i+1] > 0)
			{
				nums1[i+1] -= 1;
				nums1[i] += 10;
			}
			else		// If there isnt one next door go further a field
			{
				do
				{
					count++;
				}while(nums1[i + count] == 0);
				
				cout << "count " << count << '\n';
				nums1[i] += 10;
				nums1[i+count] -= 1;

				for(int j = 1; j < count ; j++)
				{
					nums1[i+j] += 9;
				}
			}
			answer[i] = nums1[i] - nums2[i];
		}
		
		Pause();

		OutputArray(nums1, num1Len);
		OutputArray(nums2, num1Len);
		cout << "-----\n";
		OutputArray(answer, num1Len);
		cout << '\n';
	}

where does the for loop end here?

This is a good example of what I'm talking about. When you're skimming through the program, and you have lots of nested loops, it can get confusing.

Next, when you only have one line of code after your condition, you don't need braces.

Code:

void OutputArray(int nums[], int arrayLength)
{
	for(int i = arrayLength - 1; i >= 0; i--)
	{
		
		cout << nums[i];
	}
	cout << "\n";
}

would look better like this

Code:

void OutputArray(int nums[], int arrayLength)
{
	for(int i = arrayLength - 1; i >= 0; i--)		
		cout << nums[i];

	cout << "\n";
}

Hope that helped a bit.

Originally Posted by System_159

Next, when you only have one line of code after your condition, you don't need braces.

Code:

void OutputArray(int nums[], int arrayLength)
{
	for(int i = arrayLength - 1; i >= 0; i--)
	{
		
		cout << nums[i];
	}
	cout << "\n";
}

would look better like this

Code:

void OutputArray(int nums[], int arrayLength)
{
	for(int i = arrayLength - 1; i >= 0; i--)		
		cout << nums[i];

	cout << "\n";
}

Hmmm, I don't know if I'd agree with you on that one! You don't need them, as you say, but I generally prefer to leave them in, basically as a visual reminder of the scope of the loop. Alternatively, if you really wanted, you could do this::

Code:

for ( int i=0; i<x; i++ ) cout<< "\n"; // Just as a one liner

of

Code:

for ( int i=0; i<x; cout<< "\n", i++ );

I can never remember whether the latter works. I'm quite sure it does though.

However, System, I do agree with your first point! if I had this (intentionally badly formatted!)::

Code:

int main()
{
for ( int i=0; i<24562; i++ )
{
if ( i%34 == 0 )
{
while ( i%34 != 0 )
{
cout<< "sdajkl";
}
}
else if
{
whatever
}
}
return 0;
}

nobody could deciher it!

Code:

int main()
{
for ( int i=0; i<24562; i++ )
{
if ( i%34 == 0 )
{
while ( i%34 != 0 )
{
cout<< "sdajkl";
}
} // end of if
else if
{
whatever
} // end of else
} // end of for loop
return 0;
} // End of Main

That's not to say I do it

>how horrible is my programming style!
Your style is fine. Stop worrying about it. Consistency is key, and you're consistent.

>if anyone can point out any subtle changes i would be interested.
The only serious change I would suggest is to get rid of the antisocial pause. A busy wait loop is a poor way to pause the execution, and I honestly don't see the need for it in this program.

>I can never remember whether the latter works. I'm quite sure it does though.

It does, but it sure doesn't beat the readability of the previous one.

Originally Posted by Mario F.

It does, but it sure doesn't beat the readability of the previous one.

Agreed. But what are aesthetics where exe's are concerned?

> Agreed. But what are aesthetics where exe's are concerned?

Let me put it this way... showing Armani on the outside will still not score you any brownie points if your underwear is dirty.

The code doesn't look too bad at all. And it compiles OK.

The pause loop is not probably the best way to do it. I have a quite slow computer, and I'm not sure it is meant to wait so long between doing something.

However, I don't understand the output. What I see is something like that:

NEW SUM!!!!!
999
175
---------
004 //<- some random value, mostly zeros.

What you seem to be doing here might be called reinventing the wheel. While there is a good reason for a learner to reinvent the wheel (to understand how things work), if you want to use this for a more complex program, it would be a good idea to use whatever C++ has to offer to make the task easier.

In this case I think you might take a look at stringstreams that make converting strings into other types and the other way round very easy. (You might even add that reference page to your favourites as it's full of useful stuff.)

Code:

for(;;)
	{
		cout << "NEW SUM!!!!!\n";
		int num1 = rand() % 1000 + 1;
		int num2 = rand() % 1000 + 1;

		if(num1 < num2)
		{
			int temp = num1;
			num1 = num2;
			num2 = temp;
		}

		Subtraction(num1, num2);
	}

This loop doesn't look good. If you don't have any statements to put in the for loop then a while loop might look better. More importantly, this loop has no exit condition. It means, the program will never end properly (with return 0;).