3
\$\begingroup\$

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:

  1. The file handling part of the code
  2. Detecting whether the program is being compiled on Windows or Linux
  3. Code readability (variable/function naming)
  4. Displaying error messages
  5. Removing redundant code
  6. Improving fault-tolerance of the code
  7. C++ standard practices (I promise I will remove using namespace std; later)
  8. Any other general optimization improvement

Here is the code:

#include<bits/stdc++.h>  
using  namespace std;  
#ifdef DEBUG  
#include<Debug.h>  
#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<string,string> 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 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;  
}  

// Input locations of dictionary and its backup from the user...  
void getLocations()  
{  
    while(1)  
    {  
        cout<<"\nEnter the path to store dictionary\n[ For example - /home/anmol/Documents/dictionary.txt ]\n";  
        cin.getline(dict_path,1000);  
        if(!locationExists(dict_path))  
        {  
            perror("Error ");  
            cout<<"Unable to create file at - \""<<dict_path<<"\"\nPlease Enter an alternate location\n";  
        }  
        else  
        {  
            break;  
        }  
    }  

    while(1)  
    {  
        cout<<"\nEnter the path to store backup of dictionary\n[ For example - /home/anmol/Documents/dictionary_bak.txt ]\n";  
        cin.getline(bak_dict_path,1000);  
        if(!locationExists(bak_dict_path))  
        {  
            perror("Error ");  
            cout<<"Unable to create file at - \""<<bak_dict_path<<"\"\nPlease Enter an alternate location\n";  
        }  
        else  
        {  
            break;  
        }  
    }  
}  

// Check if the configuration file is present or not  
// If it is present, then read it and check if the path to the dictionary file is valid or not...  
void checkConfig()  
{  
    if(!locationExists(config_path))  
    {  
        perror("Fatal Error ");  
        cout<<"Unable to create configuration file at - \""<<config_path<<"\"\nExiting\n";  
        exit(1);  
    }  

    if(!fileExists(config_path))  
    {  
        ofstream fout(config_path);   // Create a new configuration file...  
        if(fout.good())  
        {  
            fout<<"\n\t\t\tDo not modify this file!!!!\n\n#";  // Write warning message...  
            getLocations();   // Get the locations of dictionary and its backup...  
            fout<<dict_path<<"\n"<<bak_dict_path<<"\n";  // Write the locations to the config file...  
        }  
        else  
        {  
            perror("Fatal Error ");  
            cout<<"Unable to create configuration file at - \""<<config_path<<"\"\nExiting\n";  
            exit(1);  
        }  
    }  

    ifstream fin(config_path);  
    fin.ignore (std::numeric_limits<std::streamsize>::max(),'#');  // Eat the warning message...  
    fin.getline(dict_path,1000);  // Input the location of the dictionary from the config file into "dict_path"...  
    if(!locationExists(dict_path))  
    {  
        perror("Fatal Error ");  
        cout<<"The path for dictionary - \""<<dict_path<<"\" is invalid\nTry restarting the program\n";  
        if(remove(config_path))  // Remove the configuration file so that getLocations() gets invoked again...  
        {  
            perror("Fatal Error ");  
            cout<<"Unable to remove file - \""<<config_path<<"\"\nExiting\n"; // Permission Errors most probably...  
        }  
        exit(1);  
    }  
    fin.getline(bak_dict_path,1000);  //   Input the location of the backup dictionary from the config file into "bak_dict_path"...  
    if(!locationExists(bak_dict_path))  
    {  
        perror("Fatal Error ");  
        cout<<"The path for dictionary backup- \""<<bak_dict_path<<"\" is invalid\nTry restarting the program\n";  
        if(remove(config_path))  // Remove the configuration file so that getLocations() gets invoked again...  
        {  
            perror("Fatal Error ");  
            cout<<"Unable to remove file - \""<<config_path<<"\"\nExiting\n"; // Permission Errors most probably...  
        }  
        exit(1);  
    }  
    fin.close();  
}  

