Here are some observations that may help you improve your code.
Don't abuse using namespace std
Putting using namespace std
at the top of every program is a bad habit that you'd do well to avoid.
Don't use system("cls")
There are two reasons not to use system("cls")
or system("pause")
. The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls
or pause
, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a separate functions cls()
and pause()
and then modify your code to call those functions instead of system
. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:
void cls()
{
std::cout << "\x1b[2J";
}
Isolate platform-specific code
In this code, there are several things that are DOS/Windows only including #include <conio.h>
and the getch()
and kbhit()
functions within that, and also system("cls");
that I've already mentioned. Your code runs successfully on Linux if I supply those missing functions, but it would be nice if there were an #ifdef WINDOWS
already in the code so that one could recompile without having to alter the source code.
Eliminate "magic numbers"
There are a few numbers in the code, such as 8
and 14
that have a specific meaning in their particular context. By using named constants such as BACKSPACE
or COLUMN_WIDTH
, the program becomes easier to read and maintain.
Don't abuse the ternary operator
The code currently includes this inscrutable bit of code:
key.size() ? key.pop_back(): 0;
This is not valid C++ even if your compiler accepts it. The problem is that the two options must both evaluate to the same type. std::string.pop_back()
returns void
but 0
is an int
. The fix is to simply use an if
instead which makes the code both valid and readable C++:
if (key.size()) {
key.pop_back();
}
Be careful with references
The for
loop starts with this:
map<string,int>::iterator &tter = contacts.begin();
That's not right because tter
should be an iterator, not a reference to an iterator. It should instead be written as:
map<string,int>::iterator tter = contacts.begin();
Or even better, you could use the next suggestion if your compiler is compliant.
Use "range-for" to simplify your code
Instead of using an explicit iterator, your for loop could use "range-for":
for (const auto &person : contacts)
Simplify I/O by defining a function
This code outputs each contact when needed but it does so using a relatively long formatting string. I'd suggest creating a function do that so that that formatting code is isolated to one place. In particular, I'd create an operator<<
function like so:
std::ostream& operator<<(std::ostream &out, const std::pair<const std::string, int> &p) {
return out
<< std::setw(4) << "Name : " << std::setw(10) << std::left << p.first
<< std::setw(14) << ", Number : " << std::setw(10) << p.second;
}
Note, too, that I've used simply p.first
rather than p.first.c_str()
.
Use an initializer list to initialize static data
Use of an initializer list simplifies construction of data structures:
std::map<std::string,int> contacts{
{"Pradip",999092456},
{"Raju",999092457},
{"Praveed",999092445},
{"Prasad",999092415},
{"Asim",999092785},
{"Massi",999092215}
};
Avoid continue
and break
Looping constructs in code are generally considerably easier to read and understand if they do not use continue
or break
. In this case, it's very easy to avoid them:
for (const auto &person : contacts) {
if (string::npos != person.first.find(key)) {
std::cout << person << std::endl;
}
}
Think about the user
There is no obvious way to gracefully exit the program. I'd suggest using a switch
to control when the program is done. Something like this:
bool done=false;
while (!done)
{
if ( kbhit() ){
key_code = getch();
cls();
switch (key_code) {
case BACKSPACE:
if (key.size()) {
key.pop_back();
}
break;
case CTRL_C:
done = true;
break;
default:
key+=key_code;
}
std::cout << "Contacts with : " << key << std::endl;
// etc.
Omit return 0
When a C++ program reaches the end of main
the compiler will automatically generate code to return 0, so there is no reason to put return 0;
explicitly at the end of main
.