Thursday, November 1, 2018

Pretty bad design on the return of constructParentPath()

Not long ago, there was some major change on constructParentPath() in order to maintain 2 different types of request, one for local home path and another one for the remote home path. The FileListType is used to determine whether this function is generated in local home path or remote home path before it get returned. See the code snippet below:
FileNode* FileBot::constructParentPath(boost::filesystem::path inputPath, FileListType listType)
{
    FileNode *parent = NULL;

    ...
    ...

    for( string_split_iterator it = make_split_iterator(path, first_finder(GENERIC_PATH_SEPARATOR, is_equal()));
         it != string_split_iterator();
         ++it)
    {
        ...
        ...

        if( parent == NULL )
            if( listType == local )
                localList.push_back(*node);
            else
                remoteList.push_back(*node);
        else {
            parent->sibling.push_back(*node);
            node->setParentNode(parent);
        }

        ...
        ...
    }

    ...

    if( listType == local ) {
        localHome = parent;
        return localHome;
    }
    else {
        remoteHome = parent;
        return remoteHome;
    }
}
Notice the parent is assigned back to the respective home variable, I was thinking this code might need to rework if there is another third option is coming in. Pretty bad, right?!! Thus, I just feel the return code at the end is redundant. To prevent that from happening in future, I might as well just return the parent and let the caller to decide where the value should assign.
FileNode* FileBot::constructParentPath(boost::filesystem::path inputPath, FileListType listType)
{
    FileNode *parent = NULL;

    ...
    ...

    return parent;
}
As for this change, there are 2 callers on this function. One is loadLocalFS() and another one is loadRemoteFS(). Below are the changes made to adopt this new change:
FileNode* FileBot::loadLocalFS(boost::filesystem::path inputPath)
{
    localHome = constructParentPath(inputPath);
    constructChildPath(localHome);

    ...
}


FileNode* FileBot::loadRemoteFS(vector<string> fileList)
{
    ...

    // load up the abstract path for first entry
    remoteHome = constructParentPath(fileList[0], remote);

    ...
    ...
}

No comments: