Register a SA Forums Account here!
JOINING THE SA FORUMS WILL REMOVE THIS BIG AD, THE ANNOYING UNDERLINED ADS, AND STUPID INTERSTITIAL ADS!!!

You can: log in, read the tech support FAQ, or request your lost password. This dumb message (and those ads) will appear on every screen until you register! Get rid of this crap by registering your own SA Forums Account and joining roughly 150,000 Goons, for the one-time price of $9.95! We charge money because it costs us money per month for bills, and since we don't believe in showing ads to our users, we try to make the money back through forum registrations.
 
  • Locked thread
Gasbraai
Oct 25, 2010

Lictor my Dictor
I am trying to teach myself the intricacies of C++, just for my own gratification. I have pretty much finished a small C++ project which can be found here:

https://github.com/wvandyk/tetris

I have found code reviews from experienced developers to be very helpful in the past. I write Ruby and sometimes Clojure for a living and doing it as a day job gives you a lot of exposure to other professionals with professional opinions and experience.

Since we instituted pair programming at my job, my knowledge of the languages I use professionally has improved in leaps and bounds.
I am hoping we could do the same for each other for hobby languages or frameworks we are trying to learn.
Any C++ pros up for the challenge of my project?

Feel free to post your own projects for a review, if its Ruby I'll be willing to take a look.

Issues I feel need attention in the above C++ project:

1) I have a couple of headers that have forward definitions for certain classes they are using - I'm sure its a smell of some sort to do that.
2) The main function is a gross loopy event polling load of poo poo, I want to take it apart and shove the SDL event stuff into its own class.
3) The Renderer class is very specific to the project, I would like to make it a bit more generic and reusable.
4) I did not do any TDD on this project - mainly because I could not get googletest / googlemock working in VS Express. I would have liked to test the game logic with regards to the movements of the pieces, maybe I could have implemented t-spin triples and other wacky moves if I had a nice framework of unit tests to hash it out in.

Adbot
ADBOT LOVES YOU

shrughes
Oct 11, 2008

(call/cc call/cc)

Lunixnerd posted:

1) I have a couple of headers that have forward definitions for certain classes they are using - I'm sure its a smell of some sort to do that.

Generally speaking that's a good thing, not a smell.

(Ctrl+F for "NOTE"...)

Animator.h:
code:
#ifndef __ANIMATOR__
#define __ANIMATOR__
// ^ NOTE: Names with two underscores in a row are reserved for the
// system in some standards (maybe C++?).  I recommend using
// ANIMATOR_H_ or YOURPROJECTSPECIFICPREFIX_ANIMATOR_H_.  (You also
// can't use a single prefix underscore followed by a capital letter
// in some standards.)

// NOTE: LOL DOS line endings.

#include "Renderer.h"

class Renderer;

class Animator {
	int frame = 0;
	int frame_count = 10;
public:
	Animator();

        // NOTE: These functions are virtual with implementations.
        // But the only actual subclass, BlockAnimator, overrides both
        // of these implementations!  So frame and frame_count are
        // completely useless private variables, and the
        // implementations of these functions in Animator.cpp is
        // completely dead code.
        //
        // Virtual methods having overridden implementations is a
        // smell in any language -- there is no reason why this
        // shouldn't just be an abstract class.

	virtual bool nextFrame(void);  // <- NOTE: No need for "void"
                                       // here.  (likewise in the cpp
                                       // file.)
        // NOTE:  virtual bool nextFrame() = 0;  // and no implementation.

	virtual void render(Renderer &r);
        // ^ NOTE: I'll hold off on complaining about non-const
        // references for now, as long as you stick to using them only
        // with intrinsically noncopyable types.

        // NOTE:  virtual void render(Renderer &r) = 0;  // and no implementation.

        // NOTE: (And the dead fields frame and frame_count should be
        // removed.)

	virtual ~Animator() {};  // <- NOTE: Semicolon unnecessary,
                                 // should be omitted (because it
                                 // looks noobish).

        // NOTE: Is this type designed to be copied?  If not, you
        // should have these somewhere:
        //
        //   Animator(const Animator &) = delete;
        //   void operator=(const Animator &) = delete;
        //
        // Right now the copy constructor and assignment operator have
        // default implementations.
};

