- What is the single responsibility of the
World class?
It's just a blob containing practically every kind of functionality. That's not good design. One obvious responsibility is "represent the grid onto which blocks are placed". But that has nothing to do with creating tetroids or manipulating block lists or drawing. In fact, most of that probably doesn't need to be in a class at all. I would expect the World object to contain the BlockList you call StaticBlocks so it can define the grid on which you're playing.
- Why do you define your own
Blocklist? You said you wanted your code to be generic, so why not allow any container to be used? Why can't I use a std::vector<Block> if I want to? Or a std::set<Block>, or some home-brewed container?
- Use simple names that don't duplicate information or contradict themselves.
TranslateTetroid doesn't translate a tetroid. It translates all the blocks in a blocklist. So it should be TranslateBlocks or something. But even that is redundant. We can see from the signature (it takes a BlockList&) that it works on blocks. So just call it Translate.
- Try to avoid C-style comments (
/*...*/). C++-style (//..)behaves a bit nicer in that if you use a C-style comment out an entire block of code, it'll break if that block also contained C-style comments. (As a simple example, /*/**/*/ won't work, as the compiler will see the first */ as the end of the comment, and so the last */ won't be considered a comment.
- What's with all the (unnamed)
int parameters? It's making your code impossible to read.
- Respect language features and conventions. The way to copy an object is using its copy constructor. So rather than a
CopyTetroid function, give BlockList a copy constructor. Then if I need to copy one, I can simply do BlockList b1 = b0.
- Rather than
void SetX(Y) and Y GetX() methods, drop the redundant Get/Set prefix and simply have void X(Y) and Y X(). We know it's a getter because it takes no parameters and returns a value. And we know the other one is a setter because it takes a parameter and returns void.
BlockList isn't a very good abstraction. You have very different needs for "the current tetroid" and "the list of static blocks currently on the grid". The static blocks can be represented by a simple sequence of blocks as you have (although a sequence of rows, or a 2D array, may be more convenient), but the currently active tetroid needs additional information, such as the center of rotation (which doesn't belong in the World).
- A simple way to represent a tetroid, and to ease rotations, might be to have the member blocks store a simple offset from the center of rotation. That makes rotations easier to compute, and means that the member blocks don't have to be updated at all during translation. Just the center of rotation has to be moved.
- In the static list, it isn't even efficient for blocks to know their location. Instead, the grid should map locations to blocks (if I ask the grid "which block exists in cell
(5,8), it should be able to return the block. but the block itself doesn't need to store the coordinate. If it does, it can become a maintenance headache. What if, due to some subtle bug, two blocks end up with the same coordinate? That can happen if blocks store their own coordinate, but not if the grid holds a list of which block is where.)
- this tells us that we need one representation for a "static block", and another for a "dynamic block" (it needs to store the offset from the tetroid's center). In fact, the "static" block can be boiled down to the essentials: Either a cell in the grid contains a block, and that block has a colour, or it does not contain a block. There is no further behavior associated with these blocks, so perhaps it is the cell into which it is placed that should be modelled instead.
- and we need a class representing a movable/dynamic tetroid.
- Since a lot of your collision detection is "predictive" in that it deals with "what if I moved the object over here", it may be simpler to implement non-mutating translation/rotation functions. These should leave the original object unmodified, and a rotated/translated copy returned.
So here's a first pass on your code, simply renaming, commenting and removing code without changing the structure too much.
class World
{
public:
// Constructor/Destructor
// the constructor should bring the object into a useful state.
// For that, it needs to know the dimensions of the grid it is creating, does it not?
World(int width, int height);
~World();
// none of thes have anything to do with the world
///* Blocks Operations */
//void AppendBlock(int, int, BlockList&);
//void RemoveBlock(Block*, BlockList&);;
// Tetroid Operations
// What's wrong with using BlockList's constructor for, well, constructing BlockLists? Why do you need NewTetroid?
//void NewTetroid(int, int, int, BlockList&);
// none of these belong in the World class. They deal with BlockLists, not the entire world.
//void TranslateTetroid(int, int, BlockList&);
//void RotateTetroid(int, BlockList&);
//void CopyTetroid(BlockList&, BlockList&);
// Drawing isn't the responsibility of the world
///* Draw */
//void DrawBlockList(BlockList&);
//void DrawWalls();
// these are generic functions used to test for collisions between any two blocklists. So don't place them in the grid/world class.
///* Collisions */
//bool TranslateCollide(int, int, BlockList&, BlockList&);
//bool RotateCollide(int, BlockList&, BlockList&);
//bool OverlapCollide(BlockList&, BlockList&); // For end of game
// given that these functions take the blocklist on which they're operating as an argument, why do they need to be members of this, or any, class?
// Game Mechanics
bool AnyCompleteLines(BlockList&); // Renamed. I assume that it returns true if *any* line is complete?
bool IsLineComplete(int line, BlockList&); // Renamed. Avoid ambiguous names like "CompleteLine". is that a command? (complete this line) or a question (is this line complete)?
void ColourLine(int line, BlockList&); // how is the line supposed to be coloured? Which colour?
void DestroyLine(int line, BlockList&);
void DropLine(int, BlockList&); // Drops all blocks above line
// bad terminology. The objects are rotated about the Z axis. The x/y coordinates around which it is rotated are not axes, just a point.
int rotationAxisX;
int rotationAxisY;
// what's this for? How many rotation states exist? what are they?
int rotationState; // Which rotation it is currently in
// same as above. What is this, what is it for?
int rotationModes; // How many diff rotations possible
private:
int wallX1;
int wallX2;
int wallY1;
int wallY2;
};
// The language already has perfectly well defined containers. No need to reinvent the wheel
//class BlockList
//{
//public:
// BlockList();
// ~BlockList();
//
// Block* GetFirst();
// Block* GetLast();
//
// /* List Operations */
// void Append(int, int);
// int Remove(Block*);
// int SearchY(int);
//
//private:
// Block *first;
// Block *last;
//};
struct Colour {
int r, g, b;
};
class Block
{
public:
Block(int x, int y);
~Block();
int X();
int Y();
void Colour(const Colour& col);
void Translate(int down, int left); // add parameter names so we know the direction in which it is being translated
// what were the three original parameters for? Surely we just need to know how many 90-degree rotations in a fixed direction (clockwise, for example) are desired?
void Rotate(int cwSteps);
// If rotate/translate is non-mutating and instead create new objects, we don't need these predictive collision functions.x ½
//// Return values simulating the operation (for collision purposes)
//int IfTranslateX(int);
//int IfTranslateY(int);
//int IfRotateX(int, int, int);
//int IfRotateY(int, int, int);
// the object shouldn't know how to draw itself. That's building an awful lot of complexity into the class
//void Draw();
//Block *next; // is there a next? How come? What does it mean? In which context?
private:
int x; // position x
int y; // position y
Colour col;
//int colourR;
//int colourG;
//int colourB;
};
// Because the argument block is passed by value it is implicitly copied, so we can modify that and return it
Block Translate(Block bl, int down, int left) {
return bl.Translate(down, left);
}
Block Rotate(Block bl, cwSteps) {
return bl.Rotate(cwSteps);
}
Now, let's add some of the missing pieces:
First, we'll need to represent the "dynamic" blocks, the tetroid owning them, and the static blocks or cells in a grid.
(We'll also add a simple "Collides" method to the world/grid class)
class Grid
{
public:
// Constructor/Destructor
Grid(int width, int height);
~Grid();
// perhaps these should be moved out into a separate "game mechanics" object
bool AnyCompleteLines();
bool IsLineComplete(int line);
void ColourLine(int line, Colour col);Which colour?
void DestroyLine(int line);
void DropLine(int);
int findFirstInColumn(int x, int y); // Starting from cell (x,y), find the first non-empty cell directly below it. This corresponds to the SearchY function in the old BlockList class
// To find the contents of cell (x,y) we can do cells[x + width*y]. Write a wrapper for this:
Cell& operator()(int x, int y) { return cells[x + width*y]; }
bool Collides(Tetroid& tet); // test if a tetroid collides with the blocks currently in the grid
private:
// we can compute the wall positions on demand from the grid dimensions
int leftWallX() { return 0; }
int rightWallX() { return width; }
int topWallY() { return 0; }
int bottomWallY { return height; }
int width;
int height;
// let this contain all the cells in the grid.
std::vector<Cell> cells;
};
// represents a cell in the game board grid
class Cell {
public:
bool hasBlock();
Colour Colour();
};
struct Colour {
int r, g, b;
};
class Block
{
public:
Block(int x, int y, Colour col);
~Block();
int X();
int Y();
void X(int);
void Y(int);
void Colour(const Colour& col);
private:
int x; // x-offset from center
int y; // y-offset from center
Colour col; // this could be moved to the Tetroid class, if you assume that tetroids are always single-coloured
};
class Tetroid { // since you want this generalized for more than just Tetris, perhaps this is a bad name
public:
template <typename BlockIter>
Tetroid(BlockIter first, BlockIter last); // given a range of blocks, as represented by an iterator pair, store the blocks in the tetroid
void Translate(int down, int left) {
centerX += left;
centerY += down;
}
void Rotate(int cwSteps) {
typedef std::vector<Block>::iterator iter;
for (iter cur = blocks.begin(); cur != blocks.end(); ++cur){
// rotate the block (*cur) cwSteps times 90 degrees clockwise.
// a naive (but inefficient, especially for large rotations) solution could be this:
// while there is clockwise rotation left to perform
for (; cwSteps > 0; --cwSteps){
int x = -cur->Y(); // assuming the Y axis points downwards, the new X offset is simply the old Y offset negated
int y = cur->X(); // and the new Y offset is the old X offset unmodified
cur->X(x);
cur->Y(y);
}
// if there is any counter-clockwise rotation to perform (if cwSteps was negative)
for (; cwSteps < 0; --cwSteps){
int x = cur->Y();
int y = -cur->X();
cur->X(x);
cur->Y(y);
}
}
}
private:
int centerX, centerY;
std::vector<Block> blocks;
};
Tetroid Translate(Tetroid tet, int down, int left) {
return tet.Translate(down, left);
}
Tetroid Rotate(Tetroid tet, cwSteps) {
return tet.Rotate(cwSteps);
}
and we'll need to re-implement the speculative collision checks. Given the non-mutating Translate/Rotate methods, that is simple: We just create rotated/translated copies, and test those for collision:
// test if a tetroid t would collide with the grid g if it was translated (x,y) units
if (g.Collides(Translate(t, x, y))) { ... }
// test if a tetroid t would collide with the grid g if it was rotated x times clockwise
if (g.Collides(Rotate(t, x))) { ... }
