Simple C++ File Indexer

Question

I didn't realize I am not a very good programmer at recently. I just come out from Uni, I want to become a reliable teammate. Please help me to improve my coding style/habbits. I have carefully re-written a file indexer class which I didn't do very well in my work. I come here, hope I can improve it in terms of code correctness, best practices and design pattern usage.

--FileIndexer.h--

#ifndef FILE_INDEXER_90738592_HA8965371_DE0E_4ff7_95A0_11B956535E12_H
#define FILE_INDEXER_90738592_HA8965371_DE0E_4ff7_95A0_11B956535E12_H

#include <vector>
#include <map>
#include <string>

class FileIndexer
{
public :
    typedef unsigned int FileType;
    enum File_Type_Action
    {
        FILE_NEED_NOACTION = 0x0000,
        FILE_NEED_BACKUP = 0x0001,
        FILE_NEED_SCAN = 0x0002
    };

    struct FileTypeInfo
    {
        FileTypeInfo(): type(), actionRequired (FILE_NEED_NOACTION) {};
        FileTypeInfo(FileType _type, unsigned _actionRequired): type (_type), actionRequired (_actionRequired) {};
        FileType type;
        unsigned actionRequired;  // if multiple actions are required, then actionRequired = FILE_NEED_BACKUP | FILE_NEED_SCAN.
    };

    typedef std::map<std::wstring, FileTypeInfo> DynamicFileType; // we use the map data structure to allow to add more file types easily in future.

    struct FileDef
    {
        wchar_t     drive[_MAX_DRIVE];
        wchar_t     name[_MAX_FNAME];
        wchar_t     path[_MAX_DIR];
        wchar_t     ext[_MAX_EXT];
        _fsize_t    size;
        unsigned int type;
    };

    FileIndexer ();
    virtual ~FileIndexer () {};

    unsigned int getMaxIndex ()
    {
        return m_maxIndices;
    }

    bool setMaxIndices (const unsigned int& maxIndices);

    bool processDirectory (const std::wstring& name);

    bool getFirstFile (const std::wstring& ext, unsigned int& fileNo, FileDef& fd);

    bool getNextFile (unsigned int& fileNo, FileDef& fd);

    void listFiles (const std::wstring& ext);

    bool needsAction (const File_Type_Action& action, const FileDef& fd);

    void addSupportedFileType (const std::wstring& ext, const unsigned& actionRequired = FILE_NEED_NOACTION);

    void listSupportedFileTypes ();

    // a possible functionality in future : import multiple file types
    virtual bool importSupportFileTypes (const unsigned int numFileTypes, const std::wstring* exts, const File_Type_Action* actionRequired)
    {
        return false;
    };

    // a possible functionality in future : export multiple file types to a file
    virtual bool exportSupportFileTypes (const std::wstring& filename)
    {
        return false;
    };

    void _testIndexer ();

protected:

    DynamicFileType m_supportedFileTypes;  //dynamical supported File types

    std::vector<FileDef>    m_fileIndices;         //allow more flexible memory allocation for file indexing

    unsigned int m_maxIndices;             // the maximum number of indices supported

private:

};

--FileIndexer.cpp--

#include "stdafx.h"
#include "FileIndexer.h"
#include <io.h>
#include <stack>



FileIndexer::FileIndexer()
{
    m_maxIndices = 1024;
    m_fileIndices.reserve( m_maxIndices );   // This saves computational time for std::vector repeatly realllocating the memory, when the elements are added one by one.
    // Initialise the supported file types
    m_supportedFileTypes[std::wstring(L".txt")] =  FileTypeInfo (1, FILE_NEED_NOACTION);
    m_supportedFileTypes[std::wstring(L".xml")] =  FileTypeInfo (2, FILE_NEED_NOACTION);
    m_supportedFileTypes[std::wstring(L".exe")] =  FileTypeInfo (3, FILE_NEED_SCAN);
    m_supportedFileTypes[std::wstring(L".doc")] =  FileTypeInfo (4, FILE_NEED_BACKUP);
    m_supportedFileTypes[std::wstring(L".xls")] =  FileTypeInfo (5, FILE_NEED_BACKUP | FILE_NEED_SCAN);
    m_supportedFileTypes[std::wstring(L".ppt")] =  FileTypeInfo (6, FILE_NEED_NOACTION);
}


