Match Three game for a job interview using Cocos2d-x

The problem is that I had an interview for a gaming company and I had to write a prototype of a Match Three style game. The time line was 3 days. I made the code in about 2 days and sent it to them.

Then I had a rejection and a comment that my code has "many problems with performance, basic programmers mistakes and possible troubles with this type of level of coding". They didn't gave me more details, so I was wondering can you focus me on which parts I need to concentrate fixing.

The complete project can be found here.

Global.h

//
//  Global.h
//  MatchThree
//
//  Created by Dimitar Dimitrov on 5/2/16.
//
//

#ifndef constants_h
#define constants_h

#include 

#define EVENT_GRID_READY "event.custom.grid_is_ready"
#define EVENT_GRID_HAS_MATCHES "event.custom.grid_has_matches"

const int MAX_MATCHES = 3; // Number of matches to consider a succesfull match

// Size of the grid
const int MAX_ROWS = 7;
const int MAX_COLS = 7;

const int SWIPE_TRESHOLD = 5; // Minimum pixels for swipe move

const cocos2d::Size _BlockSize = cocos2d::Size(80, 80);

enum class BlockType : int
{
    NONE = -1,
    RED = 0,
    BLUE,
    GREEN,
    YELLOW,
    PURPLE
};

const std::map BlockTypeToFrameName
{
    {BlockType::RED, "red.png"},
    {BlockType::BLUE, "blue.png"},
    {BlockType::GREEN, "green.png"},
    {BlockType::YELLOW, "yellow.png"},
    {BlockType::PURPLE, "purple.png"}
};

typedef struct {
    int row;
    int col;
} GridPosition;

typedef struct {
    GridPosition first;
    GridPosition second;
} GridMove;

typedef struct {
    int matches;
} EventMatchesData;

#endif /* constants_h */

Block.h

```
//
// Block.h
// MatchThree
//
// Created by Dimitar Dimitrov on 5/2/16.
//
//

#ifndef Block_hpp
#define Block_hpp

#include
#include "Global.h"

class Block : public cocos2d::Node
{
public:
static Block* createBlock(BlockType type, GridPosition gridPosition);
void setActive(bool isActive);
void blink(int times = 3);
GridPosition gridPosition;

First of all, it seems you're new to CR so let me welcome you and congratulate on a good first question!

Unfortunately I too would have rejected this code sample for a job position I was hiring for. I will try to offer some insight into why.

First of remember that you're writing code that is going to be scrutinized like no tomorrow, you can't afford slip-ups.

Attention to detail

Programming is a profession where lack of attention to detail can cause bugs. I'm looking for any signs that can inform me if you have the required level of attention or not. The file Global.h has an include guard that hints that the file's name is actually constant.h right off the bat I see a sign that you're not paying attention to detail. Same goes for the other files as well.

Not to mention that the names you use for your include guards are kind of generic and prone to conflicts. You should either use #pragma once which is widely available or use a longer name on your #ifndefs.

Polluting the global namespace

You have some very generic sounding constants in the global namespace in a header that seems central for your application:

// Size of the grid
const int MAX_ROWS = 7;
const int MAX_COLS = 7;

This is bad because some one else who also isn't following best practices might have defined these constants somewhere else. It might not be a problem today, but tomorrow when you pull in another 3rd party library you might end up with a name clash.

They should be local to the class in which they are used, if you cannot do that, then that's a sign of a problem with your design.

But your global constants are const declared and that is a good redeeming quality.

Don't use old C constructs in C++ code

This type of code:

typedef struct {
    int row;
    int col;
} GridPosition;

is how you would write it in C. In C++ we prefer this:

struct GridPosition {
    int row;
    int col;
};

Using macros when other means are available.

This piece of code right here:

#define EVENT_GRID_READY "event.custom.grid_is_ready"

shows me that you are prone to using macros. Macros are evil, they have their uses when no other tool exists to do the job, but those are rare.
Really you should just have written:

const char* const EVENT_GRID_READ = "event.custom.grid_is_ready";

or if C++11 is allowed:

const auto EVENT_GRID_READ = "event.custom.grid_is_ready";

Using wrong data structure

This piece of code here:

const std::map BlockTypeToFrameName

creates a tree structure (likely red-black tree) and stores the contents in it. This means that you will be traversing pointers and have \$\mathcal{O}\left(\log\left(n\right)\right)\$ lookup time. While had you used an std::unordered_map which is a hash table you would have had \$\mathcal{O}\left(1\right)\$ lookup or even better as your enum is solid, you could have just used a plain array.

Memory leaks

Although cocos2d has some form of automatic memory management you still have to take care of things until you successfully hand the pointer over to cocos2d.

For example here:

Block* Block::createBlock(BlockType type, GridPosition gridPosition)
{
    Block* block = new Block();

    if (block->init())
    {
        block->autorelease();
        block->type = type;
        block->gridPosition = gridPosition;

        return block;
    }

    return nullptr;
}

if init() returns false, you will leak block, under normal circumstances this shouldn't happen of course but this is for a job interview. It will not have been set for auto release, and it will not have been added as a child to anything that can release it. The system cannot know if you intend to use the pointer further or not.

You could do something like this if you want to be sure you don't leak memory even in the presence of exceptions:

Block* Block::createBlock(BlockType type, GridPosition gridPosition) {
     Block* block = nullptr;

     try{
         block = new Block();
         if(!block->init())
             throw std::bad_alloc(); // Pick a suitable exception
         block->autorelease();
         block->type = type;
         block->gridPosition = gridPosition;     
         return block;
     }catch(...){
         CC_SAFE_DELETE(block); 
         throw;
     }
 }

I don't know if any of those methods can ever throw but this is the safe way to do it in the presence of exceptions.

Use of a static factory

The static factory pattern seems to be a thing in Cocos2d, although it has its uses when you want to avoid having concrete classes in the header I do not particularly like it's use.

For example I would replace Grid::createGrid with a constructor:

Grid::Grid(int rows, int cols) {
    if (!init()){
        throw std::runtime_error(); // Pick better exception
    }        
    autorelease();
    setContentSize(Size(rows * _BlockSize.width, cols * _BlockSize.height));
    setAnchorPoint(Vec2::ANCHOR_MIDDLE);
};

This has the be

StackExchange Code Review Q#129650, answer score: 12