5
\$\begingroup\$

I have a mobile game, utilizing cocos2d-x, that has a handler for touch events.

What it does:

  1. Main function that gets the touch.
  2. Loop in module A. All sprites check if the touch coordinates are in the boundary of one of the sprite. If yes, call the function that associated with the action and break the loop.
  3. The same as 2 with module B.
  4. Same as 2,3, with module C and so on.
  5. In some some parts of the loop there are Boolean indicators that say:

    1. If boolInd1 is true, ignore touches from Module A.
    2. If boolInd2 is true, ignore touches from Module B.

and so on...

It's a very long and complex function, and I was wondering if I can rewrite this function with a better structure.

void MainGame::onTouchesBegan(const std::vector<Touch*>& touches, Event  *event)
{

    bool FontSignTouched = false; 
    for( auto& touch : touches)
    {
        auto location = touch->getLocation();
         //once the next level button is triggered to show all other touchs are disabled 

        Vector<Node *> ScoreContainerChildren = pScoreContainer->getChildren();
        for (auto iter = ScoreContainerChildren.begin(); iter != ScoreContainerChildren.end(); ++iter) 
        {
            Node *childNode = *iter; 
            if(childNode->getTag() == buttons_tags::SOUND_BT)
            {

                Point thisTouchScoreContainer = pScoreContainer->convertTouchToNodeSpace(touch); 
                Sprite* pSoundBT = static_cast<Sprite*>(*iter); 
                if(pSoundBT->getBoundingBox().containsPoint(thisTouchScoreContainer))
                {

                    pScoreContainer->setSoundButtonSpriteFrame(true);
                    break;
                }

            }

            if(childNode->getTag() == buttons_tags::POINTS_CONTAINER_NODE)
            {

                Sprite* pCoinsBT = (Sprite*)childNode->getChildByTag(buttons_tags::COINS_IMG_BT);
                Sprite* pCoinsCountFrameBT = (Sprite*)childNode->getChildByTag(buttons_tags::COINS_COUNT_FRAME);
                Point thisTouchPointsContainerNode = childNode->convertTouchToNodeSpace(touch); 

                if(pCoinsBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode))
                {

                    setPopUpWindow();
                    break;
                }


                if(pCoinsCountFrameBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode))
                {

                    setPopUpWindow();
                    break;
                }                
            }

        }
        Vector<Node *> FontSelectionContainerChildren = pFontSelectionContainer->getChildren();
        for (auto iter = FontSelectionContainerChildren.begin(); iter != FontSelectionContainerChildren.end(); ++iter) 
        {
            Node *childNode = *iter; 
            if(childNode->getTag() == sign_tags::LETTER_SIGH)
            {

                Point thisTouchPositionFontSelection = this->convertTouchToNodeSpace(touch); 
                Sign* pSign = static_cast<Sign*>(*iter); 
                if(pSign->getBoundingBox().containsPoint(thisTouchPositionFontSelection))
                {

                    Settings::getInstance()->getSoundManager().playEffect(FONT_TO_SOLUTION);
                    pSolutionContainer->setFontSelectionToSulotionFont(pSign);
                    FontSignTouched = true;
                    break;
                }
            }
        }

        Vector<Node *> thisSelectionChildren = this->getChildren(); 
        if(!FontSignTouched)
        {
            for (auto iter = thisSelectionChildren.begin(); iter != thisSelectionChildren.end(); ++iter) 
            {
                Node *childNode = *iter; 
                if(childNode->getTag() == sign_tags::LETTER_SIGH)
                {
                    Point thisTouchPositionFontSelection = this->convertTouchToNodeSpace(touch);  
                    Sign* pSign = static_cast<Sign*>(*iter); 
                    if(pSign->getBoundingBox().containsPoint(thisTouchPositionFontSelection))
                    {

                        Settings::getInstance()->getSoundManager().playEffect(SOLUTION_TO_FONT);
                        pFontSelectionContainer->removeFromMainParantAndSetInSprite(pSign);
                        break;
                    }

                }
                if(childNode->getTag() == buttons_tags::NEXT_BT)
                {
                    Point thisTouchPositionFontSelection = this->convertTouchToNodeSpace(touch);  
                    Sign* pSign = static_cast<Sign*>(*iter); 
                    if(pSign->getBoundingBox().containsPoint(thisTouchPositionFontSelection))
                    {

                    }
                }
            }
        }
        else
        {
            FontSignTouched = false;
        }
    }

} 
\$\endgroup\$
2
  • \$\begingroup\$ What UI toolkit is this based on? \$\endgroup\$ Aug 7 '14 at 7:35
  • \$\begingroup\$ cocos2d-x , but what im really looking for is general idea \$\endgroup\$
    – user63898
    Aug 7 '14 at 7:56
7
\$\begingroup\$

You can easily make the code more readable by breaking the large method into three smaller ones. My suggestion:

void MainGame::ProcessScoreContainerChildren(const Touch * touch)
{
    Vector<Node *> ScoreContainerChildren = pScoreContainer->getChildren();
    for (auto childNode : ScoreContainerChildren)
    {
        if(childNode->getTag() == buttons_tags::SOUND_BT)
        {
            Point thisTouchScoreContainer = pScoreContainer->convertTouchToNodeSpace(touch); 
            Sprite* pSoundBT = static_cast<Sprite*>(childNode); 
            if(pSoundBT->getBoundingBox().containsPoint(thisTouchScoreContainer))
            {
                pScoreContainer->setSoundButtonSpriteFrame(true);
                break;
            }
        }

        if(childNode->getTag() == buttons_tags::POINTS_CONTAINER_NODE)
        {
            Sprite* pCoinsBT = (Sprite*)childNode->getChildByTag(buttons_tags::COINS_IMG_BT);
            Sprite* pCoinsCountFrameBT = (Sprite*)childNode->getChildByTag(buttons_tags::COINS_COUNT_FRAME);
            Point thisTouchPointsContainerNode = childNode->convertTouchToNodeSpace(touch); 

            if(pCoinsBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode))
            {
                setPopUpWindow();
                break;
            }

            if(pCoinsCountFrameBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode))
            {
                setPopUpWindow();
                break;
            }                
        }
    }
}