bool FileIndexer::setMaxIndices (const unsigned int& maxIndices)
{
    if (maxIndices < m_fileIndices.max_size() )  //This checks against the theoretical limit on the size of a vector.
    {
        m_maxIndices = maxIndices;
        return true;
    }

    else
        return false;
}

void FileIndexer::addSupportedFileType(const std::wstring& ext, const unsigned& _actionRequired)
{
    std::wstring s(ext);
    DynamicFileType::iterator it = m_supportedFileTypes.find(s);

    if (it == m_supportedFileTypes.end())
    {
        m_supportedFileTypes[s] = FileTypeInfo (m_supportedFileTypes.size() + 1, _actionRequired);
    }

    else
    {
        it->second.actionRequired = _actionRequired;
    }
}



bool FileIndexer::processDirectory(const std::wstring& name)
{
    if (m_fileIndices.size() >= m_maxIndices) return false;

    std::stack<std::wstring> pathNameST;                                    //using stack is much more computationally efficient than the recursive function
    pathNameST.push(name);
    std::wstring curS, search, fname;
    FileDef f;
    DynamicFileType::const_iterator it;

    do
    {
        curS = pathNameST.top();
        pathNameST.pop();
        search = curS + L"\\*";
        _wfinddata_t fd;
        intptr_t handle = _wfindfirst(search.c_str(), &fd);

        if (handle != -1)
        {
            do
            {
                // if it's a sub-dir and not ".." and "." string, then push to stack
                if ( fd.attrib & _A_SUBDIR && wcscmp(fd.name, L"..") && wcscmp(fd.name, L"."))
                {
                    fname = curS + L"\\" + fd.name;
                    pathNameST.push(fname);
                } /* else if it's a file (except system files) then */

                else if (!(fd.attrib & _A_SYSTEM) && !(fd.attrib & _A_SUBDIR))
                {
                    f.size = fd.size;
                    fname = curS + L"\\" + fd.name;
                    _wsplitpath( fname.c_str(), f.drive, f.path, f.name, f.ext );
                    it = m_supportedFileTypes.find ( std::wstring(f.ext) );

                    if ( it != m_supportedFileTypes.end())
                    {
                        f.type = it->second.type;
                        m_fileIndices.push_back(f);

                        // if fileIndex.size is equal to the maximum number of indices, the process should be terminated.
                        if (m_fileIndices.size() == m_maxIndices)
                        {
                            _findclose (handle);
                            return true;
                        }
                    }
                }
            }
            while (_wfindnext(handle, &fd) == 0);
        }

        else
        {
            int err;
            _get_errno(&err);

            switch (err)
            {
            case EINVAL:
            case ENOMEM:
                _findclose (handle);
                return false;

            case ENOENT:
                break;

            default:
                _findclose (handle);
                return false;
            }
        }

        _findclose (handle);
    }
    while (!pathNameST.empty());

    return true;
}

bool FileIndexer::getFirstFile(const std::wstring& ext, unsigned int& fileNo, FileDef& fd)
{
    DynamicFileType::const_iterator it;
    bool ret = false;
    it = m_supportedFileTypes.find(std::wstring(ext));

    if (it == m_supportedFileTypes.end()) return false;

    FileType type = it->second.type;

    for (unsigned int i = 0; i < (unsigned int) m_fileIndices.size(); i++)
        if (m_fileIndices[i].type == type)
        {
            fd = m_fileIndices[i];
            fileNo = i;
            ret = true;
            break;
        }

        return ret;
}

bool FileIndexer::getNextFile(unsigned int& fileNo, FileDef& fd)
{
    bool ret = false;

    for (unsigned int i = fileNo + 1; i < (unsigned int) m_fileIndices.size(); i++)
        if (m_fileIndices[i].type == m_fileIndices[fileNo].type)
        {
            fd = m_fileIndices[i];
            fileNo = i;
            ret = true;
            break;
        }

        return ret;
}

void FileIndexer::listSupportedFileTypes ()
{
    DynamicFileType::const_iterator it;
    wprintf(L"%10s%15s\r\n", L"File Type", L"Extension");

    for (it = m_supportedFileTypes.begin(); it != m_supportedFileTypes.end(); it++)
    {
        wprintf(L"%5u%15s\r\n", it->second.type , it->first.c_str());
    }
}

void FileIndexer::listFiles(const std::wstring& ext)
{
    FileDef fd;
    unsigned int fileNo;

    if ( getFirstFile(ext, fileNo, fd) )
    {
        wprintf(L"%s files:\n", ext);
        wprintf(L"%90s%15s\r\n", L"NAME", L"SIZE");

        do
        {
            wprintf(L"%90s%15i\r\n", fd.name, fd.size);
        }
        while ( getNextFile(fileNo, fd));
    }
}

bool FileIndexer::needsAction(const File_Type_Action& action, const FileDef& fd)
{
    DynamicFileType::const_iterator it = m_supportedFileTypes.find ( std::wstring(fd.ext));

    if (it != m_supportedFileTypes.end())
    {
        return static_cast<bool>(it->second.actionRequired & action);
    }

    else
        return false;
}


void FileIndexer::_testIndexer()
{
    bool ret1 = setMaxIndices(3024);
    addSupportedFileType(L".pdf");
    listSupportedFileTypes();
    bool ret3 =  processDirectory(L"D:\\");
    bool ret4 =  processDirectory(L"C:\\");
    //processDirectory("C:\\test directory1" );
    //processDirectory("C:\\test directory2" );
    listFiles(L".exe");
    listFiles(L".txt");
    listFiles(L".xml");
    listFiles(L".bat");
}

The code is compiled and tested in Windows. I have already tried to carefully improve it by myself. I know there are still lots of things I could miss. Please help me on this.

Thank you

Update: Incorporated review comments from harald and Loki Astari

Answer 1

Here's a few things you may want to consider:

1. Remove commented out code and statements right away. Particularly the ones that seems to be left in there for educational purposes are just noise.

2. Wrap your class into a namespace, if this is part of a project you should at least have a project scope namespace.

3. Don't use fixed length char arrays for strings, this is C++ so use std::string or another suitable string implementation instead.

4. Make the FileDef type a class with it's own methods instead of manipulating the members directly. It can probably be generalized so it won't need to be a private class under FileIndexer.

You may also want to consider breaking the class into separate smaller classes, like DirectoryProcessor, FileIterator etc.

Answer 2

Presumable this is supposed to be enum File_Type_Action:

struct FileTypeInfo
{
    FileType type;
    unsigned actionRequired; // File_Type_Action actionRequired;
};

Prefer std::wstring over character array:

struct FileDef
{
    wchar_t     drive[_MAX_DRIVE];
    wchar_t     name[_MAX_FNAME];
    wchar_t     path[_MAX_DIR];
    wchar_t     ext[_MAX_EXT];
    _fsize_t    size;
    unsigned int type;
};

This is not a polymorphic type. So no need to have a virtual destructor:

/* virtual*/ ~FileIndexer ();
// ^^^^^^^^^  Remove (unless you want people to inhert and extend!

By making you destructor virtual you are indicating that this class can be extended. But you have no virtual methods. So I see no point in using vvirutal here (it is also misleading).

Pass by reference rather than pointer:

bool getFirstFile (const wchar_t* ext, unsigned int& fileNo, FileDef* fd);
//                             ^^^^                               ^^^^^

In the case of ext: I would use std::wstring const&.
Unless fd can be NULL pass by reference. This is an indication that you don't accept NULL values. FileDef& fd

You can simplify and make this more readable:

FileTypeInfo tmp;
tmp.type = 1;
tmp.actionRequired = FILE_NEED_NOACTION;
m_supportedFileTypes[std::wstring(L".txt")] =  tmp;

// In C++03 add a constructor.
m_supportedFileTypes[std::wstring(L".txt")] = FileTypeInfo(1, FILE_NEED_NOACTION);

// In C++11 use the new syntax:
m_supportedFileTypes[std::wstring(L".txt")] = {1, FILE_NEED_NOACTION};

The destructor is useless. Remove it:

FileIndexer::~FileIndexer()
{
    m_maxIndices = 0;   // The object is gone after this.
                        // Thus this value no longer exists.
                        // Thus accessing it would be undefined behavior
                        // Thus is does not matter its value.
}

Try not to use multiple lines to do very simple tasks. It makes it harder to read:

std::stack<std::wstring> pathNameST;
std::wstring initialPath = name;
pathNameST.push(initialPath);

// Can be done like this:
pathNameST.push(name);

Answer 3

You should certainly get rid of "stdio.h" and "stdlib.h"