4
\$\begingroup\$

This is a solution for the Day 8 hackerrank.com challenge. The basic idea of the challenge is to create a mapping of names to phone numbers based on the the given input, and later look up the phone numbers for certain names. If the entry exists, print it and its number. If it doesn't, print out "Not found".

Here's my solution in C++:

#include<iostream>
#include<map>
using namespace std;
int main()
{
    int i, n;
    cin>>n;
    string name, number, key;
    map<string, string> phone_dir;
    for(i=0; i<n; i++)
    {
        cin>>name>>number;
        phone_dir.insert(pair <string, string> (name, number));
    }
    while(cin>>key)
    {
        if (phone_dir.find(key) != phone_dir.end())
        {
             cout<<key<<"="<<phone_dir.find(key)->second<< endl;
        }
        else cout<< "Not found"<<endl;
    }

    return 0;

}

Is there a more efficient solution in C++ for this problem?

\$\endgroup\$
7
\$\begingroup\$
  1. You should not use using namespace std; This is bad practice that will get you into trouble once you want to use a function that has the same name.

  2. You can use descriptive names. So instead of "n" use "numEntries" or whatever.

  3. You should declare the iteration variable inside the for loop rather than at the beginning.

  4. std::map has an emplace method that you should use rather than insert.

  5. You are using a std::map, which is is a red black tree. You most likely want a std::unordered_map

  6. You search twice for the key. With modern C++17 you can do the following:

    if (auto res = phone_dir.find(key); res != phone_dir.end())
    {
         std::cout << key << "=" << res->second << std::endl;
    } else {
         std::cout << "Not found" << std::endl;
    }
    
  7. You are using std::endl which involves a flush of the stream. You definitely only want a \n here.

  8. Whitespace is cheap. Grab some to improve readability

  9. Same for lines. It is bad practice to have braces on one condition and pu the other one on the same line. This severely affects readability

  10. return 0; is implied for main so you can skip that.

  11. I almost forgot, you should reserve the space in the map.

\$\endgroup\$
  • \$\begingroup\$ For #6, did you mean if ((auto res = phone_dir.find(key)) != phone_dir.end())? \$\endgroup\$ – Mike Borkland Jun 30 '18 at 15:31
  • \$\begingroup\$ @MikeBorkland Most people would consider that too tight - just write "auto res = ...; if (res != phone_dir.end()) { ... } You just want to avoid two calls to find(). On the other hand, that's only because you asked "is there a more efficient solution". The printing will take a lot longer than the finding, so your code is not too bad. \$\endgroup\$ – gnasher729 Jun 30 '18 at 15:51
  • \$\begingroup\$ @gnasher729 It's not my code... \$\endgroup\$ – Mike Borkland Jun 30 '18 at 16:06
  • \$\begingroup\$ @MikeBorkland ??? "For #6, did you mean..." followed presumably by your code, and followed by your name. \$\endgroup\$ – gnasher729 Jun 30 '18 at 17:34
  • \$\begingroup\$ I'm not a fan of implicitly depending on the tie feature between cin and cout, since it doesn't mean exactly what most people think it does. \$\endgroup\$ – Hurkyl Jun 30 '18 at 17:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.