// Get the contents of the dictionary from "path" and store it in "mapping"..  
void readFromFile(const char* path,map<string,string> &mapping)  
{  
    if(!fileExists(path))  //  File is not present at "path"...  
    {  
        cout<<"Creating a new file at - "<<path<<"\n";  
        ofstream fout(path);  // Create a blank file at "path" to store the dictionary/dictionary_bak..  
        if(!fout.good())  
        {  
            perror("Fatal Error ");  
            cout<<"Unable to create a new file - \""<<path<<"\"\nExiting\n";  
            exit(1);  
        }  
        fout.close();  
    }  

    mapping.clear();  
    string word,meaning;  
    ifstream fin(path);  
    while(1)  
    {  
        getline(fin,word,'\n'); // A word is terminated by a '\n'..  
        if(fin.eof())  // End-Of-File reached..  
            break;  
        if(word.empty())  
            continue;  
        getline(fin,meaning,'$');  // A meaning is terminated by a '$'...  
        if(meaning.empty())  
            continue;  
        fin.get(); // Take the remaining '\n'..  
        mapping[word]=meaning;  
    }  
}  

// Save the dictionary to the file..  
void writeToFile(const char* path,map<string,string> &mapping)  
{  
    ofstream fout;  
    fout.open(path);  
    if(!fout.good())  // Unable to open file for writing...  
    {  
        perror("Fatal Error ");  
        cout<<"Unable to create a new file - \""<<path<<"\"\nExiting\n";  
        exit(1);  
    }  

    for(const auto& it:mapping)  
    {  
        fout<<it.first<<"\n"<<it.second<<"$\n";  
        // Word terminated by a '\n'..  
        // Meaning terminated by a "0\n"..  
    }  
}  

inline void show(const map<string,string> &mapping=dictionary,int at_a_time=0)  
{  
    // Show the contents of "mapping" with "at_a_time" word-meaning pairs displayed at a time...  
    int i=1;  
    cout<<"\t\t\t"<<"No. of Entries = "<<mapping.size()<<"\n\n";  
    if(at_a_time==0) // Show all the contents at once..  
        at_a_time=mapping.size();  
    for(const auto& it:mapping)  
    {  
        cout<<i<<"). "<<it.first<<"\n\n"<<it.second<<"\n";  
        if( (i%at_a_time == 0) && i!=(int)mapping.size() )  
        {  
            cout<<"\t\tPress <Enter> to resume..   ";  
            cin.get();  // Wait for the user...  
        }  
        ++i;  
    }  
}  

// Retrieve the part of "s" before a " " is encountered..  
inline string beforeSpace(const string &query)  
{  
    stringstream ss(query);  
    string ans="";  
    ss>>ans;  
    return ans;  
}  

// Retrieve the part of "s" after a " " is encountered..  
inline string afterSpace(const string &query)  
{  
    stringstream ss(query);  
    string ans="";  
    ss>>ans>>ans;  
    return ans;  
}  

// Check if the query recieved is a keyword ..  
inline bool isReserved(string query)  
{  
    transform(query.begin(),query.end(),query.begin(),::tolower);  
    static set<string> reserved= {"help","clear","rebuild","show","bak","showbak","loadbak","test","refresh","remove"};  
    return reserved.find(beforeSpace(query))!=reserved.end();  
}  

// Show the help menu..  
void showHelp()  
{  
    static map<string,string> help;  
    help["\n\"clear\""]="Clear screen...\n";  
    help["\n\"rebuild\""]="Delete all the present entries of the dictionary...\n";  
    help["\n\"show [no. of entries at a time]\""]="Show all the present entries of the dictionary...\n";  
    help["\n\"bak\""]="Backup all the present entries of the dictionary...\n";  
    help["\n\"showbak [no. of entries at a time]\""]="Show all the present entries of the Backup dictionary...\n";  
    help["\n\"loadbak\""]="Replace the current dictionary with the most recent backup...\n";  
    help["\n\"test\""]="Take a self-test by showing any randomly chosen word from the dictionary and making you guess its meaning...\n";  
    help["\n\"refresh\""]="Reload the dictionary from the file...\n";  
    help["\n\"remove <word>\""]="Remove a word...\n";  
    for(const auto& it:help)  
    {  
        cout<<it.first<<" -- "<<it.second;  
    }  
}  

