C++

Forum

 
New to C++ Please Critique

heintzman (18)
Hello everyone, I just started looking into c++ a couple days ago and between Youtube and the tutorial pdf on here I think I'm doing well. I have created two classes a "Players" class and a "cards" class. I've just been playing around with vectors references pointers classes and more. Didn't really have a direction for this to go just wanted to keep manipulating the classes to become familiar with c++ before moving into it further.

So I am looking for people to critique my code tell me what is good what is bad, better ways of doing things, and if there is a specific way that is "more accepted" to doing something like comments what I could do there. Thanks in advance.


main.cpp
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
 
 #include <algorithm>    // std::random_shuffle
#include <ctime>        // std::time
#include <cstdlib>   
#include <stdlib.h>     /* srand, rand */


//My class files
#include "playerClass.h"

//standard namespace 
using namespace std;

//Funtion prototypes

void addPlayers(unsigned int, vector <Player>&);
//adds players to the vector
//@parm unsigned int     - the number of players to add.
//@parm vector <Player>& - they vector of all the players

void dealCards(unsigned int, vector<Player>&, vector <Card>&);
//deals x cards to specific player
//@parm unsigned int     - the number of cards to deal
//@parm vector <Player>& - they vector of the player
//@parm vector <Card>&   - our working deck

void displayDeck(const vector <Card>&);
//Displays the whole deck and card positions
//@parm const vector <Card>& - Cards in vector (deck)

void fillDeck(vector <Card>&);
//fills the deck with suits and values
//@parm vector <Card>& - the vector to be filled  

void showHands(vector <Player>&);
//shows the hands of all players
//@parm vector <Player>& - the vector of all the players


int main() {
	int hold;  //only used to keep the consol open
	int randomize; // for the shuffle
	srand(unsigned(std::time(0)));//without this random_shuffle will always give the same result
	
	//create my card and player vectors
	vector <Card> cards;
	vector <Player> players;
	

	//add playsers - fill deck - randomize "shuffle" deck - deals the cards - clear the screen
	//- shows the player hands - displays number of cards left in deck
	addPlayers(2, players);
	fillDeck(cards);
	randomize = rand() % 100 + 1;
	random_shuffle(cards.begin(), cards.end());
	dealCards(10, players, cards);
	system("CLS");
	showHands(players);

	cout << "Cards left in the deck: " << cards.size();

	cin >> hold;

	return 0;
}


void addPlayers(unsigned int amount, vector <Player>& players) {
	string name;
	int score (0);
	
	for (unsigned int i = 0; i < amount; i++) {
		cout << "Enter name of player " << i + 1 << ":";
		cin >> name;
		vector <Card> playerHand;
		Player newPlayer(name, score, playerHand);
		players.push_back(newPlayer);
	}
}

void dealCards(unsigned int amount, vector<Player>& player, vector <Card>& deck) {

	for (unsigned int i = 0; i < amount; i++) {
		for (unsigned int j = 0; j < player.size(); j++) {
			Card  newCard;
			newCard.setSuit(deck[0].getSuit());
			newCard.setValue(deck[0].getValue());
			player[j].addCard(newCard);
			deck.erase(deck.begin());
		}
	}

}

void displayDeck(const vector <Card>& cards) {

	unsigned int size = cards.size();

	for (unsigned int i = 0; i < size; i++) {
		cout << "Card " << i + 1 << " is: " << cards[i].getValue() << cards[i].getSuit() << endl;
	}

}

void fillDeck(vector <Card>& card) {
	//first loop to run every card second loop to place 13 cards per suit
	for (int i = 0; i <13; i++) {
		
		//suit value var - Always will be 1 value higher than i
		int value = i + 1;

		//select the suit and add the card to the vector
		for (int j = 1; j <= 4; j++) {

			if (j == 1) {
				Card newCard(value, 'H');
				card.push_back(newCard);
			}
			else if (j == 2) {
				Card newCard(value, 'D');
				card.push_back(newCard);
			}
			else if (j == 3) {
				Card newCard(value, 'S');
				card.push_back(newCard);
			}
			else {
				Card newCard(value, 'C');
				card.push_back(newCard);
			}
		}

	}

}

void showHands(vector <Player>& players) {

	for (unsigned int i = 0; i < players.size(); i++) {
		cout << players[i].getName() << "'s hand: \n";
		for (unsigned int j = 0; j < players[i].getHand().size();j++) {
			cout << "card " << j + 1 << ": " << players[i].getHand()[j].getValue() 
				<< players[i].getHand()[j].getSuit() << endl;
		}
		cout << "\n\n";
	}
}