// vvv NOTE: I put an "// __ANIMATOR__" comment here to help
// the reader match #endifs with their corresponding #ifs.
#endif  // __ANIMATOR__
Animator.cpp:
code:
#include "Animator.h"

Animator::Animator() {};  // <- NOTE: Semicolons should not be put
                          // after function definitions (because
                          // they're unnecessary and look noobish).

bool Animator::nextFrame(void) {  // <- NOTE: No need for "void" here.
                                  // Just write ...::nextFrame().  In
                                  // C++ that means the same thing.
                                  // (Likewise in the header.)
	if (frame == frame_count) {
		return false;
	}
	frame++;  // <- NOTE: Just use prefix increment for
                  // everything.  It is less expensive for certain
                  // iterator types, equivalent for primitive numbers,
                  // so make it a habit.
	return true;
};

void Animator::render(Renderer &r) {
	return;
}

// NOTE: A newline was missing at the end of this file.  That is bad
// for some ancient reason.

shrughes
Oct 11, 2008

(call/cc call/cc)
BlockAnimator.h:
code:
#ifndef __BLOCK_ANIMATOR__
#define __BLOCK_ANIMATOR__

// NOTE: Other projects' header files should not be included after
// yours!  One reason is, you'd rather have name conflicts happen in
// _your_ header files, instead of seeing them cryptically in others'.
// There are probably other good reasons -- I'm parroting the Google
// C++ Style Guide.  An exception is the .cpp file's corresponding .h
// file, which should go first, to make sure that the .h file includes
// or declares all of its dependencies.

#include "Animator.h"
#include <SDL.h>

class BlockAnimator : public Animator {
	int z;
	const int centerx;
	const int centery;
	int x;
	int y;
	int x2;
	int y2;
	const int block;
	const Uint64 ticktime;
	// Uint64 last_tick = SDL_GetTicks(); // <- NOTE: using member
	// initializers not inside a constructor for stateful behavior
	// like here is insane.
        Uint64 last_tick;

        // ^ NOTE: Some of these fields can (and should) be marked
        // const because they are constant for the lifetime of the
        // object, and that makes them more readable.  (Some could
        // also be static, since they're global constants.  Also, I
        // would generally move all these const fields up to the top
        // (as a default choice), except maaybe for ticktime.  It's
        // generally nice to say to the reader, ">HERE< is the
        // stateful part of the object and <THERE< is the const part
        // of the object" if there's not an overriding reason to do
        // different.

public:
        // NOTE: It's not important immediately but I think you'll
        // find that declaring and implementing the destructor next to
        // the constructors is what you end up preferring in the long
        // run.

        // NOTE: (x,y) pairs can go in a point type?  I don't know yet.

        // NOTE: I'm guessing (at first glance) block should be an
        // enum type.  enum class Block { Line, Z, S, blah blah blah
        // };
	BlockAnimator(int x, int y, int x2, int y2, int block);
	bool nextFrame(void);
	void render(Renderer &r);
	~BlockAnimator();
};

#endif
BlockAnimator.cpp:
code:
#include "BlockAnimator.h"
#include "Renderer.h"
#include <iostream>

// vvv NOTE: The parameter names of the constructor should not be the
// same as the field names.  (Your compiler should have given you a
// warning!)  I would use postfix underscores on private field names,
// myself.  If you don't, then you should use a prefix underscore
// (followed by a lowercase character) or a postfix underscore on the
// parameter names, to prevent a name conflict.
// BlockAnimator::BlockAnimator(int x, int y, int x2, int y2, int block) {
// 	centery = ((y / 32) - 2) * 32;
// 	this->x = x - centerx;
// 	this->y = y - centery;
// 	this->x2 = x2 - centerx;
// 	this->y2 = y2 - centery;
// 	this->block = block;
// };

// ^ NOTE: You should have used a member initializer list to
// initialize the members.  (This also lets you fields that are
// constant for the lifetime of the object as const -- see
// BlockAnimator.h.)  Here is a replacement implementation:
BlockAnimator::BlockAnimator(int _x, int _y, int _x2, int _y2, int _block)
    : z(256),
      centerx(4 * 32 + 64),
      centery((_y / 32 - 2) * 32),
      x(_x - centerx),  // NOTE: [1]
      y(_y - centery),
      x2(_x2 - centerx),
      y2(_y2 - centery),
      block(_block),
      ticktime(10),
      last_tick(SDL_GetTicks()) { }

