c++ vector of strings - issue with pointers

Question

I'm trying to write a method that gets a vector of strings and returns a pointer to a random element. Can you please tell what is the problem with the following code?

string* getRandomOption(vector<string> currOptions){
    vector<string>::iterator it;
    it=currOptions.begin(); 
    string* res;

    int nOptions = currOptions.size();

    if(nOptions != 1){
        int idx = rand() % (nOptions-1);

        while (idx!=0){
            it++;
            idx--;
        };

    };


    res = &(*it);
};

Thanks, Li

As Guillaume correctly said, your problem is that you chose the wrong way to pass the vector to the function. See [this answer](http://stackoverflow.com/questions/2139224/how-to-pass-objects-to-functions-in-c/2139254#2139254) for how to pass objects to functions in C++. — sbi, Sep 13 '10 at 15:18

score 10 · Accepted Answer · answered Sep 13 '10 at 14:59

10

Why return a pointer? Keep it simple!

std::string random_option(const std::vector<std::string>& options)
{
    return options[rand() % options.size()];
}

And since this works for any type, not only strings, I would prefer a generic solution:

template <typename T>
T random_element(const std::vector<T>& options)
{
    return options[rand() % options.size()];
}

answered Sep 13 '10 at 14:59

fredoverflow

256,549
94
388
662

+1 -- but see my answer for the same but with iterators. You could simply implement the vector version of this in terms of the iterator version. – Billy ONeal Sep 13 '10 at 16:46
1

This doesn't really answer the question, does it? – sellibitze Sep 13 '10 at 17:53
@sellibitze: Not really. OTOH it is the "right thing to do". @FredOverflow: What happens when `options.size()` is zero ;) – Billy ONeal Sep 13 '10 at 18:04
@Billy: How often do you have to randomly chose between zero options? :) – fredoverflow Sep 13 '10 at 18:09
Never, but that doesn't mean the function should explode (or rather enter UB). (Throwing an exception would be okay...) – Billy ONeal Sep 13 '10 at 18:21

score 5 · Answer 2 · answered Sep 13 '10 at 14:58

5

If you want to "returns a pointer to a random element" you need to pass a reference to your vector. For now, it is copied !

You should do :

string* getRandomOption(vector<string> & currOptions)

By the way there is no return in your function for the moment, you need to add a return statement to send your pointer.

answered Sep 13 '10 at 14:58

Guillaume Lebourgeois

3,796
1
20
23

That should probably be a const reference seeing as currOptions is not modified. – Billy ONeal Sep 13 '10 at 16:45
+1 @Billy: The OP wanted a string* as result. So, you need a non-const ref. In my opinion this answer could explain a little the problem with respect to dangling pointers. – sellibitze Sep 13 '10 at 17:54

score 4 · Answer 3 · answered Sep 13 '10 at 14:59

4

You are passing the vector by value, i.e. the function has a local copy of the original vector. Then you [intend to] return a pointer to an element in this vector. But when you return from the function, this local copy is destroyed, and your pointer is left dangling.

answered Sep 13 '10 at 14:59

AngryWhenHungry

458
2
15

+1 You actually answered the question and explained the problem (unlike others) – sellibitze Sep 13 '10 at 17:59

Billy ONeal · Answer 4 · 2013-04-30T18:13:11.487

Better version of the same, because it works with any container, rather than vectors only. Here's the C++03 version:

template <typename ForwardIterator>
ForwardIterator random_element(ForwardIterator begin, ForwardIterator end)
{
    typename std::iterator_traits<ForwardIterator>::difference_type
        size = std::distance(begin, end);
    if (size) //divide by zero errors are bad
        std::advance(begin, std::rand() % size);
    return begin;
}

If you're on C++11, you can replace the above with this:

template <typename ForwardIterator>
ForwardIterator random_element(ForwardIterator begin, ForwardIterator end)
{
    auto size = std::distance(begin, end);
    if (size) //divide by zero errors are bad
        std::advance(begin, std::rand() % size);
    return begin;
}

Which gets around the std::iterator_traits<t>::difference_type glue.

+1as it demonstrates some good practice and was downvoted without a reason. — Baiyan Huang, Sep 13 '10 at 16:28

Baiyan Huang · Answer 5 · 2010-09-13T15:22:10.800

1

maybe you want to change your function prototype to:

const string* getRandomOption(const vector<string>& currOptions)

or

string* getRandomOption(vector<string>& currOptions)

or else you just get an element from a temporarily copy.

edited Sep 13 '10 at 15:22

answered Sep 13 '10 at 14:58

Baiyan Huang

6,463
8
45
72

3

You cannot get a pointer to a mutable string from an immutable vector of strings. – fredoverflow Sep 13 '10 at 15:08

score 1 · Answer 6 · answered Sep 13 '10 at 15:36

1

Aside from the other problems mentioned in the other answers, when a vector resizes, any pointers or iterators (or references of any nature) become invalid. Don't return a pointer.

answered Sep 13 '10 at 15:36

Puppy

144,682
38
256
465

1

+1 because some prick downvoted all of us without commenting. – Billy ONeal Sep 13 '10 at 16:03

c++ vector of strings - issue with pointers

6 Answers6