16

I have a function where I have to modifiy the values of a vector. is it a good practice in C++ to return the vector?

Function 1:

vector<string> RemoveSpecialCharacters(vector<string> words)
{
    for (vector<string>::iterator it=words.begin(); it!=words.end(); )
    {
        if(CheckLength(*it) == false)
        {
            it = words.erase(it);
        }
        else{
            ++it;
        }
    }//end for

    return words;
}

Function 2:

void RemoveSpecialCharacters(vector<string> & words)
{
    for (vector<string>::iterator it=words.begin(); it!=words.end(); )
    {
        if(CheckLength(*it) == false)
        {
            it = words.erase(it);
        }
        else{
            ++it;
        }
    }//end for
}
Hani Goc
  • 2,371
  • 5
  • 45
  • 89
  • 4
    The examples do different things. One modifies a vector, the other returns a different vector. So which functionality do you actually need? – juanchopanza Oct 30 '13 at 09:33
  • It's always struck me as odd that copy elision *excludes* function parameters... – Kerrek SB Oct 30 '13 at 09:34
  • 1
    Function 1 is ultimately more flexible. You can say `v = f(v)` to get the behaviour of Function 2, but you can also say `v = f(g())`. – Kerrek SB Oct 30 '13 at 09:35
  • 1
    @juanchopanza I actually want to modify the values of the vector. As you can See I am erasing elements that have a specific length – Hani Goc Oct 30 '13 at 09:35
  • The question is, what is the purpose of the `BagOfWords` class, and why does the method `RemoveSpecialCharacters` get a vector of strings as input and you want it to return one. If vector of string represents your `BagOfWords`, why don't you directly manipulate it? – Dmitry Ledentsov Oct 30 '13 at 09:37
  • 1
    Well, the second version does what you want, the first one doesn't. – juanchopanza Oct 30 '13 at 09:39
  • @juanchopanza I fail to see why? – Spook Oct 30 '13 at 12:11
  • @Spook the first version creates a new vector. That is not the same as modifying an existing one. – juanchopanza Oct 30 '13 at 12:50

7 Answers7

6

Your two functions serve for two different purposes.

  • Function 1: works as remove_copy. It will not modify the existing container; it makes a copy and modifies that instead.

  • Function 2: works as remove. It will modify the existing container.

Sam
  • 7,252
  • 16
  • 46
  • 65
billz
  • 44,644
  • 9
  • 83
  • 100
  • For Function 1, typically you assign the returned value to another vector, thus another copy is made. – fchen Mar 28 '14 at 16:15
  • 1
    @fchen that copy is effectively eliminated by any modern c++ compiler and language is changing to support that, also for C++11 author should write `return std::move( words );` – Slava Dec 09 '15 at 16:03
3

It's somewhat subjective. I personally would prefer the latter, since it does not impose the cost of copying the vector onto the caller (but the caller is still free to make the copy if they so choose).

NPE
  • 486,780
  • 108
  • 951
  • 1,012
2

In this particular case I'd choose passing by reference, but not because it's a C++ practice or not, but because it actually makes more sense (the function by its name applies modification to the vector). There seems to be no actual need to return data from the function.

But that also depends on purpose of the function. If you always want to use it in the following way:

vec = bow.RemoveSpecialCharacters(vec);

Then absolutely first option is a go. Otherwise, the second one seems to be more appropriate. (Judging by the function name, the first one seems to me to be more appropriate).

In terms of performance, the first solution in modern C++11 world will be more-less a few assignments slower, so the performance impact would be negligible.

Spook
  • 25,318
  • 18
  • 90
  • 167
1

Go for option 2, modifying the vector passed as parameter.

Side note: some coding practices suggest to pass through pointers arguments that may be changed (just to make it clear at a first glance to the developers that the function may change the argument).

Claudio
  • 10,614
  • 4
  • 31
  • 71
  • Well, I think using pointers here is absolutely unnecesarry. Reference makes it equally as clear and may even be an optimization hint for a smart compiler. –  Oct 30 '13 at 09:34
  • @Claudio: That's more of a C legacy style. In C++ style, you indicate that you don't change a parameter by writing `const&`. – MSalters Oct 30 '13 at 10:42
1

The best practice is to pass by reference the vector.

Inside the function you edit it and you don't have to return it back (which in fact you are allocating a new memory space that is not needed).

If you pass it by reference the cost is much smaller and the vector is also edited out of the scope of the function

EoD
  • 357
  • 1
  • 3
  • 11
0

The first function will return a copy of the vector, so it will be slower than the second. You should use the second.

Pierre Emmanuel Lallemant
  • 2,571
  • 1
  • 13
  • 23
  • It depends on the actual needs. If there is a need for a copy, both function will perform more or less the same. – Spook Oct 30 '13 at 09:39
0

Actually neither one is a good practice for C++, if by good practice to mean how it is done in C++ libraries. You basically reimplement what std::remove_copy_if or std::remove_if does, so good practice is to implement functions (or use existing ones), that work on range, not on container, either by value or by reference.

Again this depends on how you define term good practice.

Slava
  • 43,454
  • 1
  • 47
  • 90