// NOTE [1]: It's okay to use centerx here because centerx is
// initialized before x.  centerx is initialized before x because the
// field is declared before x.  The member initializer list is also
// written that way.  Your compiler should emit a warning if its order
// is inconsistent with field order i.e. initialization order!


bool BlockAnimator::nextFrame(void) {
	Uint64 tick = SDL_GetTicks();
	if (tick - last_tick >= ticktime) {
		last_tick = tick;
                // NOTE: What's with all the "this" keyword usage?
                // Just use x and x2, etc.  If you want to make it
                // clear they're members, name them with a postfix
                // underscore, as x_ and x2_, etc.
		if ((this->x + centerx <= 0) && (this->x2 + centerx <= 0) ||
			(this->x + centerx >= 800) && (this->x2 + centerx >= 800) ||
			(this->y + centery <= 0) && (this->y2 + centery <= 0) ||
			(this->y + centery >= 768) && (this->y2 + centery >= 768) || z <= 100) {
			return false;
		}
		else
		{
			this->x = this->x * 256 / z;
			this->y = this->y * 256 / z;
			this->x2 = this->x2 * 256 / z;
			this->y2 = this->y2 * 256 / z;
			z = z - 2;
		}
		return true;
	}
	return true;
};

void BlockAnimator::render(Renderer &r) {
	r.renderBlock(x + centerx, y + centery, x2 - x, y2 - y, block - 1);
}

BlockAnimator::~BlockAnimator() {
}
GameLogic.h:
code:
#ifndef __GAMELOGIC__
#define __GAMELOGIC__
#include "PlayField.h"
#include "Tetri.h"
#include "Animator.h"

class Animator;

class GameLogic {
private:
	// v NOTE: Why are there all these initializers on these
	// fields, when some of them get reinitialized in the actual
	// constructor?  lockTimeOut ends up getting the value 800, so
	// why is it assigned 1600 here?  You should initialize all
	// the fields of an object in the same place, not half of them
	// in the header and half in the cpp file.
	Uint64 lockTimeOut = 1600;
	Uint64 dropTimeout = 800;
	Uint64 lastDrop = 0;
	Uint64 score = 0;
	Uint64 lines_completed = 0;
	int lines_to_level = 10;
	Uint64 level = 0;
	std::vector<int> blockbag;
	std::vector<Animator *> *alist;
        // v NOTE: It's so loving easy to typo nextPiece into
        // nnextPiece.
	Tetri *nnextPiece = NULL;
	Tetri *nextPiece = NULL;
public:
	GameLogic(std::vector<Animator *> &animlist);
	bool adjustFit(PlayField &board, Tetri &block);
	bool kickFit(PlayField &board, Tetri &block);
	bool checkFit(PlayField &p, Tetri &block);
	bool addBlock(PlayField &p, Tetri &block);
	void moveBlockLeft(PlayField &p, Tetri &block);
	void moveBlockRight(PlayField &p, Tetri &block);
	void moveBlockDown(PlayField &p, Tetri &block);
	void RotateBlockCW(PlayField &p, Tetri &block);
	void RotateBlockCCW(PlayField &p, Tetri &block);
	void GravityBlockDown(PlayField &p, Tetri &block);
	Uint64 getScore(void);
	Uint64 getLevel(void);
	Uint64 getLines(void);
	Tetri *nextBlock(void);
	Tetri *drawPiece(void);
	Tetri *getNextPiece(void);
	int clearLines(PlayField &p);
	void reset(void);
	~GameLogic(void);
};

#endif
GameLogic.cpp:
code:
#include "GameLogic.h"
#include "Animator.h"
#include "BlockAnimator.h"
#include <cstdio>
#include <cstdlib>
#include <ctime>

