using std::find on a container of a user-defined type c++

Question

Im trying to write a search function to get an element in std::list by suing std:find. But im stuck in the third parameter in the find argorithm, regard to this guy How to search for an element in an stl list? I did overload the operator == pretty much but it seems still not working with the std::find.

This is my code:

class Node
{
    string word;
    int count;

    public:
        Node(string _word) :word(_word), count(0) {}
        ~Node() {}

        const string& getWord() const{
            return word;
        }
        bool operator == (const Node& other) const {
            return (this->word.compare(other.word) == 0);
        }
};
const Node &getNode(const list<Node> &nodes, const string &word){
    list<Node>::iterator itr;
    itr = find(nodes.begin(), nodes.end(), new Node(word)); <-- no viable overload '='
    return *itr;
}

I'm going very crazy with that issue now, please suggest me some hints. Thanks

You have a list of `Node`s, but you're trying to find a pointer. Erase the `new`. — chris, Apr 22 '12 at 18:22
What do mean erase the new. If like this Node node(word); itr = find(nodes.begin(), nodes.end(), node); It still doesnt work — Harry, Apr 22 '12 at 18:28
@anonymous Please, check this. It works. You can also pass in the `string` directly. — pmr, Apr 22 '12 at 18:32

pmr · Accepted Answer · 2012-04-22T18:40:51.150

1

To get the code working just remove the new from the sort call. However, this is not going to make your code any better.

You don't check if the element was actually found and just dereference the iterator. If the element wasn't found, this is undefined behavior.

Now, how to fix this.

Don't provide this function. If the user of Node has a list, she should be perfectly capable of calling std::sort himself.
You don't want to write the boiler-plate of wrapping and you don't need to. Your class is convertible from std::string due to the single argument constructor taking a string (this should take a string by reference, though). So you can just write std::find(begin(nodes), end(nodes), "foobar");.
You can also mark the constructor explicit (the converting behavior is not wanted most of the time) and then just add two free operator==(const Node&, const std::string&) and operator==(const std::string&, const Node&).

In any case. Remove using namespace std; from your headers.

edited Apr 22 '12 at 18:40

answered Apr 22 '12 at 18:31

pmr

58,701
10
113
156

1

Still a good idea to make your constructor explicit unless you need otherwise. – chris Apr 22 '12 at 18:34
@chris Non-explicit can be helpful at times, but it's really delicate. Beginners should certainly avoid it. – pmr Apr 22 '12 at 18:38
Indeed, unless you have a good reason not to (like int->BigInt), avoiding it leads to less weird errors. – chris Apr 22 '12 at 18:40
Can you tell me the reason why we should remove using namespace std; from header – Harry Apr 22 '12 at 19:12
@anonymous It imports a lot of names into the current scope and makes for a lot of funny ambiguities should you ever try to call something `count`. Don't do it. http://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-a-bad-practice-in-c – pmr Apr 22 '12 at 19:45

score 1 · Answer 2 · answered Apr 22 '12 at 18:45

You have two main problems:

First, your find call is looking for a pointer to a Node. new allocates memory and returns a pointer. What you want is that exact text without new.
```
itr = find(nodes.begin(), nodes.end(), /*new*/ Node(word));
```
Also note that you can use just word instead because you provide a constructor with one string argument, so it will be implicitly converted. This is usually more bad than good though, and you'd be best to declare your constructor as explicit.
```
explicit Node(string _word) :word(_word), count(0) {} //need Node("hi")
```
This will lead to less confusing errors in the future. It's a good idea to stick explicit on by default.

Secondly, your main problem. Your function returns a const string &. Your iterator is of type list<Node>::iterator. These don't match.
```
return *itr; //not const
```
What you need is this:
```
list<Node>::const_iterator itr; //create a constant iterator instead
```
The rest can stay the same and it should work (or at least it did for me).

using std::find on a container of a user-defined type c++

2 Answers2