Help

[Newie0001]

How would I go about cleaning up this program and making it easier to understand? Any advice?

 #include <iostream>
#include <cstdlib>
#include <ctime>
#include <cmath>
using namespace std;

int rand_0toN1(int n);
void draw_a_card();
int select_next_available(int n);
bool card_drawn[52];
int cards_remaining = 52;

char *suits[4] =
		{"hearts", "diamonds", "spades", "clubs"};

char *ranks[13] =
		{"ace", "two", "three", "four", "five",
			"six", "seven", "eight", "nine",
				"ten", "jack", "queen", "king" };
int main() 
{
int n, i;
srand(time(NULL));  
while (1) 
{
	cout << "Enter no. of cards to draw ";
	cout << "(0 to exit): ";
	cin >> n;
	if (n == 0)
		break;
	for (i = 1; i <= n; i++)
		draw_a_card();
}
return 0; 
}

void draw_a_card() 
{
int r, s;
int n, card;
n = rand_0toN1(cards_remaining--);
card = select_next_available(n);
r = card % 13;          
s = card / 13;     
cout << ranks[r] << " of " << suits[s] << endl;
}

int select_next_available(int n) 
{
int i = 0;
while (card_drawn[i])
i++;
while (n-- > 0)
{     			
i++;         
while (card_drawn[i])
i++;
}
card_drawn[i] = true;
return i; }

int rand_0toN1(int n) {
	return rand() % n; }

[AbstractionAnon]

Lines 13, 16: These should be const char *

Assuming you're going to expand this into some kind of game, draw_card() should return the card drawn.

select_next_available() is hard to follow. You seem to start at 0 testing if a card has been drawn. I'm not sure what the nested while loop at line 53 is doing. I would think it would be easier to simply generate random numbers until you get one where card_drawn[] is not set.

You're relying on globals which is a poor practice. You're also relying on the fact that globals are initialized to 0. If your were to add a loop in main to play more than once, you would have to reinitialize cards_drawn[] and cards_remaining.

I'd suggest making a Deck class with a constructor to do proper initialization and a Draw() method to remove a card and track that it's been used.



Your indentation leaves a lot to be desired.

[Renthalkx97]

Here you go. I cleaned it up a LITTLE and I left comments explaining what I changed.

#include <iostream>
#include <sstream>
#include <string>
#include <cstdlib>
#include <ctime>
#include <cmath>

//got rid of this and instead just prefix all the std members with "std::" minus the quotes

//organized your function prototypes based on return value
void draw_a_card();
int rand_0toN1(int n);
int select_next_available(int n);
int cards_remaining = 52;
bool card_drawn[52];

//organized your arrays and removed the unecessary size declaration: it automatically determines the size when there's elements in it.
char *suits[] =
{ 
	"hearts", 
	"diamonds", 
	"spades", 
	"clubs" 
};

char *ranks[] =
{ 
	"ace", "two",	"three", "four", "five",
	"six", "seven", "eight", "nine",
	"ten", "jack",	"queen", "king" 
};

//added spacing between lines and refactored some things
int main()
{
	srand(time(NULL));

	int n;
	std::string input;

	while(1)  //this is all typesafe now and it won't go into an infinite loop if you enter in invalid input
	{
		std::cout << "Enter no. of cards to draw (0 to exit): ";

		std::getline(std::cin, input);

		std::stringstream ss(input);

		ss >> n;

		if(n == 0)
			break;

		for(int i = 1; i <= n; i++)
		{
			draw_a_card();
		}

		std::cin.clear();
	}

	return 0;
}

void draw_a_card()
{
	int r, s;
	int n, card;

	n = rand_0toN1(cards_remaining--);

	card = select_next_available(n);

	r = card % 13;
	s = card / 13;

	std::cout << ranks[r] << " of " << suits[s] << std::endl; //i prefer '\n' but it's up to you. 'endl' also flushes the buffer.
}

int select_next_available(int n)
{
	int i = 0;

	while(card_drawn[i])
	{
		i++;
	}

	while(n-- > 0)
	{
		i++;

		while(card_drawn[i])
		{
			i++;
		}
	}

	card_drawn[i] = true;

	return i;
}

int rand_0toN1(int n)
{
	return rand() % n;
}

[deathslice]

the cards_remaining variable should be a constant since it's always going to be 52 cards and the two char* should constant chars* since they aren't going to be modified.

[Newie0001]

Thank you for your advice guys! and @Renthalk thanks for taking the time.

[Renthalkx97]

cards_remaining does in fact get modified though.

[Newie0001]

Quick question. What is it about the code you cleaned up that makes it easier to maintain as opposed to what I used?

[deathslice]

Yes you're right I forgot that it decrements. Silly me.
regardless, you shouldn't declare as a global variable.

[Newie0001]

This is not my code, I'm just trying to understand it. Reason why I posted this here was because I wanted to learn about making code easier to understand (clean). How it could be made safer and easier to maintain etc...

[deathslice]

It's ok. it's always good to look other peoples' code and try to understand the way that they structured their program. It's even better when you take their program and modify into some completely different.

[Newie0001]

I'm still new to C++ programming and programming in general so I still have a lot to learn hahah

[deathslice]

No worries. Welcome aboard.

[Newie0001]

Thanks!