GameLogic::GameLogic(std::vector<Animator *> &animlist) {
	lockTimeOut = 800;
	dropTimeout = 440;
	lastDrop = 0;
        // v NOTE: There is no need to use .reserve(7) right before
        // assigning a 7-element initializer_list on the subsequent
        // line.
	blockbag.reserve(7);
	blockbag = { 1, 2, 3, 4, 5, 6, 7 };
        // v NOTE: This is _not_ the place to globally seed the random
        // number generator.  That would be in main.
	std::srand(std::time(NULL));
        // v NOTE: Why not pass a pointer to vector as animlist?  Then
        // the caller sees that they're passing a pointer, instead of
        // wondering whether it gets passed by value or by reference
        // or copied in.  Probably there is a design problem about
        // this too, I haven't looked at it yet.
	alist = &animlist;
	nnextPiece = NULL;
	nextPiece = drawPiece();
}

int GameLogic::clearLines(PlayField &p) {
	int lcount = 0;
	int cleared = 0;

	for (int y = 0; y < 22; y++) {
		for (int x = 0; x < 10; x++) {
			if (p.getBoardAt(x, y) != 0) {
				lcount++;
			}
		}
		if (lcount == 10) {
			cleared++;
			for (int x = 0; x < 10; x++) {
				alist->push_back(new BlockAnimator(x * 32 + 64, y * 32, x * 32 + 64 + 31, y * 32 + 31, p.getBoardAt(x, y)));
				p.setBoardAt(x, y, 0);
			}
			for (int yy = y; yy > 0; yy--) {
				for (int xx = 0; xx < 10; xx++) {
					int tv = p.getBoardAt(xx, yy - 1);
					p.setBoardAt(xx, yy, tv);
				}
			}
		}
		lcount = 0;
	}
	if (cleared > 0) {
		
		lines_completed = lines_completed + cleared;
		Uint64 line_score = (100 * cleared * cleared);
		if (level > 0) {
			line_score = line_score * level;
		}

		score = score + line_score;
		lines_to_level = lines_to_level - cleared;
		if (lines_to_level <= 0) {
			level++;
			lines_to_level = 10 + (level * 2);

                        // v NOTE: Just use dropTimeout =
                        // std::max<Uint64>(dropTimeout - 40, 80);
                        //
                        // But what if dropTimeout (a Uint64) was less
                        // than 40 somehow!  (Because of future
                        // changes to the logic somewhere.)  Then
                        // it'll wrap around to a super-high unsigned
                        // value, and not get clamped to 80 properly!
                        // This is one of many examples why you should
                        // use signed types by default.
			dropTimeout = dropTimeout - 40;
			if (dropTimeout < 80) {
				dropTimeout = 80;
			}
		}
		std::cout << "Score: " << score << std::endl;
		std::cout << "Level: " << level << std::endl;
		std::cout << "Lines: " << lines_completed << std::endl;
		std::cout << "Lines to Level: " << lines_to_level << std::endl;
		std::cout << "droptimeout: " << dropTimeout << std::endl;
	}
        // NOTE: You _could_ just have a static const char *const
        // arr[] = { "Single!", "Double!", "Triple!", "TETRIS!" };
        // std::cout << arr[cleared - 1] << std::endl;
	switch (cleared) {
	case 1:
		std::cout << "Single!" << std::endl;
		break;
	case 2:
		std::cout << "Double!" << std::endl;
		break;
	case 3:
		std::cout << "Triple!" << std::endl;
		break;
	case 4:
		std::cout << "TETRIS!" << std::endl;
		break;
	}
	return cleared;
}

