17
\$\begingroup\$

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 <cocos2d.h>

#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<BlockType, std::string> 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 <cocos2d.h>
#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;
    BlockType getType()
    {
        return type;
    }
    void setType(BlockType type);

    void explode();

    virtual void onEnter();

private:
    BlockType type;
    cocos2d::Sprite* backgroundSprite;
    cocos2d::Sprite* blockSprite;
};

#endif /* Block_hpp */

Block.cpp

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

#include "Block.h"

USING_NS_CC;

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;
}

void Block::setType(BlockType type)
{
    this->type = type;

    if (blockSprite != nullptr && type != BlockType::NONE)
        blockSprite->setSpriteFrame(BlockTypeToFrameName.at(type));
}

void Block::onEnter()
{
    Node::onEnter();

    setContentSize(_BlockSize);
    setAnchorPoint(Vec2::ANCHOR_MIDDLE);

    backgroundSprite = Sprite::createWithSpriteFrameName("back.png");
    backgroundSprite->setAnchorPoint(Vec2::ANCHOR_MIDDLE);
    backgroundSprite->setPosition(_BlockSize * 0.5f);
    addChild(backgroundSprite, 0);

    blockSprite = Sprite::createWithSpriteFrameName(BlockTypeToFrameName.at(type));
    blockSprite->setAnchorPoint(Vec2::ANCHOR_MIDDLE);
    blockSprite->setPosition(_BlockSize * 0.5f);
    addChild(blockSprite, 1);
}

void Block::setActive(bool isActive)
{
    if (isActive)
        backgroundSprite->setSpriteFrame("back_active.png");
    else
        backgroundSprite->setSpriteFrame("back.png");
}

void Block::blink(int times)
{
    backgroundSprite->runAction(Repeat::create(Sequence::create(
                                              TintTo::create(0.1f, Color3B::BLACK),
                                              TintTo::create(0.1f, Color3B::WHITE)
                                              , NULL)
                             , times));
}

void Block::explode()
{
    auto explodeParticles = ParticleSystemQuad::create("assets/explode.plist");
    explodeParticles->setAutoRemoveOnFinish(true);
    explodeParticles->setPosition(getPosition());
    getParent()->addChild(explodeParticles, 10);
}

Grid.h

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

#ifndef Grid_hpp
#define Grid_hpp

#include <cocos2d.h>
#include "Global.h"
#include "Block.h"

class Grid: public cocos2d::Layer
{
public:
    Block* blocks[MAX_ROWS][MAX_COLS];

    static Grid* createGrid(int rows, int cols);

    void removeAllBoxes();
    void generateRandomBloxes();
    Block* getBlockForPosition(cocos2d::Vec2 position);
    bool areNeighbours(Block* first, Block*second);
    void swapBoxes(Block* first, Block* second);
    bool checkForMatches(Block* block);
    std::vector<Block*> findMatches(Block* block, bool isRoot = true);
    void removeBlockAt(GridPosition pos);
    void fillBlanks();
    void resolveMatchesForBlocks(std::vector<Block*> blocks);

    std::vector<GridMove> findAvailableMoves();

    virtual void onEnter();
    bool isBisy() { return bisy; };

private:
    std::vector<Block*> matches;
    bool bisy;
};

#endif /* Grid_hpp */

Grid.cpp

//
//  Grid.cpp
//  MatchThree
//
//  Created by Dimitar Dimitrov on 5/2/16.
//
//

#include "Grid.h"
#include "SimpleAudioEngine.h"

USING_NS_CC;

Grid* Grid::createGrid(int rows, int cols)
{
    Grid* grid = new Grid();

    if (grid->init())
    {
        grid->autorelease();

        grid->setContentSize(Size(rows * _BlockSize.width, cols * _BlockSize.height));
        grid->setAnchorPoint(Vec2::ANCHOR_MIDDLE);

        return grid;
    }

    return nullptr;
};

void Grid::onEnter()
{
    Layer::onEnter();
}