// Empty the dictionary..  
void rebuild()  
{  
    dictionary.clear();  
    cout<<"Dictionary cleared!!\n\n";  
}  

// Remove a word-meaning pair from the dictionary..  
void removeWord(const string& word)  
{  
    decltype(dictionary.begin()) it;  
    if((it=dictionary.find(word))!=dictionary.end())  
    {  
        dictionary.erase(it);  
        cout<<word<<" erased!!\n";  
    }  
    else  
    {  
        cout<<"Word not found!!\n\n";  
    }  
}  

// Show a random word-meaning pair from the dictionary..  
void showRand()  
{  
    if(!dictionary.size()) // Dictionary is empty..  
        return;  
    decltype(dictionary.begin()) it=dictionary.begin();  
    advance(it,rand()%dictionary.size());  
    cout<<"\t\t\t\""<<it->first<<"\"\n\t\tPress <Enter> to show meaning..  ";  
    cin.get();  // Wait for an "enter"...  
    cout<<"\n\n"<<it->second<<"\n";  
}  

// Run the query..  
void process(const string &query)  
{  
    string before_space=beforeSpace(query);  
    string after_space=afterSpace(query);  
    transform(before_space.begin(),before_space.end(),before_space.begin(),::tolower);  
    if(before_space=="help")  
    {  
        showHelp();  
    }  
    else if(before_space=="rebuild")  
    {  
        rebuild();  
    }  
    else if(before_space=="show")  
    {  
        show(dictionary,atoi(after_space.c_str()));  
    }  
    else if(before_space=="clear")  
    {  
        system(clear_screen);  
        // The compiler keeps warning the value returned here is unused, but I do not know how to use it !!!  
    }  
    else if(before_space=="bak")  
    {  
        writeToFile(bak_dict_path,dictionary);  
    }  
    else if(before_space=="showbak")  
    {  
        readFromFile(bak_dict_path,dictionary_bak);  
        show(dictionary_bak,atoi(after_space.c_str()));  
    }  
    else if(before_space=="loadbak")  
    {  
        readFromFile(bak_dict_path,dictionary);  
        writeToFile(dict_path,dictionary);  
    }  
    else if(before_space=="test")  
    {  
        showRand();  
    }  
    else if(before_space=="remove")  
    {  
        removeWord(after_space);  
    }  
    else if(before_space=="refresh")  
    {  
        readFromFile(dict_path,dictionary);  
    }  
}  

// Input a Word/Query..  
inline string getQuery()  
{  
    string query="";  
    cout<<"\nEnter query..  ";  
    getline(cin,query,'\n');  
    return query;  
}  

// Input the meaning of a word..  
inline string getMeaning()  
{  
    string meaning="";  
    cout<<"\nEnter meaning..  ";  
    char c;  
    while(1)  
    {  
        c=cin.get();  
        if(meaning.size()&&meaning[meaning.size()-1]=='\n'&&c=='\n')  
            break;  
        meaning+=c;  
    }  
    return meaning;  
}  

// Add a word to the dictionary..  
void addWord(const string &word)  
{  
    decltype(dictionary.begin()) it;  
    if((it=dictionary.find(word))!=dictionary.end())  
    {  
        cout<<"'"<<it->first<<"' already exists!!...\n\n"<<it->second<<"\n\nWant to overwrite?? (y/n)  ";  
        char option;  
        cin>>option;  
        cin.ignore(numeric_limits<streamsize>::max(),'\n');  
        if(option=='y'||option=='Y')  
        {  
            string temp=getMeaning();  
            if(temp!=string("\n"))  
                it->second=temp;  
        }  
    }  
    else  
    {  
        string temp=getMeaning();  
        if(temp!=string("\n"))  
            dictionary[word]=temp;  
    }  
}  


