11

While working in a project with some legacy code i found this function:

std::vector<std::string> Object::getTypes(){
    static std::string types [] = {"type1","type2", "type3"};
    return std::vector<std::string> (types , types +2);
}

I would probably have written this as:

std::vector<std::string> Object::getTypes(){
    std::vector<std::string> types;
    types.push_back("type1");
    types.push_back("type2");
    types.push_back("type3");
    return types;
}

Is this merely a style choice or is there something I'm missing? Any help would be greatly appreciated. Sorry if this is too basic.

Update: Actually found different classes that override the same method do it one way or the other, so It's even more ambiguous. I would make them all the same but would prefer the better approach, if there is one.

Edit

Please note that the above legacy code is incorrect because it initializes the vector with only the first two elements of the array. However, this error has been discussed in the comments and thus should be preserved.

The correct initialization should have read as follows:

...
    return std::vector<std::string> (types, types + 3);
...
Improve this question
8
  • 2
    The first incantation could in principle be more efficient because it does not involve any memory re-allocations. There are also less function calls. Note in C++11 you can say return std::vector<std::string>{"type1","type2", "type3"};. Commented Dec 17, 2013 at 12:38
  • 2
    @juanchopanza Also, in that version, isolation of code and data is more apparent, so it's syntactically cleaner. Commented Dec 17, 2013 at 12:45
  • Correction to my previous comment: in C++11, you can achieve it in less words: return {"type1","type2", "type3"};. Commented Dec 17, 2013 at 12:48
  • 1
    The first version is more error-prone, as demonstrated by the off-by-one error here. Commented Dec 17, 2013 at 12:51
  • @MikeSeymour Not if you use std::begin and std::end (which you should, of course). Commented Dec 17, 2013 at 14:24

5 Answers 5

Reset to default
10

If you have a C++11 capable compiler and library, returning an initializer list should be enough:

std::vector<std::string> Object::getTypes(){
    return {"type1","type2", "type3"};
}
Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

5

The code you found is more efficient (because types[] is only allocated once and push_back can/will cause re-allocations). The difference though is marginal, and unless you call getTypes in a (relatively big) loop it shouldn't matter at all (and probably it won't matter much even when you do call it in a big loop).

As such, unless it creates a concrete performance problem, it's a style choice.

Improve this answer

Comments

3

Basically it's a style choice. I'd probably do something more like 

std::vector<std::string> Object::getTypes(){
    static std::string types [] = {"type1","type2", "type3"};
    return std::vector<std::string> (types, 
                   types + (sizeof(types)/sizeof(std::string)) );
}

which lets you change the number of things in types, without having to remember to update the count in the next line.

Improve this answer

5 Comments

This gives me an error if I try to do it like this, "No matching constructor for initialization of 'std::vector<std::string>'". Initially i thought that I should do it like you say, but it doesn't compile.
@AndresBucci you would need std::vector<std::string> (types , types + sizeof(types)/sizeof(std::string));
@AndresBucci - see edits, juanchopanza's comment beat my edit by a bit. Basically, vector doesn't have a (Type*, nelements) constructor, it's got a (iterator, iterator) constructor.
I wanted to select your answer as the accepted one, but since the one I selected contains an explanation I think it's gonna be better for the future readers.
What's wrong with using std::being( types ), std::end( types ) as the argument to the constructor of the vector? (If you don't have C++11, of course, you use the corresponding functions in your toolkit.)
2

The array types in the first example is declared static. This means it only exists once in memory. So there are three options for what to return and they live in static memory. Then, when you create the vector to return, you are able to allocate it's memory in one shot by passing the beginning and ending of the array as iterators.

By doing it this way, you don't have successive calls to push_back which means the vector won't have to reallocate its internal block of memory.

Also, when the vector is constructed as part of the return call, older compilers will have an easier time of doing return value optimization.

Improve this answer

1 Comment

RVO applies equally to all of the code. Even with "older" compilers (many of which actually did it better than some of the more recent compilers).
0

One reason I like to use this style of initialization with iterators (and C++11's uniform initialization and initializer lists) is that it helps separating data from code.

Repeating push_back a lot of times feels bad because I desperately need to refactor that. Also, when you really just need to initialize the container with data, then you want to see a list of the data, and not really code that generates data. The method you found in the original version matches that principle better.

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.