Keeping in mind that it is impossible to give good answers, or a good review, without a clear context of what the code is for or how it is going to be used. A single member function—not even a full class!—isolated from the rest of the program it is going to be used for is not really much context.
Use the standard library
Look up std::ranges::find_if(). (Or, if you prefer the old-fashioned way, std::find_if().)
This is your code:
std::unique_ptr<Contact> ContactManager::searchContact(const std::string& searchKey) {
for (const auto& c : contacts)
{
if (searchKey == c.getFirstName()
|| searchKey == c.getLastName()
|| searchKey == c.getPhone()
|| searchKey == c.getEmail())
{
return std::make_unique<Contact>(c);
}
}
return nullptr;
}
This is your code on std:
std::unique_ptr<Contact> ContactManager::searchContact(const std::string& searchKey)
{
auto const it = std::ranges::find_if(contacts,
[&searchKey] (auto&& c)
{
return (searchKey == c.getFirstName()
|| searchKey == c.getLastName()
|| searchKey == c.getPhone()
|| searchKey == c.getEmail());
}
);
return (it == contacts.end()) ? nullptr : std::make_unique<Contact>(*it);
}
As you can see, it is almost exactly the same, except the version using the standard algorithm is:
- … more explicit (it clearly says it is doing a “find” operation
- … better tested (betting you didn’t even bother to test your version); and
- … more powerful.
The last point refers to the fact that standard algorithms will come with built-in support for parallel searching. Even today the old-school find_if supports execution policies like std::execution::parallel_policy and std::execution::parallel_unsequenced_policy. The more modern range version does not… but that is because it is waiting on an even more powerful tool.
Also, if you later decide that it would be possible to do a much faster binary search, you can trivially replace find_if() with lower_bound(). Rewriting a hand-written linear find loop into a binary search? Much less easy.
No, really, use the standard library
There is no reason the search key has to be a std::string. Consider if I wrote this:
contact_manager.searchContact("Biff");
"Biff" is a char const[5], which probably degrades to a char const*. But because the search function takes a std::string, that means a whole std::string has to be constructed just to do a search. What a waste! (Granted, in this case, "Biff" is short enough that it will almost certainly fall under the short-string optimization… but a longer search string would require allocation.)
And the worst part is, the search itself will work just fine without a std::string. Assuming c.getFirstName() et al return std::strings (something that I have to guess, because you haven’t included the relevant code), then searchKey == c.getFirstName() will work even if searchKey is just a char const*. No std::string is needed.
This is what std::string_view was born for.
If your function looked like this:
// This is the only change ----------------------------vvvvvvvvvvvvvvvv
std::unique_ptr<Contact> ContactManager::searchContact(std::string_view searchKey) {
for (const auto& c : contacts)
{
if (searchKey == c.getFirstName()
|| searchKey == c.getLastName()
|| searchKey == c.getPhone()
|| searchKey == c.getEmail())
{
return std::make_unique<Contact>(c);
}
}
return nullptr;
}
Then I could do this:
// Call it with a character array pointer:
contact_manager.searchContact("Biff");
// Call it with a string:
contact_manager.searchContact("Biff"s);
// Call it with a string view:
contact_manager.searchContact("Biff"sv);
// Even call it with a 3rd party library string type (so long as it
// supports implicit conversion to std::string_view):
contact_manager.searchContact(ThirdPartyStringType{"Biff"});
And in all cases, no std::strings are constructed, and no allocation is done.
Questions
❓ “Is returning the contact object as a pointer(std::uniqu_ptr<Contact>) a good idea or is it more optimized to return the index of the contact and then print with that index?”
❗ Returning a unique_ptr is almost certainly wrong. Returning an index is also wrong.
Let’s start with unique_ptr.
The problem with unique_ptr is that it requires creating a copy of the contact, because unique_ptr has to hold a unique object (hence the name), and you can’t give it the object that is in the contact manager… thus, a copy is required.
But that’s silly. When someone asks you to find an item, you don’t go out, locate the item, and then reproduce an exact copy of it to hand over to the person. You do one of two things:
- you tell them the location (address) of the item, so they can go get it themselves if they want it; or
- you take (move) the item from its location, and hand it over to the person.
So for any search function, there are really only two sensible options:
- return a pointer or reference to the item; or
- move the item out of wherever it is being held, and return it to the user.
In practice, the first option is the better one, because if the user only wants to know if the item exists, or they just want to query it somehow, they can do so… but even if the user then wants to actually take the item, they themselves can then move it out of the location (using some other part of the interface). That leaves the decision in the hands of the user.
So there is your answer: the correct thing to return from a search function—from any search function, in general—is a pointer or reference to the item.
If your search function cannot possibly fail, then a reference is the best choice. If it can fail, then what you would want, ideally, is a std::optional<Contact&>. Unfortunately, std::optional does not support references (though it may in the future, and note the proposal actually considers your exact use case). The “classic” way to do an “optional reference” is… a pointer. Not a unique_ptr. Not a smart pointer (though something like std::experimental::observer_ptr would work). Just a bare pointer. It’s not great, but it works.
(You could go all-in and make a proper return type, one that wraps a pointer, but does not require checking for nullptr, and does not trigger UB if you forget to do that check. Not only that, you can add an ultra-modern monadic interface, like:
contacts.searchContact("Biff")
.andThen([] (auto&& contact) { sendMessageTo(contact); })
.orElse(printContactNotFoundError)
;
That’s quite a bit of extra work, but it can be very much worth it.)
So, your function should really be:
auto ContactManager::searchContact(std::string_view searchKey) -> Contact*
{
auto const it = std::ranges::find_if(contacts,
[&searchKey] (auto&& c)
{
return (searchKey == c.getFirstName()
|| searchKey == c.getLastName()
|| searchKey == c.getPhone()
|| searchKey == c.getEmail());
}
);
return (it == contacts.end()) ? nullptr : std::address_of(*it);
}
Now, what about returning an index?
The problem with returning an index is that the index is useless unless you also return the thing the index is for.
In other words, suppose searchContact("Biff") returns 2. Well… so what? I mean, at least you know “Biff” is in there. But to actually get more useful information, you need to know what 2 refers to. That means you need to expose the internal data, and allow users to access contacts by index.
So, again, the correct thing to return from any search function is a reference (or pointer, or iterator; I mean a “reference” as a general concept, not specifically a C++ reference). Specifically, an optional (or “nullable”) reference (again, I am speaking in general concepts, not specific stuff, so I don’t mean optional<T&> specifically).
As for what kind of (optional) reference you should return specifically, that depends a lot on context… and you have given no context, so I can’t give you a good answer. A raw pointer would work… but is not really a great option. There are better options (an iterator, maybe, if your contact list supports a range interface), but I can’t guess what would work for your case without context.
❓ “Second thing is about code logic. Is matching any property of the contact(firstName,lastName,phone,email) with const std::string& searchKey good or some easy and more optimized logic can be implemented here?”
❗ There are a lot of ways to answer this question. It is impossible to guess what kind of answer you want.
One way would be to say: No, matching with const std::string& searchKey is not good. Use std::string_view. That will be much more optimized, because it will not require constructing strings for the search key.
But another way would be to suggest that you are creating a problem you don’t need. Instead of worrying about whether comparing a search key to first-name-last-name-phone-and-email makes sense, how about you just… don’t.
Here’s what I mean:
template <std::predicate<Contact const&> F>
auto ContactManager::searchContact(F&& func) -> Contact*
{
auto const it = std::ranges::find_if(contacts, std::forward<Func>(func));
return (it == contacts.end()) ? nullptr : std::address_of(*it);
}
Yup. That’s it.
Now, if I want to search for “Biff” by first-name-last-name-phone-and-email, I can do:
auto result = contacts.searchContact([] (auto&& c)
{
return ("Biff" == c.getFirstName()
|| "Biff" == c.getLastName()
|| "Biff" == c.getPhone()
|| "Biff" == c.getEmail());
});
If I want to generalize that, I can do:
auto const by_matching_anything_to = [](std::string_view key)
{
return [key] (auto&& c)
{
return (key == c.getFirstName()
|| key == c.getLastName()
|| key == c.getPhone()
|| key == c.getEmail());
};
};
auto result_1 = contacts.searchContact(by_matching_anything_to("Biff"));
auto result_2 = contacts.searchContact(by_matching_anything_to("Doc"));
Okay, but what if I know the contact I’m looking for has last name “McFly” or “MacFly”, and their phone number contains the digits “42”? No problem!
auto result = contacts.searchContacts([] (auto&& contact)
{
return (contact.getLastName() == "McFly"
or contact.getLastName() == "MacFly")
and contact.getPhone().contains("42");
});
See what I’m getting at? Don’t stress over whether your search logic is sound. Make it the user’s problem. Oh, sure, you can be nice and provide some default search tools. But give the user the freedom to design their own custom search strategies, and now you no longer have to worry about whether you have covered every conceivable situation.
Summary
If I were writing this code, this is roughly how I would do it:
class ContactManager
{
public:
// A proper search result type, that prevents UB if you fail
// to check the result (perhaps by throwing an exception),
// and provides monadic ops.
//
// But, for a first, quick-and-dirty pass, this could just be:
// using search_result_t = Contact*;
// using const_search_result_t = Contact const*;
//
// Note that these need to be two distinct types for const-
// correctness. And the non-const version needs to be implicitly-
// convertible to the const version. But there are tricks to
// avoid having to write basically the same type twice.
class search_result_t;
class const_search_result_t;
// ... etc. ...
};
// The const version, returns a const_search_result_t, which allows
// users to *view* the contact, but not change it.
template <std::predicate<Contact const&> Pred>
constexpr auto ContactManager::searchContact(Pred&& pred) const& noexcept
-> const_search_result_t
{
if (auto const it = std::ranges::find_if(contacts, std::forward<Pred>(pred)); it != std::ranges::end(contacts))
return const_search_result_t{it};
else
return const_search_result_t{};
}
// The non-const version
template <std::predicate<Contact const&> Pred>
constexpr auto ContactManager::searchContact(Pred&& pred) & noexcept
-> search_result_t
{
if (auto const it = std::ranges::find_if(contacts, std::forward<Pred>(pred)); it != std::ranges::end(contacts))
return search_result_t{it};
else
return search_result_t{};
}
That is:
- The return type is an optional reference. (Which could basically be a pointer, if you’re lazy, but a proper result type would be better.)
- The search logic is externalized, and instead provided by the user. You can provide ready-made search logic to make things simpler for the user, if you like. But always give the user the ability to search the way they want.
const correct. If you try to search a const contacts manager, you will only a get a const view as a result, which won’t let you change anything. But if you search a non-const contacts manager, you have the option of changing things.- One thing I haven’t commented on is the possibility of returning multiple results. You will have to completely change your interface, but do consider that a search won’t necessarily return a single result.
Happy coding!