void Grid::generateRandomBloxes()
{
    for (int col = 0; col < MAX_COLS; col++)
    {
        for (int row = 0; row < MAX_ROWS; row++)
        {
            Block* block = Block::createBlock((BlockType)random((int)BlockType::RED, (int)BlockType::PURPLE), {row, col});

            // Make sure there are no initial groups of matches
            while(checkForMatches(block) == true)
                block->setType((BlockType)random((int)BlockType::RED, (int)BlockType::PURPLE));

            block->setPosition(row * _BlockSize.width + _BlockSize.width * 0.5f,
                               col * _BlockSize.height + _BlockSize.height * 0.5f);

            addChild(block, 1);
            blocks[row][col] = block;
        }
    }
}

void Grid::removeAllBoxes()
{
    stopAllActions();
    bisy = false;

    for (int col = 0; col < MAX_COLS; col++)
        for (int row = 0; row < MAX_ROWS;row++)
        {
            if (blocks[row][col] != nullptr)
            {
                removeChild(blocks[row][col]);
                blocks[row][col] = nullptr;
            }
        }
}

Block* Grid::getBlockForPosition(cocos2d::Vec2 position)
{
    unsigned int row = (unsigned int) ceil((position.x - getPosition().x) / _BlockSize.width) - 1;
    unsigned int col = (unsigned int) ceil((position.y - getPosition().y) / _BlockSize.height) - 1;

    CCASSERT((col < MAX_COLS && row < MAX_ROWS), "Incorrect row/col. Index out of range");

    if (blocks[row][col])
        return blocks[row][col];

    return nullptr;
}

void Grid::swapBoxes(Block* first, Block* second)
{
    if (!first || !second)
        return;

    if (!areNeighbours(first, second))
    {
        CCLOGERROR("Blocks [%d, %d] and [%d, %d] are not neighbours!",
                   first->gridPosition.row,
                   first->gridPosition.col,
                   second->gridPosition.row,
                   second->gridPosition.col
                   );
        return;
    }

    blocks[first->gridPosition.row][first->gridPosition.col] = second;
    blocks[second->gridPosition.row][second->gridPosition.col] = first;

    // Swap bloxes indexes
    GridPosition temp = first->gridPosition;
    first->gridPosition = second->gridPosition;
    second->gridPosition = temp;    
}

void Grid::removeBlockAt(GridPosition pos)
{
    Block* block = blocks[pos.row][pos.col];
    if (block)
    {
        blocks[pos.row][pos.col] = nullptr;
        block->explode();
        block->setType(BlockType::NONE);
        removeChild(block, true);
    }
}

bool Grid::areNeighbours(Block* first, Block* second)
{
    if (!first || !second)
        return false;

    int x = abs(first->gridPosition.row - second->gridPosition.row);
    int y = abs(first->gridPosition.col - second->gridPosition.col);

    return (x == 1 && y == 0) || (x == 0 && y == 1);
}

bool Grid::checkForMatches(Block* block)
{
    if (!block)
        return false;

    GridPosition gp = block->gridPosition;

    // Check horizontal
    int matches = 1;
    int row = gp.row;
    while(--row > -1 && blocks[row][gp.col] && blocks[row][gp.col]->getType() == block->getType()) // Check left
        matches++;

    row = gp.row;
    while(++row < MAX_ROWS && blocks[row][gp.col] && blocks[row][gp.col]->getType() == block->getType()) // Check right
        matches++;

    if (matches >= MAX_MATCHES)
        return true;

    // Check vertical
    matches = 1;
    int col = gp.col;
    while(--col > -1 && blocks[gp.row][col] && blocks[gp.row][col]->getType() == block->getType()) // Check down
        matches++;

    col = gp.col;
    while(++col < MAX_COLS && blocks[gp.row][col] && blocks[gp.row][col]->getType() == block->getType()) // Check up
        matches++;

    return (matches >= MAX_MATCHES);
}