// Main starts from here...  
int main()  
{  
    srand(time(NULL)); // For showRand()...  
    checkConfig();  
    readFromFile(dict_path,dictionary);  
    while(1)  
    {  
        string query=getQuery();  
        if(query=="") // Exit if the query entered is blank...  
            break;  
        else if(isReserved(query))  
            process(query);  
        else  
            addWord(query);  
    }  
    writeToFile(dict_path,dictionary);  

}  

In case you want to read a little more about how it should work, here is the Github Link .

\$\endgroup\$
3
  • \$\begingroup\$ Just two little things I noticed while scrolling over your code. 1) When comparing an std::string with a C-style string you use x != string("y") at some locations. This is a) inconsistent as you use x != "y" at other points and b) inefficient as you are building an std::string where it is not necessary (there are overloads for C-style strings for operator==, operator!=, compare(), ...). 2) To check that an std::string is empty you sometimes use x == "" and sometimes x.empty(). Here, too, stay consistent. I generally prefer empty(). \$\endgroup\$
    – Max Truxa
    Commented May 2, 2014 at 14:38
  • 1
    \$\begingroup\$ This is really C code pretending to be C++. \$\endgroup\$ Commented May 2, 2014 at 18:23
  • \$\begingroup\$ Sorry for replying so late. My practical exams had been preponed. \$\endgroup\$ Commented May 8, 2014 at 15:32

2 Answers 2

8
\$\begingroup\$

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<bits/stdc++.h>  

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<Debug.h>  
#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<string,string> 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;  
}  
  1. Return true if the file exists.
    Actually: If the file exists and is readable (but misleading name).
  2. OR Return true if the File can be created as writable.
    Note: Subtle difference between one and two (readable/writable)
  3. If not readable and writable but not deletable exit(1)
  4. 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 (catch all exceptions in main and have the same logging applied to all points in your code). Or potentially the exception could be fixed.

Don't use this version of getline.

        cin.getline(dict_path,1000);  

What happens if the user enters a really long path. Prefer to use std::getline(). This version reads from a stream into a string that is auto resized to make room for user input.

Don't manually close a file (let RAII do it). Unless you are explicitly checking for a close error and prepared to compensate for it.

    fin.close();  

It is easier to put the read as part of the while condition (so that the loop is entered only if both reads succeed).

A couple of other things to note (they may be intentional and if so they should be commented). If word is valid then an empty meaning causes both to be dropped (ie not put in the map). If meaning hits eof (ie there is none) you don't test for this (thus meaning is left unchanged) you don't exit the loop. This will put random (probably the last value) value into the map.

    mapping.clear();  
    string word,meaning;  
    ifstream fin(path);  
    while(1)  
    {  
        getline(fin,word,'\n'); // A word is terminated by a '\n'..  
        if(fin.eof())  // End-Of-File reached..  
            break;  
        if(word.empty())  
            continue;  
        getline(fin,meaning,'$');  // A meaning is terminated by a '$'...  
        if(meaning.empty())  
            continue;  
        fin.get(); // Take the remaining '\n'..  
        mapping[word]=meaning;  
    }  
} 

I would write it like this:

    mapping.clear();  
    std::string word;
    std::string meaning;  
    ifstream    fin(path);  
    while(std::getline(fin, word))  
    {  
        if(word.empty())  
            continue;  

        if (getline(fin,meaning,'$') && (!meaning.empty())  
        {
            mapping.insert(std::make_pair(word, meaning));
            // I believe C++11 has emplace for this.
        }
        fin.get(); // Take the remaining '\n'..  
    }  
} 