// v NOTE: One does not simply return a pointer to a freshly allocated
// object on the heap.  If you were writing C, the right way to return a fresh
// heap pointer would be:
//
// void drawPiece(Tetri **ptr_out)
//
// maybe with a name saying "create" or "alloc" or something.  That
// way, the caller has to explicitly put the pointer in a variable,
// that they'll see and remember to free.  They can't accidentally
// pass the result directly to a function that doesn't free the
// pointer.
//
// In C++, you'd still want to treat raw pointers the same way, but
// instead here this function should return a std::unique_ptr<Tetri>.
Tetri *GameLogic::drawPiece(void) {
	// v NOTE: npiece isn't used until the switch statement below,
	// so why is it used here?
	Tetri *npiece;
	int r = std::rand() % blockbag.size();
	int piece = blockbag[r];
	blockbag.erase(blockbag.begin() + r);

	if (blockbag.size() == 0) {
		blockbag = { 1, 2, 3, 4, 5, 6, 7 };
	}

	switch (piece) {
	// NOTE: It would be perfectly fine to have
	// case 1: return new Iblock();
	// case 2: return new Lblock();
	// ...
        // (ignoring that you'd really want to return
        // std::unique_ptr<Iblock>(new Iblock())) -- or
        // std::make_unique<IBlock>() in C++14.

	case 1:
		npiece = new Iblock();
		break;
	case 2:
		npiece = new Lblock();
		break;
	case 3:
		npiece = new Jblock();
		break;
	case 4:
		npiece = new Oblock();
		break;
	case 5:
		npiece = new Sblock();
		break;
	case 6:
		npiece = new Tblock();
		break;
	case 7:
		npiece = new Zblock();
		break;
	default:
		// NOTE: Returning NULL in an impossible case is nuts.
		// A likely explanation is a memory error causing
		// corruption getting you to this state, so you should
		// just crash.
		npiece = NULL;
		break;
	}

	return npiece;
}

Tetri *GameLogic::nextBlock(void) {
	// v NOTE: There is no reason for rpiece to be assigned NULL
	// immediately before being assigned the value of nextPiece.
	//
	// Also, same comment about implicitly passing ownership of
	// pointers around applies -- this should return a
	// std::unique_ptr<Tetri>.
	Tetri *rpiece = NULL;
	rpiece = nextPiece;
	nnextPiece = drawPiece();
	nextPiece = nnextPiece;
	return rpiece;
}

Tetri *GameLogic::getNextPiece(void) {
	// NOTE: Hopefully the caller of this function _doesn't_
	// except ownership of the pointer.

	// NOTE: Hopefully nothing happens that results in nextPiece
	// being deleted while the caller is using its return value.
	// This is a fragile interface...
	return nextPiece;
}

bool GameLogic::adjustFit(PlayField &board, Tetri &block) {
	int original_x = block.get_x();
	int original_y = block.get_y();
	if (checkFit(board, block)) {
		return true;
	}
	else {
		// v NOTE: foo == false should be written !foo.
		while (((block.xOfLeftMostBlock() + block.get_x()) < 0) && (checkFit(board, block) == false)) {
			block.set_x(block.get_x() + 1);
		}
		while (((block.xOfRightMostBlock() + block.get_x()) > 9) && (checkFit(board, block) == false)) {
			block.set_x(block.get_x() - 1);
		}
	}
	if (!checkFit(board, block)) {
		block.set_y(original_y);
		block.set_x(original_x);
		return false;
	}
	return true;
};

bool GameLogic::kickFit(PlayField &board, Tetri &block) {
	int original_x = block.get_x();
	int original_y = block.get_y();

	if (!checkFit(board, block)) {
		block.set_y(original_y);
		block.set_x(original_x + 1);
		if (!checkFit(board, block)) {
			block.set_x(original_x - 1);
			if (!checkFit(board, block)) {
				block.set_x(original_x);
				return false;
			}
			return true;
		}
		block.set_x(original_x);
	}
	return false;
}

bool GameLogic::checkFit(PlayField &p, Tetri &block) {
	// vvv NOTE: This looks sketchy, hopefully I get back to this.
	// (This can be a const int *?)
	int *frame = block.getFrame();
	// vvv NOTE: This function is complicated enough that these
	// variables should be const.
	int pos_x = block.get_x();
	int pos_y = block.get_y();

	// NOTE: Unless I'm mistaken, block can be passed as a const
	// reference (a const Tetri &block) if its methods are
	// properly declared const.  PlayField can too, presumably, if
	// its methods are properly declared as const.

	for (int i_y = 0; i_y < 4; i_y++) {
		for (int i_x = 0; i_x < 4; i_x++) {
			if (frame[i_x + i_y * 4] != 0) {
				if (!((p.checkInBounds(i_x + pos_x, i_y + pos_y)) && p.boardEmptyAt(i_x + pos_x, i_y + pos_y))) {
					return false;
				}
			}
		}
	}
	return true;
};

