views:

2537

answers:

7

Background

I have a container class which uses vector<std::string> internally. I have provided a method AddChar(std::string) to this wrapper class which does a *push_back()* to the internal vector. In my code, I have to add multiple items to the container some time. For that I have to use

container.AddChar("First");
container.AddChar("Second");

This makes the code larger. So to make it more easier, I plan to overload operator <<. So that I can write

container << "First" << "Second"

and two items will get added to underlying vector.

Here is the code I used for that

class ExtendedVector
{
private:
    vector<string> container;

public:
    friend ExtendedVector& operator<<(ExtendedVector& cont,const std::string str){
     cont.AddChar(str);
     return cont;
    }

    void AddChar(const std::string str)
    {
     container.push_back(str);
    }

    string ToString()
    {
     string output;
     vector<string>::iterator it = container.begin();
     while(it != container.end())
     {
      output += *it;
      ++it;
     }
     return output;
    }
};

It is working as expected.

Questions

  1. Is operator overload written correctly?
  2. Is it a good practice to overload operators in situations like this?
  3. Will there be any performance issues or any other issues with this code?

Any thoughts?

Edit

After hearing the excellent comments, I decided not to overload << as it doesn't make sense here. I removed the operator overload code and here is the final code.

class ExtendedVector
{
private:
    vector<string> container;

public:

    ExtendedVector& AddChar(const std::string str)
    {
     container.push_back(str);
     return *this;
    }

         .. other methods
}

This allows me to add

container.AddChar("First").AddChar("Second")

In C#, I can do this more easily by using the params keyword. Code will be like

void AddChar(params string[] str)
{
    foreach(string s in str)
       // add to the underlying collection
}

I know in C++, we can use ... to specify variable langth of parameters. But AFAIK, it is not type safe. So is it a recommended practice to do so? So that I can write

container.AddChar("First","Second")

Thanks for the replies.

+3  A: 

Is it a good practice to overload operators in situations like this?

I don't think so. It's confusing as hell for someone who doesn't know that you've overloaded the operator. Just stick to descriptive method names and forget about the extra characters you're typing, its just not worth it. Your maintainer (or you yourself in 6 months) will thank you.

Kevin
I agree. Operator overloading can be cool, but when you're trying to maintain code months later, even if you wrote it, you want to be able to understand what it does with minimal effort. Clear code is better than cool code that is confusing.
unforgiven3
+3  A: 

1) Yes, except since AddChar is public there's no reason it needs to be a friend.

2) This is arguable. << is sort of in the position of being the operator whose overloading for "weird" things is at least grudgingly accepted.

3) Nothing obvious. As always, profiling is your friend. You may want to consider passing the string parameters to AddChar and operator<< by const reference (const std::string&) to avoid unnecessary copying.

Logan Capaldo
+1 for the std::string reference to avoid copying. And for everything else.
Thanks. But if I remove friend, compiler will throw error.
Appu
indeed, i gave you a +1 too. i especially like #1 . i'll put it clear though: "make it a non-member non-friend function" :) and i like #2 too. rather than saying "bad bad" you give basis for arguments :) and i think op<< is well accepted as an insertion-op.
Johannes Schaub - litb
+1  A: 

I'd prefer not to overload it that way personally because vectors don't normally have an overloaded left shift operator - it's not really it's idiom ;-)

I'd probably return a reference from AddChar instead like so:

ExtendedVector& AddChar(const std::string& str) {
    container.push_back(str);
    return *this;
}

so you can then do

container.AddChar("First").AddChar("Second");

which isn't really much bigger than bitshift operators.

(also see Logan's comment about passing strings in by reference instead of by value).

Peter
You beat me to this! By, uhm, a few minutes ... ;)
That's a good suggestion. Thanks.
Appu
It's not a vector though. It's a class which just so happens to store a vector internally. operator<< might make perfect sense. It's already standard for stream-like classes after all. Would need a bit more context to say for sure though. :)
jalf
Yeah agreed, technically it's not a vector. But my impression (which may be wrong, since we don't have the whole thing) is that it's only adding a few functions to it, so it very much looks like one. My suggestion's kind of a stylistic thing anyway, so other's mileage may vary :-)
Peter
+1  A: 

The operator is not correctly overloaded here. There is no reason to make the operator a friend since it can be a member of the class. Friend is for functions which are not actual members of the class (such as when overloading << for ostream so the object can be output to cout or ofstreams).

What you actually want the operator to be:

ExtendedVector& operator<<(const std::string str){
    AddChar(str);
    return *this;
}

