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?
3
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.
6
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.
1