std::vector<Block*> Grid::findMatches(Block* block, bool isRoot)
{
    if (isRoot)
        matches.clear();

    matches.push_back(block);

    GridPosition pos = block->gridPosition;

    Block* leftBlock = (pos.row > 0)?blocks[pos.row - 1][pos.col]:nullptr;
    Block* rightBlock = (pos.row < MAX_ROWS - 1)?blocks[pos.row + 1][pos.col]:nullptr;
    Block* topBlock = (pos.col < MAX_COLS - 1)?blocks[pos.row][pos.col + 1]:nullptr;
    Block* bottomBlock = (pos.col > 0)?blocks[pos.row][pos.col - 1]:nullptr;

    // Left
    if (leftBlock && leftBlock->getType() == block->getType())
    {
        if (std::find(matches.begin(), matches.end(), leftBlock) == matches.end())
            findMatches(leftBlock, false);
    }

    // Right
    if (rightBlock && rightBlock->getType() == block->getType())
    {
        if (std::find(matches.begin(), matches.end(), rightBlock) == matches.end())
            findMatches(rightBlock, false);
    }

    // Top
    if (topBlock && topBlock->getType() == block->getType())
    {
        if (std::find(matches.begin(), matches.end(), topBlock) == matches.end())
            findMatches(topBlock, false);
    }

    // Bottom
    if (bottomBlock && bottomBlock->getType() == block->getType())
    {
        if (std::find(matches.begin(), matches.end(), bottomBlock) == matches.end())
            findMatches(bottomBlock, false);
    }

    return matches;
}


std::vector<GridMove> Grid::findAvailableMoves() {
    std::vector<GridMove> availableMoves;
    Block* otherBlock;

    for (int row = 0; row < MAX_ROWS; row++)
    {
        for (int col = 0; col < MAX_COLS; col++)
        {
            Block *block = blocks[row][col];
            if (block == nullptr)
                continue;

            // Swap right and check if successfull, than swap them back
            if (row < MAX_ROWS - 1 && blocks[row + 1][col])
            {
                otherBlock = blocks[row + 1][col];
                swapBoxes(block, otherBlock);
                if (checkForMatches(block) || checkForMatches(otherBlock))
                    availableMoves.push_back({otherBlock->gridPosition, block->gridPosition});
                swapBoxes(otherBlock, block);
            }

            // Swap up and check
            if (col < MAX_COLS - 1 && blocks[row][col + 1])
            {
                otherBlock = blocks[row][col + 1];
                swapBoxes(block, otherBlock);
                if (checkForMatches(block) || checkForMatches(otherBlock))
                    availableMoves.push_back({otherBlock->gridPosition, block->gridPosition});
                swapBoxes(otherBlock, block);
            }
        }
    }

    return availableMoves;
}


void Grid::fillBlanks()
{
    bisy = true;
    std::vector<Block*> addedBlocks;
    for (int row = 0; row < MAX_ROWS; row++)
    {
        int blanks = 0;

        // Find blank spots for the column
        for (int col = 0; col < MAX_COLS; col++)
        {
            if (blocks[row][col] == nullptr)
                blanks++;
            else if (blanks > 0) // Move blocks above the blank down
            {
                int new_col = col - blanks;
                blocks[row][col]->runAction(EaseBounceOut::create(
                                                                  MoveTo::create(0.7f,
                                                                                 Vec2(blocks[row][col]->getPositionX(),
                                                                                      new_col * _BlockSize.height + _BlockSize.height * 0.5f)
                                                                                 )));
                blocks[row][new_col] = blocks[row][col];
                blocks[row][col] = nullptr;
                blocks[row][new_col]->gridPosition = {row, new_col};

                addedBlocks.push_back(blocks[row][new_col]);
            }
        }

        // Add new blocks to fill the missing ones
        while(blanks--) {
            int new_col = MAX_COLS - blanks - 1;
            Block* block = Block::createBlock((BlockType)random((int)BlockType::RED, (int)BlockType::PURPLE), {row, new_col});

            Vec2 newPosition = Vec2(row * _BlockSize.width + _BlockSize.width * 0.5f,
                                    new_col * _BlockSize.height + _BlockSize.height * 0.5f);

            block->setPositionX(newPosition.x);
            block->setPositionY(getContentSize().height + (MAX_COLS - blanks) * _BlockSize.height * 0.5f);
            block->gridPosition = {row, new_col};
            block->runAction(EaseBounceOut::create(MoveTo::create(0.7f, newPosition)));

            addChild(block, 1);
            blocks[row][new_col] = block;
            addedBlocks.push_back(block);
        }

    }

    runAction(Sequence::create(
                               DelayTime::create(0.75f),
                               CallFunc::create(CC_CALLBACK_0(Grid::resolveMatchesForBlocks, this, addedBlocks)),
                               NULL));
}

