6
\$\begingroup\$

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 <iostream> 
#include <stdio.h> 
#include <conio.h>
#include <string>
#include <map>
#include<iomanip>

using namespace std; 



int main() { 
    map<string,int> 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<<"Contacts with : "<<key<<endl;

                for (map<string,int>::iterator &tter = contacts.begin();tter != contacts.end();tter++)
                {
                    if (string::npos == tter->first.find(key) && !key.empty()){
                        continue;
                    }
                    std::cout<<setw(4)<<"Name : "<<setw(10)<< std::left <<tter->first.c_str()<<setw(14)<<", Number : "<<setw(10)<<tter->second<<std::endl;
                }

            }
            else 
                continue;
    }
    return 0; 
}
\$\endgroup\$
1
  • 7
    \$\begingroup\$ I went ahead and added the misplaced code into your question. The edit will bump your question into a review queue to be reopened (I suspect it will). You were being downvoted at first because it seemed you hadn't implemented the code yet, so there was nothing to review. Your answer was downvoted because it wasn't a review, but should have been an edit to the question. Sorry about any confusion. You may want to take some time to read over our help center. Welcome to CR. \$\endgroup\$
    – RubberDuck
    Commented Jul 12, 2015 at 13:22

2 Answers 2

7
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

Formatting

There are several coding style issues that hurt readability:

  • Inconsistent indentation: the body of the main while is over-indented
  • Inconsistent placement of braces: sometimes you put the opening brace { on the same line as the statement, sometimes on the next line. There is no standard for this, but one thing is clear, that you should be consistent, and do it the same way everywhere
  • Put spaces around operators, for example:
    • contacts["Pradip"]=999092456; -> contacts["Pradip"] = 999092456;
    • key+=key_code; -> key += key_code;
    • std::cout<<setw(4)<<"Name : " -> std::cout << setw(4) << "Name : "
  • It's recommend to put braces on all if-else statements, even if the body would have a single line, for example in else continue

Other obvious coding style problems

Avoid using namespace std, it's considered bad practice.

Declare variables in the smallest scope necessary. For example key_code could be declared right where you assign a value to it:

int key_code = getch();

The loop variable tter is very poorly named. "contact" would be better, as you are iterating over entries in a map of contacts.

Unnecessary code

In this code, the else continue is pointless, you can safely delete it:

while (true) {
    if (kbhit()) {
        // ...
    } else continue;
}

Unnecessary evaluations

In the if condition inside this loop, the expression !key.empty() is evaluated in every iteration. This is pointless, because the value of key doesn't change inside the loop.

for (map<string,int>::iterator &tter = contacts.begin();tter != contacts.end();tter++) {
    if (string::npos == tter->first.find(key) && !key.empty()){
        continue;

It would be better to move this condition outside of the loop, like this:

if (!key.empty()) {
    for (map<string,int>::iterator &tter = contacts.begin();tter != contacts.end();tter++) {
        if (string::npos == tter->first.find(key)) {
            continue;
\$\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.