Sign in to follow this  
Cl9

Neglecting container check, how bad is my teammate?

Recommended Posts

Does anyone find this code being very bad?

Can you rate this from 0 to 10. I am very interested what is your opinion on neglacting to check containers properly before accessing them.

 

Just for a little context, they spent around 2hours trying to find this error which was causing crashes:

void Unit::LoopUnitAI(Object & o)
{
    if(stateAction == uni::BSFollowNextCommand)
    {

        if(isUpdateLogic(uni::LOGIC_UPDATE_DELAY + rand()% uni::LOGIC_UPDATE_DELAY_DISTANCE))
        {
            if(listAIActions.front().check()) //<---- Error lies here
            {
                listAIActions.pop_front();
                isActionFinished = true;
            }
            if(isActionFinished)
            {
                if(listAIActions.size() > 0)
                {
                    isActionFinished = false;
                    listAIActions.front().action();
                }
                else
                {
                    setStateAction(*this, uni::BSStandGuard);
                }
            }
        }

    }
    else if(stateAction == uni::BSStandGuard)
    {
        if(isUpdateResetLogic(uni::LOGIC_RESET_DELAY))//Every so often check if there are no new commands
            setStateAction(*this, uni::BSFollowNextCommand);
    }
}
Edited by Cl9

Share this post


Link to post
Share on other sites

Before I could rate this code, is there any other code prior to this which checks that listAIActions is not empty?

 

Is the function you paste documented to state that the list should not be empty when it is called?

 

To me things like this are acceptable if clearly documented as expected behaviour... 

Share this post


Link to post
Share on other sites

Before I could rate this code, is there any other code prior to this which checks that listAIActions is not empty?

 

Is the function you paste documented to state that the list should not be empty when it is called?

 

To me things like this are acceptable if clearly documented as expected behaviour... 

 

There were no checks to ensure that listAIActions wasn't empty, or it wouldn't have been crashing surely? ;)

And I doubt he classed it as expected behaviour if it took him 2 hours to find...

(Basically, it was an oversight)

Edited by Cl9

Share this post


Link to post
Share on other sites
if(listAIActions.front().check()) //<---- Error lies here
{
   listAIActions.pop_front();
   isActionFinished = true;
}
if(listAIActions.size() > 0)
{
    isActionFinished = false;
    listAIActions.front().action();
}

The fact that code does two similar things with very different style suggest me that either those are writed by two different persons, in different times or the error one is just forget to clean/refactor. Bugs happen, get over it.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this