void Grid::resolveMatchesForBlocks(std::vector<Block*> blocks)
{
    bool hasMatches = false;
    std::vector<Block*> blocksToRemove;

    for (int i = 0; i < blocks.size(); i++)
    {
        Block* block = blocks.at(i);

        if (block && block->getType() != BlockType::NONE && checkForMatches(block))
        {
            hasMatches = true;
            for (auto match : findMatches(block))
            {
                if (std::find(blocksToRemove.begin(), blocksToRemove.end(), match) == blocksToRemove.end())
                    blocksToRemove.push_back(match);
            }
        }
    }


    if (hasMatches) {
        EventCustom hasMatchesEvent(EVENT_GRID_HAS_MATCHES);
        EventMatchesData em;
        em.matches = (int) blocksToRemove.size();
        hasMatchesEvent.setUserData((void *) &em);
        _eventDispatcher->dispatchEvent(&hasMatchesEvent);

        CocosDenshion::SimpleAudioEngine::getInstance()->playEffect("assets/pop.wav");
        for (auto block : blocksToRemove)
            removeBlockAt(block->gridPosition);

        fillBlanks();
    }
    else
    {
        EventCustom gridReadyEvent(EVENT_GRID_READY);
        _eventDispatcher->dispatchEvent(&gridReadyEvent);

        bisy = false;
    }
}

GameScene.h

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

#ifndef GameScene_hpp
#define GameScene_hpp

#include <cocos2d.h>
#include "Grid.h"

class GameScene : public cocos2d::Layer
{
public:
    static cocos2d::Scene* createScene();

    virtual bool init();
    virtual void onEnter();

private:
    Block* activeBlock;
    Grid* grid;
    bool isBisy;
    cocos2d::Label* scoreLabel;
    std::vector<GridMove> availableMoves;
    int currentScore;

    void swapBoxes(Block* first, Block* second);
    void newGame(cocos2d::Ref* ref);
    void showHint(cocos2d::Ref* ref);
    void checkForMatches(Block* first, Block* second);
    void lockTouches(float time);
    void unlockTouches(float dt);
    void addScore(int score);
    void resolveMatchesForBlock(Block* block);

    bool onTouchBegan(cocos2d::Touch* touch, cocos2d::Event* event);
    void onTouchMoved(cocos2d::Touch* touch, cocos2d::Event* event);
    void onGridReady(cocos2d::EventCustom* event);
    void onGridMatches(cocos2d::EventCustom* event);
};

#endif /* GameScene_hpp */

GameScene.cpp

//
//  GameScene.cpp
//  MatchThree
//
//  Created by Dimitar Dimitrov on 5/2/16.
//
//

#include "GameScene.h"
#include "SimpleAudioEngine.h"

USING_NS_CC;

Scene* GameScene::createScene()
{
    auto scene = Scene::create();
    auto gameScene = new GameScene();

    gameScene->autorelease();

    if (gameScene->init())
        scene->addChild(gameScene);
    else
        return nullptr;

    return scene;
}


bool GameScene::init()
{
    if (!Layer::init())
        return false;

    isBisy = false;
    srand(utils::gettime());

    // Preload assets
    SpriteFrameCache::getInstance()->addSpriteFramesWithFile("assets/blocks.plist");
    CocosDenshion::SimpleAudioEngine::getInstance()->preloadEffect("assets/pop.wav");
    Director::getInstance()->getTextureCache()->addImage("assets/bg.jpg");

    // Register event listeners
    auto touchListener = EventListenerTouchOneByOne::create();
    touchListener->onTouchBegan = CC_CALLBACK_2(Grid::onTouchBegan, this);
    touchListener->onTouchMoved = CC_CALLBACK_2(Grid::onTouchMoved, this);
    _eventDispatcher->addEventListenerWithSceneGraphPriority(touchListener, this);

    auto gridListener = EventListenerCustom::create(EVENT_GRID_READY, CC_CALLBACK_1(GameScene::onGridReady, this));
    _eventDispatcher->addEventListenerWithSceneGraphPriority(gridListener, this);

    auto matchesListener = EventListenerCustom::create(EVENT_GRID_HAS_MATCHES, CC_CALLBACK_1(GameScene::onGridMatches, this));
    _eventDispatcher->addEventListenerWithSceneGraphPriority(matchesListener, this);

    return true;
}

