Program simulating dictionary using file handling

I have made a program in C++ simulating a dictionary with very basic functionality. I do not have much experience in file handling with C++. Also, I want it to run on Linux and Windows.

It would be great if you could review/improve it on these things:

• The file handling part of the code

• Detecting whether the program is being compiled on Windows or Linux

• Code readability (variable/function naming)

• Displaying error messages

• Removing redundant code

• Improving fault-tolerance of the code

• C++ standard practices (I promise I will remove using namespace std; later)

• Any other general optimization improvement

Here is the code:

```
#include
using namespace std;
#ifdef DEBUG
#include
#endif

//***
// Global Declarations begin...

#if defined linux || defined __linux__ || defined __linux // Linux
const char* config_path=strcat(getenv("HOME"),"/.dict_config.txt"); // Path to store configuration File which will contain location of dictionary and its backup on Linux...
const char* clear_screen="reset";
#elif defined __WIN32 || defined __WIN64 // Windows
const char* config_path=strcat(getenv("USERPROFILE"),"\\My Documents\\dict_config.txt"); // Path to store configuration File which will contain location of dictionary and its backup on Windows...
const char* clear_screen="cls";
#endif

map dictionary,dictionary_bak; // The underlying Data Structure for dictionary...
char dict_path[1001]; // dict_path stores the location of the dictionary..
char bak_dict_path[1001]; // bak_dict_path stores the location of the backup dictionary..

// Global Declarations end...
//****

// Check if a file having the address "path" exists..
bool fileExists(const char* path)
{
ifstream fin(path);
return fin.good();
}

// Check i

The main problem with this code is that it is not C++.

It is basically a set of functions (ie. Just C code that happens to use some C++ structures and objects in passing). There is nothing inherently C++ about this code.

You should never be including this.

#include

Its an implementation file. Stick to the header files defined by the standard (they don't have .h on the end).

You covered this yourself:

using  namespace std;

Don't conditionally include headers.

#ifdef DEBUG  
#include  
#endif

If things are turned on/off by Debug it should be done inside the Debug.h file. As a user of the file I should not need to remember that I conditionally include it.

Don't put variables inside conditional blocks. You put the values that can be defined in the platform specific part but the variable declaration then use the definitions you use. and you should always put a check for untested platforms so that when it is ported it is easy to find the places that need to be ported.

#if defined linux || defined __linux__ || defined __linux  // Linux  
const char* config_path=strcat(getenv("HOME"),"/.dict_config.txt");  // Path to store configuration File which will contain location of dictionary and its backup on Linux...  
const char* clear_screen="reset";  
#elif defined __WIN32 || defined __WIN64  // Windows  
const char* config_path=strcat(getenv("USERPROFILE"),"\\My Documents\\dict_config.txt");  // Path to store configuration File which will contain location of dictionary and its backup on Windows...  
const char* clear_screen="cls";  
#endif

This is how I would do it:

#if defined linux || defined __linux__ || defined __linux  // Linux  
#define PLATFORM_CONFIG_PATH    strcat(getenv("HOME"),"/.dict_config.txt")
#define PLATFORM_RESET_COMMAND  "reset"

#elif defined __WIN32 || defined __WIN64  // Windows  
#define PLATFORM_CONFIG_PATH    strcat(getenv("USERPROFILE"),"\\My Documents\\dict_config.txt")
#define PLATFORM_RESET_COMMAND  "cls"

#else
#error "Untested platform"
#endif  

/*
 * Path to store configuration File
 * which will contain location of dictionary and its backup on Linux...
 */
const char* config_path  = PLATFORM_CONFIG_PATH;  
const char* clear_screen = PLATFORM_RESET_COMMAND;

PS The above code is also broken.

The strcat() command assumes there is enough space in the destination. This is not true. Additionally the result of getenv() is not modifiable (even if does return char* (C is not know for its const correctness)). So copy data onto the end of the string is not allowed

Looks like this is your main object. Global mutable state is a bad idea. Looks like this should be a class and all the following methods members of that class. It also makes the re-using the object easier (Note: The above is config_path is not mutable so it is OK to make it global).

map dictionary,dictionary_bak;  // The underlying Data Structure for dictionary...  
char dict_path[1001];       // dict_path stores the location of the dictionary..  
char bak_dict_path[1001];   // bak_dict_path stores the location of the backup dictionary..

If you are doing filesystem level stuff. It may be better to use file system API calls.

// Check if a file having the address "path" exists..  
bool fileExists(const char* path)  
{  
    ifstream fin(path);  
    return fin.good();  
}

Technically the above does not test for existence. It tests if you can open AND read it. So the code is wrong based on the comments (and its name).

Boost provides a cross platform API for file system access boost::file_system.

I have a problem with the following. As depending on the state of the file system it returns different things.

// Check if the location of a file having the address "path" is a valid location..  
bool locationExists(const char* path)  
{  
    if(fileExists(path))  // A file already exists there.. so OK..  
        return true;  

    ofstream fout(path);  // Try to create a file with write permissions...  
    if(fout.good())  // File is writable...  
    {  
        fout.close();  
        if(remove(path)) // Remove the file which was created...  
        {  
            perror("Fatal Error ");  
            cout<<"Unable to remove file - \""<<path<<"\"\nExiting\n"; // Permission Errors most probably...  
            exit(1);  
        }  
        return true;  
    }  
    return false;  
}

• Return true if the file exists.

Actually: If the file exists and is readable (but misleading name).

• OR Return true if the File can be created as writable.

Note: Subtle difference between one and two (readable/writable)

• If not readable and writable but not deletable exit(1)

• Otherwise return false.

None of these conditions match the name of the function.

Also I would prefer it throw an exception rather than call exit(1). The result is probably the same (application termination). But this would allow you to handle all logging in the same way

StackExchange Code Review Q#48759, answer score: 8