Directory traversal issue

I'm trying to write a function that calculates the size of a given directory tree. It's your run-of-the-mill directory traversal that simply adds the sizes of each file. It should be simple enough, but for some reason the function doesn't return the correct size for all folders. I'm not sure if it doesn't report large file sizes correctly or large folder sizes, or maybe it skips some folders - I don't know. Some folders it reports fine, and others not. Any ideas?

Code:

---

long CDirectoryInfo::GetDirectorySize(CString path)
{
    WIN32_FIND_DATA data;
    HANDLE hFind;
    CString szCurrentPath = path + "\\*.*";
    hFind = FindFirstFile((LPCTSTR)szCurrentPath, &data);
    if(hFind == INVALID_HANDLE_VALUE)
    {
        return -1;
    }
    do
    {
        if(data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
        {
            if((strcmp(data.cFileName, ".") != 0) && (strcmp(data.cFileName, "..") != 0))
        {
            szCurrentPath = path + "\\" + data.cFileName;
            GetDirectorySize(szCurrentPath);
                        }
        }
        else
        {
            m_lDirectorySize += data.nFileSizeHigh * (MAXDWORD + 1) + data.nFileSizeLow;
        }
    }
    while(FindNextFile(hFind, &data));
    FindClose(hFind);

    return m_lDirectorySize;
}

---

You ignore the result of your recursive calls.

Quote:

---

Originally Posted by Salem

You ignore the result of your recursive calls.

---

I'm not sure why that applies. Forgive my ignorance for saying so. As I see it, m_lDirectorySize is a member variable that gets initialized to zero in the class constructor. Then, each time the the function finds a (non-directory) file, it increases m_lDirectorySizeby the size of that file. Why, then, would I need to bother with the intermediate return values? I could instead write the function to not return a value, and write another function, like long GetDirectorySize() that returns m_lDirectorySize. The only thing I can think of is that each time the function calls itself, a "temporary" object of the class is created, reinitializing the value of m_lDirectorySizeto zero. If that's the case, would I declare m_lDirectorySize static?

OK, so why do you have
return m_lDirectorySize;
at the end then?

Either it always returns the result and you don't need a member variable, or it never returns the result and you have another member function to retrieve the total. Choose one (it doesn't matter), but don't have sloppy interfaces like this.

The next step would be say

Code:

---

long CDirectoryInfo::GetDirectorySize(CString path)
{
    WIN32_FIND_DATA data;
    HANDLE hFind;
    CString szCurrentPath = path + "\\*.*";
    hFind = FindFirstFile((LPCTSTR)szCurrentPath, &data);
    if(hFind == INVALID_HANDLE_VALUE)
    {
        return -1;
    }
    do
    {
        if(data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
        {
            if((strcmp(data.cFileName, ".") != 0) && (strcmp(data.cFileName, "..") != 0))
            {
                szCurrentPath = path + "\\" + data.cFileName;
                cout << "Recursing to " << szCurrentPath;
                GetDirectorySize(szCurrentPath);
            }
        }
        else
        {
            m_lDirectorySize += data.nFileSizeHigh * (MAXDWORD + 1) + data.nFileSizeLow;
            cout << "Total size currently " << m_lDirectorySize;
        }
    }
    while(FindNextFile(hFind, &data));
    FindClose(hFind);

    return m_lDirectorySize;
}

---

Add some debug to the code, and watch what happens.
Or maybe use the debugger to put breakpoints on those locations, and watch the variables.

> The only thing I can think of is that each time the function calls itself,
> a "temporary" object of the class is created,
So test your idea with some debug in the constructor.

Are there very large files?
On the face of it, your size calculation overflows.

Quote:

---

Originally Posted by Mostly Harmless

Code:

---

        if(data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
        {
            if((strcmp(data.cFileName, ".") != 0) && (strcmp(data.cFileName, "..") != 0))
        {
            szCurrentPath = path + "\\" + data.cFileName;
            GetDirectorySize(szCurrentPath);
                        }
        }

---

---

Salem fixes your little indentation "problem."
Avoid from doing that, it makes the code look sloppy, ugly and it gets harder to read.
Tabs are better than spaces too, if you ask me.

It looks like you're not processing the results of your FindFirstFile call:

Code:

---

long CDirectoryInfo::GetDirectorySize(CString path)
{
    WIN32_FIND_DATA data;
    HANDLE hFind;
    CString szCurrentPath = path + "\\*.*";
    hFind = FindFirstFile((LPCTSTR)szCurrentPath, &data);
    if(hFind == INVALID_HANDLE_VALUE)
    {
        return -1;
    }
        if (data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY &&
                (strcmp(data.cFileName, ".") != 0) &&
                (strcmp(data.cFileName, "..") != 0))
        {
                szCurrentPath = path + "\\" + data.cFileName;
                GetDirectorySize(szCurrentPath);
        }
        else
        {
                if((strcmp(data.cFileName, ".") != 0) && (strcmp(data.cFileName, "..") != 0))
                {
                        m_lDirectorySize += data.nFileSizeHigh * (MAXDWORD + 1) + data.nFileSizeLow;
                }
        } 
    do
    {
        if(data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)
        {
            if((strcmp(data.cFileName, ".") != 0) && (strcmp(data.cFileName, "..") != 0))
        {
            szCurrentPath = path + "\\" + data.cFileName;
            GetDirectorySize(szCurrentPath);
                        }
        }
        else
        {
            m_lDirectorySize += data.nFileSizeHigh * (MAXDWORD + 1) + data.nFileSizeLow;
        }
    }
    while(FindNextFile(hFind, &data));
    FindClose(hFind);

    return m_lDirectorySize;
}

---