void GameScene::onEnter()
{
    Layer::onEnter();

    Size screenSize = Director::getInstance()->getVisibleSize();

    auto background = Sprite::create("assets/bg.jpg");
    background->setAnchorPoint(Vec2::ANCHOR_MIDDLE);
    background->setPosition(screenSize * 0.5f);
    background->setScaleX(screenSize.width / background->getContentSize().width);
    background->setScaleY(screenSize.height / background->getContentSize().height);
    addChild(background, 0);

    grid = Grid::createGrid(MAX_ROWS, MAX_COLS);
    grid->setPosition(screenSize.width * 0.5f - grid->getContentSize().width * 0.5f,
                      screenSize.height * 0.5f - grid->getContentSize().height * 0.5f - 30);
    addChild(grid, 1);

    // HUD
    auto hintButton = MenuItemSprite::create(Sprite::createWithSpriteFrameName("button_pressed.png"),
                                             Sprite::createWithSpriteFrameName("button.png"));
    hintButton->setAnchorPoint(Vec2::ANCHOR_TOP_LEFT);
    hintButton->setPosition(20, screenSize.height - 20);
    auto hintLabel = Label::createWithSystemFont("HINT", "Verdana", 32);
    hintLabel->setPosition(hintButton->getContentSize().width * 0.5f, hintButton->getContentSize().height * 0.5f + 5);
    hintLabel->enableShadow();
    hintButton->addChild(hintLabel);
    hintButton->setCallback(CC_CALLBACK_1(GameScene::showHint, this));


    auto newGameButton = MenuItemSprite::create(Sprite::createWithSpriteFrameName("button_pressed.png"),
                                                Sprite::createWithSpriteFrameName("button.png"));
    newGameButton->setAnchorPoint(Vec2::ANCHOR_TOP_LEFT);
    newGameButton->setPosition(40 + hintButton->getContentSize().width, screenSize.height - 20);
    auto newGameLabel = Label::createWithSystemFont("NEW GAME", "Verdana", 32);
    newGameLabel->enableShadow();
    newGameLabel->setPosition(newGameButton->getContentSize().width * 0.5f, newGameButton->getContentSize().height * 0.5f + 5);
    newGameButton->addChild(newGameLabel);
    newGameButton->setCallback(CC_CALLBACK_1(GameScene::newGame, this));

    auto menu = Menu::create(hintButton, newGameButton, nullptr);
    menu->setPosition(Vec2::ZERO);
    addChild(menu, 2);

    scoreLabel = Label::createWithSystemFont("", "Verdana", 32);
    scoreLabel->setColor(Color3B::WHITE);
    scoreLabel->enableShadow();
    scoreLabel->setAnchorPoint(Vec2::ANCHOR_TOP_RIGHT);
    scoreLabel->setPosition(screenSize.width - 20, screenSize.height - 20);
    addChild(scoreLabel, 2);

    newGame(this);
}

#pragma mark Touches
bool GameScene::onTouchBegan(Touch* touch, Event* event)
{
    Vec2 point = touch->getLocation();

    if (isBisy || grid->isBisy() || grid->getBoundingBox().containsPoint(point) == false)
        return false;

    auto block = grid->getBlockForPosition(point);
    if (block == nullptr || block == activeBlock)
        return true;

    if (activeBlock != nullptr)
    {
        // If boxes are neighbours try to swap them
        if (grid->areNeighbours(activeBlock, block))
        {
            swapBoxes(block, activeBlock);
            runAction(Sequence::create(
                                       DelayTime::create(0.3f), // Wait for the animation to end
                                       CallFunc::create(CC_CALLBACK_0(GameScene::checkForMatches, this, activeBlock, block)),
                      nullptr));

            activeBlock->setActive(false);
            block->setActive(false);
            activeBlock = nullptr;
        }
        else // If not set active the selected one
        {
            activeBlock->setActive(false);
            activeBlock = block;
            activeBlock->setActive(true);
        }
    }
    else
    {
        activeBlock = block;
        activeBlock->setActive(true);
    }

        return true;
}

// Detect swipes
void GameScene::onTouchMoved(cocos2d::Touch* touch, cocos2d::Event* event)
{
    if (isBisy || grid->isBisy() || activeBlock == nullptr)
        return;

    Vec2 touchDistance = touch->getLocation() - touch->getPreviousLocation();
    Block* blockToSwap = nullptr;

    // Right
    if (touchDistance.x >= SWIPE_TRESHOLD && (activeBlock->gridPosition.row + 1 < MAX_ROWS))
        blockToSwap = grid->blocks[activeBlock->gridPosition.row + 1][activeBlock->gridPosition.col];

    // Left
    if (touchDistance.x <= -SWIPE_TRESHOLD && (activeBlock->gridPosition.row > 0))
        blockToSwap = grid->blocks[activeBlock->gridPosition.row - 1][activeBlock->gridPosition.col];

    // Up
    if (touchDistance.y >= SWIPE_TRESHOLD && (activeBlock->gridPosition.col + 1 < MAX_COLS))
        blockToSwap = grid->blocks[activeBlock->gridPosition.row][activeBlock->gridPosition.col + 1];

    // Down
    if (touchDistance.y <= -SWIPE_TRESHOLD && (activeBlock->gridPosition.col > 0))
        blockToSwap = grid->blocks[activeBlock->gridPosition.row][activeBlock->gridPosition.col - 1];

    if (blockToSwap != nullptr)
    {
        swapBoxes(blockToSwap, activeBlock);
        runAction(Sequence::create(
                                   DelayTime::create(0.3f), // Wait for the animation to end
                                   CallFunc::create(CC_CALLBACK_0(GameScene::checkForMatches, this, activeBlock, blockToSwap)),
                                   nullptr));

        activeBlock->setActive(false);
        activeBlock = nullptr;
    }

}

void GameScene::checkForMatches(Block* first, Block* second)
{
    bool firstMatches = grid->checkForMatches(first);
    bool secondMatches = grid->checkForMatches(second);

    if (firstMatches == false && secondMatches == false)
    {
        // No matches swap them back and return
        swapBoxes(first, second);
        return;
    }

    if (firstMatches)
    {
        resolveMatchesForBlock(first);
    }

    if (secondMatches)
    {
        resolveMatchesForBlock(second);
    }

    if (firstMatches || secondMatches)
    {
        lockTouches(0.7f);
        grid->fillBlanks();

        activeBlock = nullptr;
    }
}

void GameScene::swapBoxes(Block* first, Block* second)
{
    lockTouches(0.3f);
    grid->swapBoxes(first, second);
    first->runAction(MoveTo::create(0.3f, second->getPosition()));
    second->runAction(MoveTo::create(0.3f, first->getPosition()));
}

void GameScene::resolveMatchesForBlock(Block* block)
{
    if (block == nullptr)
        return;

    auto matches = grid->findMatches(block);
    addScore((int)matches.size());

    CocosDenshion::SimpleAudioEngine::getInstance()->playEffect("assets/pop.wav");

    for (auto match : matches)
        grid->removeBlockAt(match->gridPosition);

}

void GameScene::newGame(cocos2d::Ref* ref)
{
    grid->removeAllBoxes();
    grid->generateRandomBloxes();
    availableMoves = grid->findAvailableMoves();

    activeBlock = nullptr;
    addScore(-currentScore);

    if (availableMoves.size() == 0)
        newGame(this);

}

// Select a random move from available ones and show it
void GameScene::showHint(cocos2d::Ref *ref)
{   
    if (!grid->isBisy() && availableMoves.size() > 0)
    {
        GridMove move = availableMoves.at(random(0, (int)availableMoves.size() - 1));
        grid->blocks[move.first.row][move.first.col]->blink();
        grid->blocks[move.second.row][move.second.col]->blink();
    }
}

void GameScene::lockTouches(float time)
{
    unschedule(CC_SCHEDULE_SELECTOR(GameScene::unlockTouches));
    scheduleOnce(CC_SCHEDULE_SELECTOR(GameScene::unlockTouches), 0.5f);
    isBisy = true;
}

void GameScene::unlockTouches(float dt)
{
    isBisy = false;
}

void GameScene::addScore(int score)
{
    currentScore += score;
    char score_string[512];
    snprintf(score_string, 512, "Score: %d", currentScore);
    scoreLabel->setString(score_string);
}


void GameScene::onGridReady(cocos2d::EventCustom *event)
{
    availableMoves = grid->findAvailableMoves();
    if (availableMoves.size() == 0)
        newGame(this);
}

void GameScene::onGridMatches(cocos2d::EventCustom* event)
{
    EventMatchesData *em = (EventMatchesData *) event->getUserData();
    addScore(em->matches);
}
\$\endgroup\$
12
\$\begingroup\$

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<BlockType, std::string> 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 benefit that it is now impossible to create an uninitialised Grid by mistake. Also notice that the code is shorter. There is no null pointers involved so you do not need to check for nulls (you get an exception but you can just let that fly up the call chain and catch it at a suitable location and display the error to the user).

Long time ago it used to be that constructors wouldn't handle exceptions thrown from them properly but this is a thing of the past, all semi modern compilers can handle exceptions in constructors just fine. Just be aware that the destructor will not be ran if you throw from a constructor and that you need to clean up any resources you might have allocated before the exception. And I think this may be the cause of using static "create" functions in cocos2d. Also it is okay to do work in the constructor.

Failure to implement the rule of three

In your classes you are failing to provide default constructor that initializes the node to a known state. Although I'm sure your code works for now but in larger projects this is not so easy to see. It is good practice to always initialize everything. Even if you don't provide an explicit constructor you can still initialize members at the declaration. For example in Block.h:

private:
    BlockType type;
    cocos2d::Sprite* backgroundSprite;
    cocos2d::Sprite* blockSprite;
};

should be:

 private:
     BlockType type = BlockType::NONE;
     cocos2d::Sprite* backgroundSprite = nullptr;
     cocos2d::Sprite* blockSprite = nullptr;
 };

This is possible since C++11.

Now because you are using Cocos2d that is doing memory management behind your back you might get away with the compiler generated copy constructor and assignment operator. But even so because this is an interview problem you really should make it explicit with a comment like so:

 // The default copy ctr works because memory is managed by Cocos2d.
 Block(const Block&) = default; 
 // Default assignment operator works because memory is managed by Cocos2d.
 Block& operator = (const Block&) = default;

or simply disable them if you are not sure that it will work (I'm not 100% sure myself tbh, I would err on the side of caution and disable if I don't need them):

 // Disallow copy construction and assignment.
 // It is not safe to use with Cocos2d.
 Block(const Block&) = delete;
 Block& operator = (const Block&) = delete;

You should also make the destructor explicit with a comment to show that you have thought about the memory management and concluded that it is safe. This is reassuring for the people deciding if they want to hire you:

 // Resources are freed automatically by Cocos2d GC system.
 virtual ~Block() = default;

The rule of three, that you always have to implement the destructor, copy constructor and copy assignment if you are dealing with dynamic memory is such a big thing in C++. You really want to show that you know this to the interviewers and it is not obvious from the code that you do.

\$\endgroup\$
  • \$\begingroup\$ Thank you, very much for this detailed review! Looks like I need to go and put some more effort on learning the new standards (and the old ones). For my next interview I should pay more attention into details. \$\endgroup\$ – Димитър Димитров May 30 '16 at 20:18
  • \$\begingroup\$ I see, but. Without any meaning of arguing. Cocos2d-x provides a memory management by reference counting, so any objects marked as autorelease will be managed by the framework. In my case all my objects are marked as autorelease, so they should be propperly managed. So in this case I intentionally do not delete manually and objects. \$\endgroup\$ – Димитър Димитров May 30 '16 at 20:40
  • \$\begingroup\$ I looked into cocos2d and you are correct, it handles memory internally as you say. I do not like the way they do it because it is error prone. The point about intialization and rule of three still stands. I will edit my post. \$\endgroup\$ – Emily L. May 31 '16 at 6:16
  • \$\begingroup\$ I thank you very much for the time you took and all the details you provided. Some of the things you mentioned like the static pattern is a cocos2d-x thing which I tried to follow in the context that the company I was applying for is using it. And i really need to concentrate fixing my old C style habits. Your answer is more than accepted :-) Thanks again! \$\endgroup\$ – Димитър Димитров May 31 '16 at 7:18
  • \$\begingroup\$ Just to let you know. Based on your answer I think I've managed to clean things a bit and made a new branch. Some of them Ieft as they are, just to be more consistent with the framework. Also if anyone is interested In how the game actually looks I've made a demo video here. \$\endgroup\$ – Димитър Димитров May 31 '16 at 10:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.