HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Contact search application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contactsearchapplication

Problem

Create a C++ program which will act like contact search app in mobile.
For example, if the following contacts are saved in contact list:



Massi

Pradip

Prasad

Praveed

Raju



And if user type "a" then following names should be displayed:



Massi

Pradip

Prasad

Praveed

Raju



if user type "Pr" then following names should be displayed:



Pradip

Prasad

Praveed



Basically it should display all the names which contain the search
string anywhere in name.

#include  
#include  
#include 
#include 
#include 
#include

using namespace std; 

int main() { 
    map contacts;
    contacts["Pradip"]=999092456;
    contacts["Raju"]=999092457;
    contacts["Praveed"]=999092445;
    contacts["Prasad"]=999092415;
    contacts["Asim"]=999092785;
    contacts["Massi"]=999092215;
    int key_code;
    string key;
    while (true)
    {

            if ( kbhit() ){

                key_code = getch();
                system("CLS");

                if (key_code == 8 ){
                    key.size() ? key.pop_back(): 0;
                }
                else{
                    key+=key_code;
                }
                cout::iterator &tter = contacts.begin();tter != contacts.end();tter++)
                {
                    if (string::npos == tter->first.find(key) && !key.empty()){
                        continue;
                    }
                    std::coutfirst.c_str()second<<std::endl;
                }

            }
            else 
                continue;
    }
    return 0; 
}

Solution

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 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::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::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 &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 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.

Code Snippets

void cls()
{
    std::cout << "\x1b[2J";
}
key.size() ? key.pop_back(): 0;
if (key.size()) {
    key.pop_back();
}
map<string,int>::iterator &tter = contacts.begin();
map<string,int>::iterator tter = contacts.begin();

Context

StackExchange Code Review Q#96656, answer score: 7

Revisions (0)

No revisions yet.