Return to Answer

• 1

• 17.3k
• 4
• 31
• 89

// Will remove these in the future 
using namespace std;
using namespace boost::filesystem;
#if BOOST_OS_LINUX // Linux
#define PLATFORM_CONFIG_PATH (getenv("HOME")+string("/.dict_config.txt"))
#define PLATFORM_RESET_COMMAND "reset"
#elif BOOST_OS_WINDOWS // Windows
#define PLATFORM_CONFIG_PATH (getenv("USERPROFILE")+string("\\My Documents\\dict_config.txt"))
#define PLATFORM_RESET_COMMAND "cls"
#else
#error "Untested platform"
#endif

A few points for the block above:

• First issue here is exposing the full namespaces into the global scope right on the header file. Since you have even noted that it should be removed, then just do it. If you don't remove it at once, chances are you will just continue to use the names without namespace qualification forever. In case you haven't read this thread yet: Why is "using namespace std;" considered bad practice?

• Now the macros. It would be better, in my opinion, to declare a couple inline functions instead. For the PLATFORM_RESET_COMMAND it would also be better to wrap the system() call in a function as well, since there might not be an equivalent to a system("cls") on some other platform you might wish to support at some point in the future. What I would suggest:

   #if BOOST_OS_LINUX
   inline std::string getPlatformConfigPath() 
   {
   return getenv("HOME") + string("/.dict_config.txt");
   }
   inline void platformClearConsole()
   {
   system("reset");
   }
   // The rest ...
  

Since we are talking about the system() function, it should be noted that this function is considered to be unsafe, since it might trigger the execution of an external untrusted process, so it might be used as an exploit. A program more concerned with data safety and security should never use this library function.

• One very minor thing, but those comments:

   #if BOOST_OS_LINUX // Linux
   ...
   #elif BOOST_OS_WINDOWS // Windows
   ...
  

Are completely redundant, since the macro is already properly named.

Other miscellaneous issues:

• Your ToString() method is redundant with the Standard Library. Take a look at std::to_string().

• I don't see any methods marked with const. Methods that don't change member data, such as your ShowHelp() can be marked with a const at the end to ensure const correctness.

• decltype in a place like this make the code too verbose, defeating the purpose of type inference:

decltype( dict.begin() ) it = dict.begin();

Use auto instead.

• Not the most idiomatic way to test for an empty container:

if ( !dict.size() ) // Dictionary is empty

Use the empty() method instead and then remove the comment.

• Instead of throwing std::string, the usual convention is throw one of the standard exception classes provided by the C++ Library: <stdexcept>. You can also derive from one of those classes and define a more specific exception type to your application.

• In the big if/else-if chain on DoReserved(), you should consider providing a fallback else that throws an exception or just assert, to shield yourself from silently ignoring a bad input value.

Separate data from presentation:

At the moment, your Dictionary class is more like a DictionaryConsoleApp. Both the dictionary lookup/management and user interface are crammed inside the same class. Not just that, but the class also does several other things, like file reading and writing. So the separation of concerns is far from ideal.

What you should aim next, in my opinion, is to separate the actual work of a Dictionary from the UI and data presentation. A Dictionary should only provide methods to lookup() a word or string and return its definition, as well as possibly allow the caller to define() a new entry, as it is in your case.

If you make this separation, then you can create a Graphical User Interface without having to change a single line of code in Dictionary.