patterncppMinor
Simple C++ File Indexer
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
--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
Prefer std::wstring over character array:
This is not a polymorphic type. So no need to have a virtual destructor:
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:
In the case of ext: I would use
Unless
You can simplify and make this more readable:
The destructor is useless. Remove it:
Try not to use multiple lines to do very simple tasks. It makes it harder to read:
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& fdYou 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.