Friday, June 29, 2018

Some flaw happened in clearing the memory

There is some amendment being done in clearMemory() for a very long time ago. I can't recall what I did last time, the code seemed likely been done in a rush and there are not test well. This is the piece I mention in this post:
void FileBot::clearMemory(FileNode *fileNode)
{
    bool lastNode = false;

    // this case will happened only when the root is supply
    if( fileList.size() == 0 || (fileList.begin())->sibling.empty() ) {
        BOOST_LOG_TRIVIAL(info) << "current node has no sibling: " << endl;
        return;
    }

    // if this is the only node in the container
    // clean up the container and then bail
    FileNode *firstNode = &*(fileList.begin());

    // test on first node is empty
    if( !firstNode->sibling.empty() && firstNode->sibling.size() == 0 ) {
        BOOST_LOG_TRIVIAL(info) << "This is the only node. Root size [" << fileList.size() << "]";
        fileList.erase_and_dispose(fileList.begin(), fileList.end(), DisposeFileNode());
        return;
    }

    // if this is not the only node in the container
    // clean up the container recursively
    if( fileNode == NULL ) {
       BOOST_LOG_TRIVIAL(info) << "current node has no sibling: " << fileNode;

       fileNode = &*(fileList.begin());
       fileList.erase_and_dispose(fileList.begin(), fileList.end(), DisposeFileNode());
       lastNode = true;
    }
    else {
        BOOST_LOG_TRIVIAL(info) << "current node has sibling: " << fileNode << " : " << fileNode->getName() << " sibling : " << fileNode->sibling.size();

        if( fileNode->sibling.size() > 0 ) {
            for( FileNodeListType::iterator it = fileNode->sibling.begin(); it != fileNode->sibling.end(); it++ ) {
                clearMemory(&*(it));
            }
        }
        else {
            cout << fileNode << " has no more sibling." << endl;
            return;
        }

        BOOST_LOG_TRIVIAL(info) << "cleaning sibling memory for this node: " << fileNode << " node name: " << fileNode->getName();
        fileNode->sibling.erase_and_dispose(fileNode->sibling.begin(), fileNode->sibling.end(), DisposeFileNode());
    }

    if( lastNode ) {
        BOOST_LOG_TRIVIAL(info) << "Reacing the root. Root size [" << fileList.size() << "]";
        fileList.erase_and_dispose(fileList.begin(), fileList.end(), DisposeFileNode());
    }
}
In the past few days I sat down just to review my test case, I found out some of the code was not covered by the test cases. Those uncover pieces are 16-20, 24-30, and 49-52. I was shocked when I see those uncovered pieces. I wonder whether those pieces are unused code since none of the test cases cover them up.

I study the code carefully, test run for few times and finally I got a fantastic conclusion. There are 2 sections are required for this function to carry on its job; first section would be the validation, this will validate whether the container, as well as, the FileNode's sibling contain any elements in it? This is the one:
    // this case will happened only when the root is supply
    if( fileList.size() == 0 || (fileList.begin())->sibling.empty() ) {
        BOOST_LOG_TRIVIAL(info) << "current node has no sibling: " << endl;
        return;
    }
Whereas the second section would be the core logic focus on cleaning the container, as well as, the sibling of each FileNode. Here is the one:
    BOOST_LOG_TRIVIAL(info) << "current node has sibling: " << fileNode << " : " << fileNode->getName() << " sibling : " << fileNode->sibling.size();

    if( fileNode->sibling.size() > 0 ) {
        for( FileNodeListType::iterator it = fileNode->sibling.begin(); it != fileNode->sibling.end(); it++ ) {
            clearMemory(&*(it));
        }
    }
    else {
         cout << fileNode << " has no more sibling." << endl;
         return;
    }

    BOOST_LOG_TRIVIAL(info) << "cleaning sibling memory for this node: " << fileNode << " node name: " << fileNode->getName();
    fileNode->sibling.erase_and_dispose(fileNode->sibling.begin(), fileNode->sibling.end(), DisposeFileNode());
Now the code is much more readable. Don't you agree? Oh! by the way, I already moved this function into the destructor since I'll invoke this function at the end of each unit test. Don't you feel much better?
FileBot::~FileBot()
{
    clearMemory(&*(fileList.begin()));
}

No comments: