HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Simple C++ File Indexer

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
filesimpleindexer

Problem

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 // INDEXER is a name which is too common. It has potential to conflict with other macros. it's better to use a descriptive name.
#define FILE_INDEXER_90738592_HA8965371_DE0E_4ff7_95A0_11B956535E12_H

#include
#include
#include

// Try to avoid including the entire std, since it contaminates the namespace and may cause ambiguous matches.
//using namespace std;

class FileIndexer // : public baseClass, it's a wise way to inherit basic functionalities from its super class, if necessary.
{
public :
typedef unsigned int FileType;
enum File_Type_Action
{
FILE_NEED_NOACTION = 0x0000,
FILE_NEED_BACKUP = 0x0001,
FILE_NEED_SCAN = 0x0002
};

struct FileTypeInfo
{
FileType type;
unsigned actionRequired;
};

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

struct FileDef
{
// using the standard constant FILENAME_MAX is much safer than a hard-coded constant.
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 wchar_t* name);

bool getFirstFile (const wchar_t* ext, unsigned int& fileNo, FileDef

Solution

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 pathNameST;
std::wstring initialPath = name;
pathNameST.push(initialPath);

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

Code Snippets

struct FileTypeInfo
{
    FileType type;
    unsigned actionRequired; // File_Type_Action actionRequired;
};
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;
};
/* virtual*/ ~FileIndexer ();
// ^^^^^^^^^  Remove (unless you want people to inhert and extend!
bool getFirstFile (const wchar_t* ext, unsigned int& fileNo, FileDef* fd);
//                             ^^^^                               ^^^^^
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};

Context

StackExchange Code Review Q#23079, answer score: 5

Revisions (0)

No revisions yet.