bool GameLogic::addBlock(PlayField &p, Tetri &block) {
	int *frame = block.getFrame();
	int pos_x = block.get_x();
	int pos_y = block.get_y();

	for (int i_y = 0; i_y < 4; i_y++) {
		for (int i_x = 0; i_x < 4; i_x++) {
			if (frame[i_x + i_y * 4] != 0) {
				if ((p.checkInBounds(i_x + pos_x, i_y + pos_y)) && p.boardEmptyAt(i_x + pos_x, i_y + pos_y)) {
					p.setBoardAt(i_x + pos_x, i_y + pos_y, frame[i_x + i_y * 4]);
				}
				else
				{
					return false;
				}

                                // ^ NOTE: Just use `} else {` srsly.
			}
		}
	}
	return true;
};

void GameLogic::moveBlockLeft(PlayField &p, Tetri &block) {
	int t_x = block.get_x();
	int t_y = block.get_y();

	block.set_x(t_x - 1);
	if (!adjustFit(p, block)) {
		block.set_x(t_x);
		block.set_y(t_y);
	}
};

void GameLogic::moveBlockRight(PlayField &p, Tetri &block) {
	int t_x = block.get_x();
	int t_y = block.get_y();

	block.set_x(t_x + 1);
	if (!adjustFit(p, block)) {
		block.set_x(t_x);
		block.set_y(t_y);
	}
};

void GameLogic::moveBlockDown(PlayField &p, Tetri &block) {
	int t_x = block.get_x();
	int t_y = block.get_y();

	block.set_y(t_y + 1);
	if (!adjustFit(p, block)) {
		block.set_x(t_x);
		block.set_y(t_y);
	}
};

void GameLogic::RotateBlockCW(PlayField &p, Tetri &block) {
	int t_x = block.get_x();
	int t_y = block.get_y();

	block.rotateCW();
	if (!adjustFit(p, block)) {
		block.rotateCCW();
		block.set_x(t_x);
		block.set_y(t_y);
	}
};

void GameLogic::RotateBlockCCW(PlayField &p, Tetri &block) {
	int t_x = block.get_x();
	int t_y = block.get_y();

	block.rotateCCW();
	if (!adjustFit(p, block)) {
		block.rotateCW();
		block.set_x(t_x);
		block.set_y(t_y);
	}
};

void GameLogic::GravityBlockDown(PlayField &p, Tetri &block) {
	if (&block == NULL || &p == NULL) {
		return;
	}
	int t_x = block.get_x();
	int t_y = block.get_y();

	if (SDL_GetTicks() - lastDrop >= dropTimeout) {
		block.set_y(t_y + 1);
		lastDrop = SDL_GetTicks();

		if (!adjustFit(p, block)) {
			block.set_y(t_y);
			block.set_x(t_x);
			if (block.getLockTimer() == 0) {
				block.setLockTimer(SDL_GetTicks());
			}
			else 
			{
				if (SDL_GetTicks() - block.getLockTimer() >= lockTimeOut) {
					block.setLocked(true);
				}
			}
		}
		else 
		{
			block.setLockTimer(0);
		}
	}
}

Uint64 GameLogic::getScore(void) {
	return score;
}

Uint64 GameLogic::getLevel(void) {
	return level;
}

Uint64 GameLogic::getLines(void) {
	return lines_completed;
}

GameLogic::~GameLogic(void) {
	if (nnextPiece != NULL) {
		delete nnextPiece;
	}
}

void GameLogic::reset(void) {
	// NOTE: This is poo poo duplication of the constructor code.
	// And: if you're resetting the game, can you not just
	// construct a brand new GameLogic object?

	lastDrop = 0;
	score = 0;
	lines_completed = 0;
	lines_to_level = 10;
	level = 0;
	lockTimeOut = 800;
	dropTimeout = 440;

        // vvv NOTE: delete already checks for a NULL pointer and does
        // nothing if it's NULL.
	if (nnextPiece != NULL) {
		delete nnextPiece;
	}
	nnextPiece = NULL;

        // vvv NOTE: Are we just memory-leaking nextPiece here?
        // (Questions like this would be less prevalent with a
        // std::unique_ptr, of course.)
	nextPiece = drawPiece();
}
playfield.h:
code:
#ifndef __PLAYFIELD__
#define __PLAYFIELD__