PlayerClass.cpp

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
 
#include "playerClass.h"

Player::Player() {
	score;
	name;
	hand;
}

Player::Player(string newName, int newScore, vector <Card> newHand){
	score = newScore;
	name = newName;
	hand = newHand;
}

Player::~Player() {

}

int Player::getScore() const {
	return score;
}

string Player::getName() const {
	return name;
}

vector <Card> Player::getHand() const {
	return hand;
}

void Player::setScore(int newScore) {
	score = newScore;
}

void Player::setName(string newName) {
	name = newName;
}

void Player::setHand(vector <Card> newHand) {

	hand = newHand;

}

void Player::addCard(Card newCard){

	hand.push_back(newCard);
}


deckClass.cpp

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
 
//Function Definitions

#include "deckClass.h"

Card::Card() {
	suit;
	value;
}

Card::Card(int newvalue, char newsuit){
	value = newvalue;
	suit = newsuit;
}

Card::~Card() {

}

char Card::getSuit() const {
	return suit;
}

int Card::getValue() const {
	return value;
}

void Card::setSuit(char newsuit) {
	suit = newsuit;
}

void Card::setValue(int newvalue) {
	value = newvalue;
}


playerClass.h

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
 
#include <string>
#include "deckClass.h"

using namespace std;

#ifndef playerClass_H
#define playerClass_H

class Player
{
	public:
		//Default constructor
		Player();

		//overload constructor
		Player(string, int, vector<Card>);

		//Destructor
		~Player();

		//Accessor Functions

		int getScore() const;
		//returns the players score

		string getName() const;
		//returns players name

		vector <Card> getHand() const;
		//returns the vector of the players hand

		//Mutator Functions
		void setScore(int);
		//sets the players score
		//@parm int - players new score

		void setName(string);
		//sets players name
		//@parm string - Players name

		void setHand(vector <Card>);
		//sets the players hand
		//@parm vector <card> - a vector of a card hand

		void addCard(Card);
		//adds a card to the players hand
		//@parm vector <Card> - a vector of the deck
private:
	//Member variables
	int score;
	string name;
	vector<Card> hand;

};

#endif


deckClass.h

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
 
#include <iostream>
#include <vector>
using namespace std;

#ifndef deckclass_H
#define deckclass_H



class Card {

	public:
		//Default constructor
		Card();

		//overload constructor
		Card(int, char);

		//Destructor
		~Card();

		//Accessor Functions
		int getValue() const; 
			//returns the value of the cardre
		
		char getSuit() const;
			//returns suit of card

			//Mutator Functions
		void setValue(int);
			//sets card value
		   //@parm int - value of card (1-13)

		void setSuit(char);
			//sets card suit
			//@parm char - suit of card ('H' 'D' 'S' 'C')


	private:
		//Member variables
		int value;
		char suit;
};




#endif // !deckclass_H
booradley60 (896)
From just a cursory glance...

In deckClass.cpp
In the parameterless constructor, initialize the members to a sensible default value. Don't just say suit; Say suit = 'H'; or something. If it doesn't make sense to initialize a Card with default values, then don't provide a constructor with no parameters. Only create your parameterized constructor (line 10) and force anyone making objects of your class to provide sensible initial values for your class members.

Same goes for PlayerClass.cpp.
jlb (4973)
A couple of things I noticed after a very quick scan.

1. You have using statements inside your #include files, a very bad idea.
2. You have #included a C++ std header and the C std header that provides the same functionality, you should only include the C++ standard headers.
3. You should read up on and start using C++ constructor initialization lists.
4. You have some code duplication that should be eliminated. 
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
 
			if (j == 1) {
				Card newCard(value, 'H');
				card.push_back(newCard);
			}
			else if (j == 2) {
				Card newCard(value, 'D');
				card.push_back(newCard);
			}
			else if (j == 3) {
				Card newCard(value, 'S');
				card.push_back(newCard);
			}
			else {
				Card newCard(value, 'C');
				card.push_back(newCard);
			}

The above can be simplified:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
 
            char card_value;
			if (j == 1) {
                card_value = 'H';
			}
			else if (j == 2) {
				card_value =  'D';
			}
			else if (j == 3) {
				card_value = 'S';
			}
			else {
				card_value =  'C';
			}
			card.push_back({value, card_value});




heintzman (18)
Awesome feedback, thanks to both of you!
Topic archived. No new replies allowed.