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? 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 thesystem()
call in a function as well, since there might not be an equivalent to asystem("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 ...
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 thesystem()
call in a function as well, since there might not be an equivalent to asystem("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 ...
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 thesystem()
call in a function as well, since there might not be an equivalent to asystem("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 ...
// 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 thesystem()
call in a function as well, since there might not be an equivalent to asystem("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 atstd::to_string()
.I don't see any methods marked with
const
. Methods that don't change member data, such as yourShowHelp()
can be marked with aconst
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 isthrow
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 fallbackelse
that throws an exception or justassert
, 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
.