// NOTE: No need for the iostream header here: it's only used in the
// .cpp file.
#include <iostream>
#include <vector>

class PlayField {
private:
	// vvv NOTE: No reason this can't just be a std::vector<std::vector<int>>.
	// vvv NOTE: Or an int[22][10] for that matter, as long as the
	// size is unconfigurable...
	std::vector<int> board;

public:

	PlayField();
	bool setBoardAt(int x, int y, int val);
	// NOTE: I marked the method getBoardAt const as an example.
	int getBoardAt(int x, int y) const;
	bool checkInBounds(int x, int y);
	bool boardEmptyAt(int x, int y);
	void drawBoard(void);
	void clearBoard(void);
};

#endif
playfield.cpp:
code:
#include "PlayField.h"

// NOTE: So much duplication of playing field constants (like 22 and
// 10 and 220) here and elsewhere.

bool PlayField::checkInBounds(int x, int y) {
	if ((x >= 0) && (x < 10) && (y >= 0) && (y < 22)) {
		return true;
	}
	return false;
};

bool PlayField::boardEmptyAt(int x, int y) {
	if (board[x + y * 10] == 0) {
		return true;
	}
	return false;
};

PlayField::PlayField() {
	// NOTE: board = std::vector<int>(220);
	//    or board = std::vector<int>(220, 0); (same thing)
	// Actually, use a member initializer list for the constructor of course:
	// PlayField::PlayField() : board(220, 0) { }


	board.reserve(220);
	for (int i = 0; i < 220; i++) {
		board.push_back(0);
	}
};

bool PlayField::setBoardAt(int x, int y, int val) {
	int index = x + y * 10;
	// NOTE: What's wrong with index being 0?
	if (index < 220 && index > 0) {
		board[index] = val;
		return true;
	}
	return false;
};

// NOTE: This method could be const.  I marked it const as an example.
int PlayField::getBoardAt(int x, int y) const {
	int index = x + y * 10;
	// NOTE: What's wrong with index being 0?
	if (index < 220 && index > 0) {
		return board[index];
	}
	return 0;
}

void PlayField::drawBoard(void) {
	for (int y = 0; y < 22; y++) {
		for (int x = 0; x < 10; x++) {
			std::cout << board[x + y * 10] << " ";
		}
		std::cout << std::endl;
	}
};

void PlayField::clearBoard(void) {
        // NOTE: std::fill(board.begin(), board.end(), 0);
	for (int y = 0; y < 22; y++) {
		for (int x = 0; x < 10; x++) {
			board[x + y * 10] = 0;
		}
	}
}
This is as far as I got. Though I noticed more memory management shenanigans elsewhere.

Gasbraai
Oct 25, 2010

Lictor my Dictor
This is amazing and a huge amount to digest already, thank you so much. I have some trouble with the idea of 'const' as in, when it applies and to what it applies. Do you have any recommendations for reading material regarding C++ 'const' rules specifically?

With regards to the idea of allocating an object on the heap and returning a pointer to it from a method in a different object, is there like a standard best-practice way to do that? I would like to read up on the std::pointer types you talk about.

Gasbraai fucked around with this message at 14:00 on Apr 24, 2014

Hammerite
Mar 9, 2007

And you don't remember what I said here, either, but it was pompous and stupid.
Jade Ear Joe

Lunixnerd posted:

This is amazing and a huge amount to digest already, thank you so much. I have some trouble with the idea of 'const' as in, when it applies and to what it applies. Do you have any recommendations for reading material regarding C++ 'const' rules specifically?

"const" can appear in a few different contexts but it does approximately the same thing in each of them, namely, indicates to the compiler that some specific thing is not allowed to be changed. Then, if your code goes ahead and tries to change it after all, it won't compile.

If it appears in a parameter list for a function then it means (approximately, in simple cases) that you're declaring that you won't change that parameter within the function. So here:

int do_thing (int const arg)

you are saying you will not change arg in the body of the function. This is not very interesting information in this case, because arg within the function is a local copy; but if arg were passed by reference, like

int do_thing (int const & arg)