It is usually considered bad practice to overload operators in a way that has them do something than they do normally. << is normally bit shift, so overloading it this way can be confusing. Obviously STL overloads << for "stream insertion" and so along with that it might make sense to overload it for your use in a similar way. But that doesn't seem like what you're doing, so you probably want to avoid it.

There are no performance issues since operator overloading is the same as a regular function call, just the call is hidden because it is done automatically by the compiler.

SoapBox
+2  A: 

The operator overloading in this case is not good practice as it makes the code less readable. The standard std::vector doesn't have it either for pushing elements, for good reasons.

If you're worried about the caller code being too long, you could consider this instead of the overloaded operator:

container.AddChar("First").AddChar("Second");

This will be possible if you have AddChar() return *this.

It's funny that you have this toString() function. In that case, an operator<< to output to a stream would be the standard thing to use instead! So if you want to use operators make the toString() function an operator<<.

Good suggestion. Thanks
Appu
This is at least as confusing and unexpected as the << semantics
Assaf Lavie
A: 

This is going to make things rather confusing, I would use the same syntax as std::cin into a variable:

std::cin >> someint;

"First" >> container;

This way it is at least an insertion operator. To me when anything has a << overloaded operator I expect it to be outputting something. Just like std::cout.

X-Istence
Hmm. I think of >> as extraction *from* the input stream to my objects!
Simon Buchan
I don't think there is a right answer, I don't believe the << nor the >> operator are right for what he is attempting to accomplish. Personally I would just keep the push_back mechanism that is already established as the "right" way to do things, extra typing is not a bad thing!
X-Istence
+4  A: 

Is operator overload written correctly?

It is, but one can do better. Like someone else mentioned, your function can be defined entirely out of existing, public functions. Why not make it use only those? Right now, it is a friend, which means it belongs to the implementation details. The same is true if you put operator<< as a member into your class. However, make your operator<< a non-member, non-friend function.

class ExtendedVector {
    ...
};

// note, now it is *entirely decoupled* from any private members!
ExtendedVector& operator<<(ExtendedVector& cont, const std::string& str){
    cont.AddChar(str);
    return cont;
}

If you change your class, you will not be sure that that your operator<< will still work. But if your operator<< entirely depends only on public functions, then you can be sure that it will work after changes were made to implementation details of your class only. Yay!

Is it a good practice to overload operators in situations like this?

As another guy said again, this is arguable. In many situations, operator overloading will look "neat" at first sight, but will look like hell next year, because you have no clue anymore what you had in mind when giving some symbols special love. In the case of operator<<, i think this is an OK use. Its use as an insertion operator for streams is well known. And i know of Qt and KDE applications that use it extensively in cases like

QStringList items; 
items << "item1" << "item2";

A similar case is boost.format which also reuses operator% for passing arguments for placeholders in its string:

format("hello %1%, i'm %2% y'old") % "benny" % 21

It's of course also arguable to use it there. But its use for printf format specifies are well known and so its use is OK there too, imho. But as always, style is also subjective so take it with a grain of salt :)

How can i accept variable length arguments in a typesafe way?

Well, there is the way of accepting a vector if you are looking for homogeneous arguments:

void AddChars(std::vector<std::string> const& v) {
    std::vector<std::string>::const_iterator cit =
        v.begin();
    for(;cit != v.begin(); ++cit) {
        AddChar(*cit);
    }
}

It's not really confortable to pass it though. You have to construct your vector manually and then pass... I see you already have the right feeling about the vararg style functions. One should not use them for this kind of code and only when interfacing with C code or debugging functions if at all. Another way to handle this case is to apply preprocessor programming. This is an advanced topic and is quite hacky. The idea is to automatically generate overloads up to some upper limit roughly like this:

#define GEN_OVERLOAD(X) \
void AddChars(GEN_ARGS(X, std::string arg)) { \
    /* now access arg0 ... arg(X-1) */ \
    /* AddChar(arg0); ... AddChar(arg(N-1)); */ \
    GEN_PRINT_ARG1(X, AddChar, arg) \
}

/* call macro with 0, 1, ..., 9 as argument
GEN_PRINT(10, GEN_OVERLOAD)

That is pseudo code. You can have a look at the boost preprocessor library here.

Next C++ version will offer far better possibilities. Initializer lists can be used:

void AddChars(initializer_list<std::string> ilist) {
    // range based for loop
    for(std::string const& s : ilist) {
        AddChar(s);
    }
}

...
AddChars({"hello", "you", "this is fun"});

It's also possible in next C++ to support arbitrary many (mixed-type) arguments using variadic templates. GCC4.4 will have support for them. GCC 4.3 already partially supports them.

Johannes Schaub - litb
As always great post. Thanks
Appu
i'm glad you like it
Johannes Schaub - litb