void MainGame::ProcessSelectionChildren(const Touch * touch, bool & FontSignTouched)
{
    auto location = touch->getLocation();
    Vector<Node *> FontSelectionContainerChildren = pFontSelectionContainer->getChildren();
    for (auto childNode : FontSelectionContainerChildren) 
    {
        if(childNode->getTag() == sign_tags::LETTER_SIGH)
        {
            Point thisTouchPositionFontSelection = this->convertTouchToNodeSpace(touch); 
            Sign* pSign = static_cast<Sign*>(childNode); 
            if(pSign->getBoundingBox().containsPoint(thisTouchPositionFontSelection))
            {
                Settings::getInstance()->getSoundManager().playEffect(FONT_TO_SOLUTION);
                pSolutionContainer->setFontSelectionToSulotionFont(pSign);
                FontSignTouched = true;
                break;
            }
        }
    }

    Vector<Node *> thisSelectionChildren = this->getChildren(); 
    if(!FontSignTouched)
    {
        for (auto childNode : thisSelectionChildren) 
        {
            if(childNode->getTag() == sign_tags::LETTER_SIGH)
            {
                Point thisTouchPositionFontSelection = this->convertTouchToNodeSpace(touch);  
                Sign* pSign = static_cast<Sign*>(childNode); 
                if(pSign->getBoundingBox().containsPoint(thisTouchPositionFontSelection))
                {
                    Settings::getInstance()->getSoundManager().playEffect(SOLUTION_TO_FONT);
                    pFontSelectionContainer->removeFromMainParantAndSetInSprite(pSign);
                    break;
                }
            }
            if(childNode->getTag() == buttons_tags::NEXT_BT)
            {
                Point thisTouchPositionFontSelection = this->convertTouchToNodeSpace(touch);  
                Sign* pSign = static_cast<Sign*>(childNode); 
                if(pSign->getBoundingBox().containsPoint(thisTouchPositionFontSelection))
                {

                }
            }
        }
    }
    else
    {
        FontSignTouched = false;
    }
}

void MainGame::onTouchesBegan(const std::vector<Touch*>& touches, Event *event)
{
    bool FontSignTouched = false; 
    for (auto& touch : touches)
    {
        ProcessScoreContainerChildren(touch);
        ProcessSelectionChildren(touch, FontSignTouched);
    }
} 

With more knowledge of the project, you could probably break it down further into other sub-methods, possibly reusable ones too. But the point to note here is: Don't be afraid of breaking down a large function into a few case-specific helper methods. This is what C++ private methods are for.

Also, notice that I've changed all your for() loops to use range based iteration. This also made the code more compact and eliminated a few temp variables.

\$\endgroup\$
2
  • \$\begingroup\$ hey thanks for the response , but i dont look for breaking into methods im looking for pattern systematic logic flow \$\endgroup\$
    – user63898
    Aug 7 '14 at 14:50
  • 4
    \$\begingroup\$ @user63898 On Code Review, all aspects of your code are open for review, whether you ask for it or not. Ultimately, it's your decision as to whether an answer helps you or not, but this answer contains some good advice that you should seriously consider. \$\endgroup\$
    – Schism
    Aug 7 '14 at 15:24
4
\$\begingroup\$

There are two things that strike me, just looking at your code:

  • Some lines seem to be overly long. It seems to be at least partly due to the long names of the cocos2d-x API though. You could at least try to choose a smaller variable name for thisTouchPointsContainerNode.
  • There are many linebreaks you could do without.

This piece of code does the same thing twice for two different conditions:

if(pCoinsBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode))
{
    setPopUpWindow();
    break;
}


if(pCoinsCountFrameBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode))
{
    setPopUpWindow();
    break;
}

You could easily merge the two conditions:

if (pCoinsBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode)
    || pCoinsCountFrameBT->getBoundingBox().containsPoint(thisTouchPointsContainerNode))
{
    setPopUpWindow();
    break;
}

Vector<Node *> thisSelectionChildren = this->getChildren(); 
if(!FontSignTouched)
{
    // ...
}

Here, thisSelectionChildren is only used if FontSignTouched is false. Since the variable is not used later, you should move it inside the if to avoid creating a useless std::vector when FontSignTouched is true:

if(!FontSignTouched)
{
    Vector<Node *> thisSelectionChildren = this->getChildren();
    // ...
}

Also, why do you have an empty if statement?

if(pSign->getBoundingBox().containsPoint(thisTouchPositionFontSelection))
{

}
\$\endgroup\$
2
  • \$\begingroup\$ hehe i love when the variables has allot of info about the context they are i learned it from objectivc variable naming and i love it! when i go back to my code i want all the info i can get and i hate long comments ... \$\endgroup\$
    – user63898
    Aug 7 '14 at 15:20
  • \$\begingroup\$ @user63898 Having a meaningful name is good, but the name is used only twice for the same thing in a small function and never used again. Since small functions are good, you generally have the variable declaration under the eyes and this declaration alone provides the context you need. While I agree that some context can be good for public members, you generally don't need that much context for a mere temp name :p \$\endgroup\$
    – Morwenn
    Aug 7 '14 at 15:27

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.