Your code also assumes that meaning is terminated by '$\n'. I don't see the point in having a double set of character to terminate meaning. I would just make it '\n' terminated.

The keyword inline is usless. Don't use it unless the language requires. You don't need it here so don't use it.

inline void show(const map<string,string> &mapping=dictionary,int at_a_time=0)  

This does not do what the comment (and the function name says).

// Retrieve the part of "s" before a " " is encountered..  
inline string beforeSpace(const string &query)  
{  
    stringstream ss(query);  
    string ans="";  
    ss>>ans;  
    return ans;  
}  

This drops leading space. The returns the first white space separated word.

This sort of says what it does (but not very accurately).

// Retrieve the part of "s" after a " " is encountered..  
inline string afterSpace(const string &query)  
{  
    stringstream ss(query);  
    string ans="";  
    ss>>ans>>ans;  
    return ans;  
}  

It returns the second space separated word.

Hmmm. decltype(dictionary.begin()) it;

Easier to write:

    auto it = dictionary.begin();

Hmm.

    string before_space=beforeSpace(query);  
    string after_space=afterSpace(query);  

    // easier to write as:
    string before_space;  
    string after_space;
    std::stringstream(query) >> before_space >> after_space;

Rather than have a big if statement.

    transform(before_space.begin(),before_space.end(),before_space.begin(),::tolower);  
    if(before_space=="help")  
    {  
        showHelp();  
    }  
    else ......

Look up the Command Pattern this will make it much easier to read.

\$\endgroup\$
25
  • \$\begingroup\$ 1. Its an implementation file. Stick to the header files defined by the standard (they don't have .h on the end). I am compiling using these compiler flags -: mingw32-g++.exe -O2 -Wshadow -Winit-self -Wredundant-decls -Wfloat-equal -Winline -Wunreachable-code -pedantic -std=c++11 -Wfatal-errors -Wextra -Wall What other flags do I need to include to enforce the standards ?? \$\endgroup\$ Commented May 8, 2014 at 14:06
  • \$\begingroup\$ Don't conditionally include headers. Thanks for the suggestion. I have instead changed my IDE's settings to include that header file from the command line using the compiler flag -include. \$\endgroup\$ Commented May 8, 2014 at 14:30
  • \$\begingroup\$ 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* Is there any alternative correct way to implement this? \$\endgroup\$ Commented May 8, 2014 at 14:42
  • \$\begingroup\$ 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). Actually I wanted to check if I could read the file or not. Maybe I should change the function name to something like bool isFileReadable(const char* path). \$\endgroup\$ Commented May 8, 2014 at 14:52
  • 1
    \$\begingroup\$ @AnmolSinghJaggi Loki has made many excellent points in this review, which may be overwhelming for you. Please take some time to research each point. Many of your follow-up queries would make good Stack Overflow questions, e.g. "I wrote const char* config_path=strcat(getenv("HOME"),"/.dict_config.txt"); and was told in a review that the code was broken. Why?" Or, "I was told not to #include <bits/stdc++.h> in my C++ program. What should I write instead, and why?" \$\endgroup\$ Commented May 8, 2014 at 18:24
4
\$\begingroup\$
  1. Do not pollute the global namespace. Put this stuff:

    #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
    

    into its own namespace.
    Using your own namespace can help you avoid any possible name clashes.

  2. I could be wrong, but #include<bits/stdc++.h> does not seem to be a standard, portable header. I would manually include all of the headers that you need instead of using it.

  3. In most cases, prefer std::string over const char *, especially for buffers. std::string, like all the other standard containers, manages its own memory and provides less headaches to use.

    Let's take a look at this code:

    cout<<"\nEnter the path to store dictionary\n[ For example - /home/anmol/Documents/dictionary.txt ]\n";  
    cin.getline(dict_path,1000);
    

    Rather than worrying about buffer sizes, you could do something like this:

    std::string strDictionaryPath ;
    std::cout<<"\nEnter the path to store dictionary\n[ For example - /home/anmol/Documents/dictionary.txt ]\n";
    std::cin >> strDictionaryPath ;
    
  4. Using C-functions in a C++ application is almost always the wrong thing to do. At the very minimum, you should not mix C-IO functions with C++-IO functions.

    Rather than using perror, write to std::cerr instead. If you really need the error message that perror gives, then take a look at this solution.

  5. Avoid using exit. If anybody else has to use your code, then it would be very frustrating to have their software close whenever one of your functions fail. It would be better to throw an exception in most cases.