then someone using your code can be confident that they can pass a local variable in to the function and it won't get changed, even though it is passed by reference.

In cases where you are passing a pointer (or a pointer to a pointer, or a pointer to a pointer to a pointer, or...) you can apply const both to the base type and to any * that appears in the argument type declaration. When it is applied to the base type it means you can't change the thing that is pointed-to. When it is applied to a * it means that that pointer can't be changed, i.e. you might or might not be able to change the object pointed-to but you can't change the pointer to point to another object. So

code:
int do_thing (int * arg)
    // You can change the integer pointed-to, and you can change which integer is pointed-to.
int do_thing (int const * arg)
    // You can change which integer is pointed-to, but you can't use the pointer to change
    // the integer that it points to at any given moment.
int do_thing (int * const arg)
    // You can change the integer pointed-to, but you can't change which integer is pointed-to.
int do_thing (int const * const arg)
    // You can't change the integer pointed-to, and you can't change which integer is pointed-to.
If it appears after a declaration of a method in a class then it means that the method doesn't change the state of the object. So a get_* method might be const because it does not change the object in any way, just reports on some aspect of its state:

int get_age () const

You cannot call a non-const method from within a const method, because after all that would present a pretty silly loophole.

The return value of a function can also be declared const. Someone else will have to explain that one. There is every chance that I have missed something else out, as well.

BTW in case you are wondering why I have written "int const" and elsewhere you see "const int": they are the same thing, "const" (used in specifying a type) always applies to whatever is to its immediate left, unless there is nothing to its left in which case it applies to whatever is to its immediate right.

You might find this Stack Overflow question useful.

Hammerite fucked around with this message at 18:01 on Apr 24, 2014

MrMoo
Sep 14, 2000

There are standard fixed width int datatypes now, please try to avoid everything else:
code:
int8_t
int16_t
int32_t
int64_t	...
Even Microsoft support them since VS2010SP1, you can find compatibility headers for older compilers.

High Protein
Jul 12, 2009
-You can use #pragma once instead of include guards.

-Definitely do something other than this-> to access your member variables; member_ is ok, though I prefer m_member.

-Try using assert() to check for pre/postconditions that should hold if your code is correct, such as if a pointer is set (although in such cases a reference can usually be used), the result of some calculation, dependencies between variables, etc. Also, you can use assert(0) for impossible default: cases in switch statements.

-I see that you often check preconditions and then immediately return if they're not met, which is good and avoids deeply-nested if statements.

-Use nullptr for null pointers instead of NULL. However as has been said, avoid using raw pointers unless it's just to refer to some already existing object you don't own and don't need to keep alive. Unique_ptr is automatically set to nullptr for you. Unique_ptr makes it clear who owns a piece of memory and who is only accessing/referencing it. Again, for that kind of stuff just use references if possible. You can use the get() function to pass a unique_ptr into a function that expects a raw pointer, for instance to fill some buffer.

-About the const talk, although it might seem like overkill, make a habit of even making value function parameters, such as integers, const. On larger codebases, and or code that has multiple people working on it, it prevents stupid bugs due to someone modifying a parameter somewhere in a function, and then someone else coming along that assumes the parameter still contains its original value. Const temporary values are also handy for breaking calculations up into self-documenting steps.

-Although this is personal taste, I prefer listing public functions and members before anything protected or private, that way it's easy to see the public interface of a class. It's also cleaner when a private function requires some data type that's public.

I also like to group overridden virtual methods (marked with the override keyword) under a comment that states whose functions they override. Personally I like to put those in a separate section under my own class' public/protected/private stuff but some of my coworkers prefer to group them under those, it kind of depends what ends up being cleaner.

-I don't necessarily agree that only fixed-width types should be used; try to match whatever types you use the functions you're interacting with. For stl stuff this often means size_t, but for instance a lot of Windows functions use plain ints.

Adbot
ADBOT LOVES YOU

Marta Velasquez
Mar 9, 2013

Good thing I was feeling suicidal this morning...
Fallen Rib
If you want to dive into a lot of reading, there is a great C++ FAQ that answers a lot of questions you'll have when first starting out.

For example, there's an entire section on const.

One note, though: this doesn't include C++11, so things like unique_ptr aren't there.

  • Locked thread