Code Clean-up/Critique
Author
I've pretty much finished up the group of classes for a card game. Everything works fine, but now I'd like some help to clean it up stylistically and syntactically. There are a few things that I believe are bad practice (like my arrays) but I'm unsure of what to do with them.
If anyone could point out some places where I can rewrite code to make it more precise/concise, that would be wonderful. Any other tips would be great. For example, I'm sure I could use more references as parameters than I am already. I'm also pretty sure I've over coded some of the sections, so any help cutting down would be nice. My goal is to make something that is very reusable and clean.
Finally, any tips/functions that can be included to help debugging is appreciated. I'm unsure of how/where to include that in my source.
And just to note: I had a hard time with the suit and the rank classes so there is a better way of doing what I did. I guess I could of just created an instance of each card right from the start and ignored all of the static members, but tips are appreciated.
So to the code.
The general idea is that the card class creates card objects composed of a rank instance and a suit instance. From there, the stack class controls the instances of each card. The player is simply a stack with a name, so it inherits from that.
The source can be found here.
I know I am asking a lot but I can't even begin to mention how helpful this kind of information is for me. I've asked a few questions before and have gotten spot on answers that have really helped me out.
P.S. I also apologize for the lack of comments, but I believe most of it is self explanatory. Also, some of the tabbing came out a little off upon being uploaded, so ignore the minor inconsistencies.
You'll probably get better results if you posted some of the actual code you think has problems instead of expecting people to just browse your source at random.
int main () { //Implementation here.}Finish the implementation first, then we can talk about the finer points.
But as of right now, you don't have anything done yet. You might even find out that the class design is incorrect, or unsuitable.
Until you show *how* you intend to use these classes, they are little more than random code. The fact that they look nice is mostly irrelevant when there is no functionality yet. It's possible to write poetically beatiful code, only to realize it allow you to do what you want, it's missing a crucial aspect or feature which cannot be added later or that the use of the code would require dirty hacks and workarounds to do what you want it to do.
The alternative, to discuss the design, would be UML diagrams.
Hmmm..., let's see what I can find.
Well, you have a function in Card that is called isSameAs(). You could always replace that with an overridden equals operator, so instead of:
You're right, the Rank class is... interesting. It looks like it works, so that isn't really a problem. You could have done it a different way, such as have a static collection of ranks in the Card class, and then just instantiate one Rank that all cards will reference. So when you create your Card, you could say Card( Card::Ranks.Ace, Card::Suits.Clubs), and so that way you only ever have one instance of each rank and suit. Then, you can remove the rank and suit information from the main.cpp file and just give each Rank and Suit instance its own value and name variables.
Also, you can once again overload some operators, such as ==, !=, <, <=, >, >= on the rank and suit classes so that they are easily comparable. In fact, I would do this for the Card class, and then you can easily sort all of the cards in a given collection without having to refer to the underlying rank and suit. It would make the Card class more encapsulated.
Other than that, you have some very well written code, and everything is very clean. Other than the lack of comments, everything is pretty good.
Well, you have a function in Card that is called isSameAs(). You could always replace that with an overridden equals operator, so instead of:
if( card1.isSameAs(card2) )if(card1 == card2)You're right, the Rank class is... interesting. It looks like it works, so that isn't really a problem. You could have done it a different way, such as have a static collection of ranks in the Card class, and then just instantiate one Rank that all cards will reference. So when you create your Card, you could say Card( Card::Ranks.Ace, Card::Suits.Clubs), and so that way you only ever have one instance of each rank and suit. Then, you can remove the rank and suit information from the main.cpp file and just give each Rank and Suit instance its own value and name variables.
Also, you can once again overload some operators, such as ==, !=, <, <=, >, >= on the rank and suit classes so that they are easily comparable. In fact, I would do this for the Card class, and then you can easily sort all of the cards in a given collection without having to refer to the underlying rank and suit. It would make the Card class more encapsulated.
Other than that, you have some very well written code, and everything is very clean. Other than the lack of comments, everything is pretty good.
Author
Quote:Original post by Antheus
*** Source Snippet Removed ***
Until you show *how* you intend to use these classes, they are little more than random code. The fact that they look nice is mostly irrelevant when there is no functionality yet. It's possible to write poetically beatiful code, only to realize it allow you to do what you want, it's missing a crucial aspect or feature which cannot be added later or that the use of the code would require dirty hacks and workarounds to do what you want it to do.
The alternative, to discuss the design, would be UML diagrams.
Thanks Antheus and tstrimp. Will definitely get something simple up. Didn't mean the post to be some of the poetic non-sense you talked about. I just didn't want to get too far, burying myself in cruddy code.
ussnewjersery: Thanks for the tips. I will work on some operator overloading to clean it up some.
I'm still battling the rank class, I know. Your idea is something to keep in mind.
Author
Sorry for the double post. The rank class I've created is really starting to throw wrenches into my code, at least it feels like. It seems awkward because of the static members. However, it does work.
Can anyone else think of another way to do this, leaving an ability to change the card values while still being able to recognize the card under some sort of id?
The reason I did it this way is because it makes for easy changing and string output. Is this the best/only way? Should I just switch it to something simpler like a single integer value?
Can anyone else think of another way to do this, leaving an ability to change the card values while still being able to recognize the card under some sort of id?
The reason I did it this way is because it makes for easy changing and string output. Is this the best/only way? Should I just switch it to something simpler like a single integer value?
class Rank {public: Rank(); Rank(int index); bool operator== (Rank otherRank); bool operator< (Rank otherRank); bool operator> (Rank otherRank); static void setAceHigh(); static void setKingHigh(); static void setValue(int index, int value); static int getValue(int index); int getIndex(); int getValue(); char getSymbol(); string getName(); bool isSameAs(Rank otherRank); int compareTo(Rank otherRank);private: static string rankNames[]; static int rankValues[]; int index; };
The path you're headed down with the Rank class could lead to over-design, but there are some useful ideas here (and not ones that many people would think of), so I'll bite.
First off, we don't really compare (in the main code) ranks; we compare cards. A card has a rank and a suit.
Looking at the static values here, it becomes clear that ranks don't exist in a vacuum: a rule-set for a given card game specifies some set of rank values.
Cards are only comparable within the context of a common rule-set, so the 'rank' object needs to know about some rule-set to which it belongs, as well as some indication of which of the available ranks it is. Then we make a Ruleset object that contains a set of rank-names and rank-values. Of course, it makes sense to pair those values up, so we make a struct to represent a rank-name and a rank-value paired together. I'll call that a RankPrototype; it abstracts the idea of what's possible for the Rank of a Card in a given Ruleset.
A Game, then (or if you prefer, a Deck) has a bunch of Card objects, and a Ruleset. Each Card's Rank is initialized to "know about" the Game's Ruleset.
The statics now disappear by being replaced by Ruleset objects. We don't need a complicated interface for these; we just need to be able to add RankPrototypes (so that we can set the thing up), and get values and names (not set them, because we did that by creating the RankPrototypes in the first place). We certainly don't need setAceHigh() or setKingHigh(). Those don't even *mean* anything for all card games out there. But anyway, we don't want to be managing our own memory for this; the standard library provides std::vector, which we should be smart enough to be using if we're smart enough to use std::string.
This is all off the top of my head and may have problems :) But hopefully you will get some useful ideas from here.
First off, we don't really compare (in the main code) ranks; we compare cards. A card has a rank and a suit.
Looking at the static values here, it becomes clear that ranks don't exist in a vacuum: a rule-set for a given card game specifies some set of rank values.
Cards are only comparable within the context of a common rule-set, so the 'rank' object needs to know about some rule-set to which it belongs, as well as some indication of which of the available ranks it is. Then we make a Ruleset object that contains a set of rank-names and rank-values. Of course, it makes sense to pair those values up, so we make a struct to represent a rank-name and a rank-value paired together. I'll call that a RankPrototype; it abstracts the idea of what's possible for the Rank of a Card in a given Ruleset.
A Game, then (or if you prefer, a Deck) has a bunch of Card objects, and a Ruleset. Each Card's Rank is initialized to "know about" the Game's Ruleset.
The statics now disappear by being replaced by Ruleset objects. We don't need a complicated interface for these; we just need to be able to add RankPrototypes (so that we can set the thing up), and get values and names (not set them, because we did that by creating the RankPrototypes in the first place). We certainly don't need setAceHigh() or setKingHigh(). Those don't even *mean* anything for all card games out there. But anyway, we don't want to be managing our own memory for this; the standard library provides std::vector, which we should be smart enough to be using if we're smart enough to use std::string.
struct RankPrototype { string name; int value;};class Ruleset { std::vector<RankPrototype> prototypes; public: void addPrototype(const RankPrototype& rp) { prototypes.push_back(rp); } string name(int index) const; int value(int index) const; // the Deck needs the following information in order to make one // unique Card of each type: int size() const { return prototypes.size(); }};class Rank { const Ruleset* context; // That pointer does NOT "own" the Ruleset object; we will never // "new" or "delete" this pointer. It simply points to an *existing* // Ruleset, the context in which the rank makes sense. int index; public: Rank(const Ruleset& context, int index) : context(&context), index(index) {} string name() const { return context->name(index); } int value() const { return context->value(index); } bool operator==(const Rank& other) const { return context == other.context && index == other.index; } bool operator<(const Rank& other) const { return context == other.context && value() < other.value(); } // We can similarly define other comparisons. Note there are not 3 // but 6 comparison operations: <, <=, ==, !=, >= and >. // Note also that Ranks from different contexts are incomparable; // *all 6* should return false; and that for equality I require the // name to match as well as the value (by using the index directly), // but names are unimportant when we check which rank is lower. // Those might not be the semantics you want. You might not care about // the name for equality, either, for example. Or you might care about // it for == and !=, but not for <= and >=.};class Card { Rank r; Suit s; public: bool operator==(const Card& other) { return r == other.r && s == other.s; } bool operator<(const Card& other) { return r < other.r || (r == other.r && s < other.s); } // for example. string name() { return r.name() + " of " + s.name(); }};This is all off the top of my head and may have problems :) But hopefully you will get some useful ideas from here.
let me see what i can find. i will update here as i see stuff.
1.) i would avoid the use of #pragmas (#pragma once in a cpp file? why?)
2.) some of your member functions should be labeled const
3.) i would avoid using using in the global namespace
4.) separate your member functions in your cpp file with an extra line break
5.) why do u need an "error" rank and suit?
6.) why are the functions in rank static? this can be handled better in your implementation
7.) why use compareTo? are you a closet Java/C# programmer ;-) he he he
8.) too much passing by value (use references and const references when you can)
9.) the rest seems aight. good luck in finishing your card game!
1.) i would avoid the use of #pragmas (#pragma once in a cpp file? why?)
2.) some of your member functions should be labeled const
3.) i would avoid using using in the global namespace
4.) separate your member functions in your cpp file with an extra line break
5.) why do u need an "error" rank and suit?
6.) why are the functions in rank static? this can be handled better in your implementation
7.) why use compareTo? are you a closet Java/C# programmer ;-) he he he
8.) too much passing by value (use references and const references when you can)
9.) the rest seems aight. good luck in finishing your card game!
Author
Wow! Thanks Zahlman. Thats a very interesting way of doing it and certainly something I wouldn't be capable of creating on my own. I'm not sure I follow the idea perfectly just yet, but I will definitely fiddle with it. How exactly would that be implemented to the card? Would I just create an instance of the Ruleset class and push_back structs right at the start of the program then create one instance of the Rank class or multiple?
It is leagues better that what I have so I appreciate it! Would I do something for the Suit class? I know I didn't mention it in my previous post, but it was structured very much the same as the Rank class.
Thanks for the check list Yadango, I will run through all those tips. If I do something strange it is probably because I misunderstood some sort of concept along the way. So if you see something out of the ordinary, that would explain why.
It is leagues better that what I have so I appreciate it! Would I do something for the Suit class? I know I didn't mention it in my previous post, but it was structured very much the same as the Rank class.
Thanks for the check list Yadango, I will run through all those tips. If I do something strange it is probably because I misunderstood some sort of concept along the way. So if you see something out of the ordinary, that would explain why.
This topic is closed to new replies.
Advertisement
Popular Topics
Advertisement