Below I have a rather incomplete and somewhat messy interface. This is not the best way of doing this, it is just to give you an idea of how to do it the "C++ way". To keep things simple (for myself), I use an std::vector instead of an std::map, but once again, this is just to give you a rough idea on how to do this.

For my own convenience, I did this all in a single file. You should break this up into multiple files. Here are the headers that I used:

#include <algorithm>
#include <cctype>
#include <fstream>
#include <iostream>
#include <iterator>
#include <string>
#include <vector>

First, we would create a struct to store each dictionary entry.

struct DictionaryEntry
{
    DictionaryEntry () ;
    DictionaryEntry (const std::string &word, const std::string definition) ;

    std::string word_ ;
    std::string definition_ ;
};

For convenience, we'll implement a default constructor and a constructor that takes two arguments. Notice that we use a constructor initialization list with the second constructor.

DictionaryEntry::DictionaryEntry ()
{
}

DictionaryEntry::DictionaryEntry (const std::string &word, const std::string definition) : word_(word), definition_(definition)
{
}

Then we overload a few operators. Please note that these are not member functions. Here is a good guide for operator overloading.

// This will be useful for finding words in our dictionary.
bool operator== (const DictionaryEntry &lhs, const DictionaryEntry &rhs)
{
    return lhs.word_ == rhs.word_ ;
}

// This will be useful for sorting words in our dictionary.
bool operator< (const DictionaryEntry &lhs, const DictionaryEntry &rhs)
{
    return lhs.word_ < rhs.word_ ;
}

// This will be useful for saving dictionary entries to a file.
template <class Stream>
Stream& operator<< (Stream &os, const DictionaryEntry &de)
{
    os << de.word_ << de.definition_ << "$" "\n" ;
    return os ;
}

// This will be useful for extracting words from a file.
template <class Stream>
Stream& operator>> (Stream &is, DictionaryEntry &de)
{
    is >> de.word_ ;
    std::getline (is, de.definition_, '$') ;
    return is ;
}

Then we would create a dictionary class. Here's an idea of what the interface could look like. Please note that this interface is incomplete and not the greatest design, but it should get you going in the right direction. Also note, as I said before, I'm using std::vector just as an example here. I'll leave altering this class to use std::map as an exercise for you.

class Dictionary
{
    typedef std::vector <DictionaryEntry>::const_iterator const_iterator ;

public:
    Dictionary () ;
    Dictionary (const std::string &file_path) ;
    ~Dictionary () ;

    const_iterator cbegin () const {
        return entries_.cbegin () ;
    }

    const_iterator cend () const {
        return entries_.cend () ;
    }

    void Insert (DictionaryEntry de) ;
    void Insert (const std::string &word, const std::string &definition) ;

    void Load (std::string file_path) ;
    void Save (const std::string &file_path) ;

    const_iterator LookupWord (const std::string &word) const ;

private:
    bool valid_ ;
    std::string file_path_ ;
    std::vector <DictionaryEntry> entries_ ;
};

In the second constructor for Dictionary, we attempt to load the file right away. This is somewhat similar to calling the constructor of an std::ofstream, but not quite.

Dictionary::Dictionary () : valid_ (false) 
{
}

Dictionary::Dictionary (const std::string &file_path) : valid_ (false)
{
    this->Load (file_path) ;
}

