I'm writing a minesweeper game with curses
. To represent each tile, I wrote small Tile
class that contains information about the possible state of each tile (whether or not it's flagged, whether or not it has a bomb under it, and whether or not it's "hidden"). This is the interface so far:
class Tile {
bool hidden;
bool containsBomb;
bool hasFlag;
public:
Tile(bool startWithBomb, bool startHidden = true);
bool isHidden() const;
void makeHidden();
void unhide();
bool hasBomb() const;
void placeBomb();
void removeBomb();
bool hasFlag() const;
void placeFlag();
void removeFlag();
};
I could have made the setters for each a single function, but I think them being separate will lead to more readable code (tile.setFlag(false)
vs tile.removeFlag()
)
My problem is, this can potentially lead to "bad states". An unhidden tile shouldn't be able to contain a flag for example.
I was reading over this post, and several users "bash" using getters/setters; with good reasons.
My question is: Before I get farther in designing this class, is there a better way to set this up to avoid using generic getter/setters for each private flag?
-
I read over the linked post and the anti getter/setter article attached. Seems like C++ and C# have very different philosophies.Stephen– Stephen2015年05月08日 04:59:45 +00:00Commented May 8, 2015 at 4:59
-
@Stephen Yes. AFAIK, c# has shortcuts for writing getters/setters.Carcigenicate– Carcigenicate2015年05月08日 13:15:00 +00:00Commented May 8, 2015 at 13:15
-
even without those shortcuts it's always been considered good practice to expose properties, even with simple getters/setters.Stephen– Stephen2015年05月11日 00:09:20 +00:00Commented May 11, 2015 at 0:09
2 Answers 2
Yes, having exactly one getter/setter pair per private flag is a code smell. If you aren't providing a simple interface to hide some implementation details, you're not benefiting very much from using a class.
My main concrete suggestion would be to represent the internal state in a way that makes "bad states" theoretically impossible. For instance:
class Tile {
enum DisplayState { DEFAULT, FLAGGED, KNOWN_EMPTY };
DisplayState displayState;
bool containsBomb;
...
};
Notice that containsBomb
can be true or false in any of the three states, so it makes sense to keep that member separate. It's up to you whether things like calling removeFlag() on a KNOWN_EMPTY tile should throw an exception or simply do nothing.
Now the big-picture issue. If the part of the code that uses Tile objects really does need all of these methods, i.e. full access to and control of the Tile's entire state, then it's not really clear how you benefit from making Tile a class (as opposed to a POD struct) in the first place. Perhaps it would be better to have a Board class with a few 2D arrays of booleans to represent all the flags/bombs/etc, since that class would be able to hide a lot of iteration logic from its users. I can't say for sure if that'd be an improvement without reading all of your code, but hopefully that helps.
-
I was going to make it a 2D array of
Tile
s. If the user clicks on a tile, theBoard
would issue a command to the clickedTile
. What is a POD Struct? Thank you for your response.Carcigenicate– Carcigenicate2015年05月07日 20:06:46 +00:00Commented May 7, 2015 at 20:06 -
POD = Plain Old Data. The technical definition is complicated (nothing is simple in C++), but in this case I'm using it to mean a struct with just members and no methods.Ixrec– Ixrec2015年05月07日 20:09:09 +00:00Commented May 7, 2015 at 20:09
-
Oh, like using the class as a "bag of data"?Carcigenicate– Carcigenicate2015年05月07日 20:09:53 +00:00Commented May 7, 2015 at 20:09
-
Exactly. Regarding the first half of your comment: Is it simpler for the Board to implement setFlag() as flagsArray[x][y] = true, or as tilesArray[x][y].setFlag(true)? Again, I can't know for sure, but my guess would be that the former has a slight edge.Ixrec– Ixrec2015年05月07日 20:11:30 +00:00Commented May 7, 2015 at 20:11
-
I just kind of dived in with general ideas on how I wanted it to work, so I'm not sure exactly which would be cleaner yet. I'm probably going to need to do some refactoring down the road. Wouldn't storing all the data in one array be more efficient than storing bools across a few array representing different parts of the state of the tile? I know vector has the specialization for bools, but that would still require each update to iterate over 3 arrays instead of 1.Carcigenicate– Carcigenicate2015年05月07日 20:15:19 +00:00Commented May 7, 2015 at 20:15
My problem is, this can potentially lead to "bad states".
The opposite is true: The setter allows you to prevent "bad states". You are the one to define the behaviour of the state machine, by specifying what happens when one variable changes.
An unhidden tile shouldn't be able to contain a flag for example.
Therefore, calling unhide()
should set hasFlag
to false and placeFlag()
should only set hasFlag
to true if hidden
is true.
There is no such thing as a "bad state" because you should define your state machine well and handle every case. These setters are just the implementation of that state machine.
Reaching a "bad state" is a flaw in the transfer function.
-
1Right. The reason for making private variables is to control how they are used with an interface you define. Make a good interface, don't expose the hidden members!Alan Wolfe– Alan Wolfe2015年05月09日 04:37:17 +00:00Commented May 9, 2015 at 4:37