If an exception is thrown for some reason, as long as our Dictionary contains valid values, our destructor will attempt to write the values back to a file. This way, Dictionary manages its own resources and you do not have to worry about calling save manually. This concept is called Resource Acquisition Is Initialization (RAII). Note the try-catch block. It's there because a destructor should not throw exceptions.

Dictionary::~Dictionary ()
{
    // A destructor must not throw an exception.
    try {
        this->Save (file_path_) ;
    }

    catch (std::ios_base::failure &e) {
        std::cerr << e.what () ;
    }
}

This Insert method does the job, but not really well. Because I used std::vector rather than std::map, I have somehow take care of duplicate words. My strategy here (out of laziness) is to simply allow them. Also note that my operator>> stores '\n', which makes this even more ugly.

void Dictionary::Insert (DictionaryEntry de)
{
    std::string &word = de.word_ ;
    std::transform (std::begin (word), std::end (word), std::begin (word), std::tolower) ;

    word += "\n" ; // ugly, but '\n' are embedded already.
    de.definition_ += "\n" ; // ugly, but '\n' are embedded already.

    entries_.push_back (de) ;
    valid_ = true ; // Useful if we are creating the dictionary from scratch.
}

void Dictionary::Insert (const std::string &word, const std::string &definition)
{
    this->Insert (DictionaryEntry (word, definition)) ;
}

In this Load function, I do not set the failbit as a condition to throw an exception. This is because std::copy will set the failbit when its done (at least on Visual Studio 2012). Because of this, we have to manually check if the file was properly opened. Notice how member variable are not modified until the very end of this function. This is so that in case the function decides to throw an exception, the class will not be placed in an invalid state. This means this function is conscious about exception safety.

void Dictionary::Load (std::string file_path)
{
    std::ifstream file (file_path) ;
    file.exceptions (std::ios::badbit) ;

    if (file.is_open () == false) {
        throw std::ios_base::failure ("Could not open file.") ;
    }

    std::vector <DictionaryEntry> entries ;

    auto begin = std::istream_iterator <DictionaryEntry> (file) ;
    auto end = std::istream_iterator <DictionaryEntry> () ;
    std::copy (begin, end, std::back_inserter (entries)) ;

    // Copy data to member functions:
    std::swap (file_path_, file_path) ;
    std::swap (entries_, entries) ;
    valid_ = true ;
}

Here is where the operator< becomes useful. Before we write our dictionary entries back to disk, we need to sort them. Other than that, this Save function is pretty straight forward.

void Dictionary::Save (const std::string &file_path)
{
    if (valid_) {
        std::sort (std::begin (entries_), std::end (entries_)) ;

        std::ofstream file (file_path) ;
        file.exceptions (std::ios::badbit) ;

        if (file.is_open () == false) {
            throw std::ios_base::failure ("Could not open file.") ;
        }

        auto out = std::ostream_iterator <DictionaryEntry> (file) ;
        std::copy (std::begin (entries_), std::end (entries_), out) ;
    }
}

This is somewhat ugly for this interface, but this is a more std-like approach. This is also where our operator== comes in handy. Basically, if we do not find the word, we return entries.cend(), which is basically a pointer that points past the end of our vector.

Dictionary::const_iterator Dictionary::LookupWord (const std::string &word) const
{
    return std::find (std::begin (entries_), std::end (entries_), DictionaryEntry (word, "")) ; 
}

And here is a sample main function:

int main (void) 
{
    Dictionary dictionary ;

    try {
        dictionary.Load ("dictionary.txt") ;

        auto entry = dictionary.LookupWord ("proliferate") ;
        if (entry != dictionary.cend ()) {
            std::cout << *entry ;
        }

        dictionary.Insert ("MyNewWord", "My new definition.") ;
    }

    catch (std::ios_base::failure &e) {
        std::cerr << e.what () << "\n" ;
        return 1 ;
    }

    return 0 ;
}

Once again, this isn't the best way to do it. This is just to get you to start writing C++ instead of C with Classes

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.