Discuss this snippet #1

[moorecm]

I thought it might be interesting to hold a discussion from time to time about a given code snippet. Things that some may find enlightening and others can share their views, critiques, and recommendations. This will be my first thread of this type but if it seems like a good idea, I might follow up with more every so often. Feel free to join in the discussion. Here goes!

Discuss:

std::string createKey( const Product & p ) {
    std::string key( "" + p.getId() + p.getName() + p.getCategory() );
    return key;
}

class Product {
    int id_;
    std::string name_;
    std::string category_;
public:
    int getId()               const { return id_; }
    std::string getName()     const { return name_; }
    std::string getCategory() const { return category_; }
    //...
};

EDIT: added const to getters so that it compiles.

[jdkunk]

Strings should not be passed with a copy. Instead pass them back as a const reference and let the user of the class decide whether or not they want/need them to be copied.

 const std::string & getName()     { return name_; }
 const std::string & getCategory() { return category_; }

This snippet fails to compile because p.getId() is an integer, and integers cannot be concated to a string in this fasion. Instead use std::stringstream str; str<<p.getId()<<p.getName()<<p.getCatagory(); return str.str();

Thanks,
Jeff Kunkel

[moorecm]

jdkunk wrote:

This snippet fails to compile because p.getId() is an integer, and integers cannot be concated to a string in this fasion. Instead use std::stringstream str; str<<p.getId()<<p.getName()<<p.getCatagory(); return str.str();

Ah...but it DOES compile!

[650 bash chad@asdf discussion1]$ ls
main.cpp
[650 bash chad@asdf discussion1]$ cat main.cpp 
#include <iostream>
#include <string>

class Product {
    int id_;
    std::string name_;
    std::string category_;
public:
    int getId()               const { return id_; }
    std::string getName()     const { return name_; }
    std::string getCategory() const { return category_; }
    //...
    Product():id_(10),name_("name"),category_("category") {}
};

std::string createKey( const Product & p ) {
    std::string key( "" + p.getId() + p.getName() + p.getCategory() );
    return key;
}

int main() {
    std::cout << createKey( Product() ) << std::endl;
    return 0;
}
[650 bash chad@asdf discussion1]$ g++ main.cpp
[651 bash chad@asdf discussion1]$ ./a.out 

namecategory
[652 bash chad@asdf discussion1]$ g++ --version
g++ (GCC) 4.1.1
Copyright (C) 2006 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[653 bash chad@asdf discussion1]$ 

[LB]

Are you sure it isn't an extension/mod/plugin for your compiler that allows that?

[firedraco]

No, it's just the int is being cast to a char which can be append to the string.

Which is probably not the intent anyway.

[LB]

But ... implicitly casting an int to a char generally gives a 'loss of precision' warning...does it not?

[hamsterman]

Maybe int is being cast to a char* and added to "" ?

[LB]

As far as I know, casting an integer to a pointer means that its memory address is the value of the integer, not a string representation of the integer or the integer itself...

char* ch = (char*)123;
ch points to a character at memory slot 123, but that could just be random memory!

[moorecm]

firedraco wrote:

No, it's just the int is being cast to a char which can be append to the string.

Is it? Let's try an 'A':

[732 bash chad@asdf discussion1]$ cat main.cpp 
#include <iostream>
#include <string>

class Product {
    int id_;
    std::string name_;
    std::string category_;
public:
    int getId()               const { return id_; }
    std::string getName()     const { return name_; }
    std::string getCategory() const { return category_; }
    //...
    Product():id_('A'),name_("name"),category_("category") {}
};

std::string createKey( const Product & p ) {
    std::string key( "" + p.getId() + p.getName() + p.getCategory() );
    return key;
}

int main() {
    std::cout << createKey( Product() ) << std::endl;
    return 0;
}
[731 bash chad@asdf discussion1]$ g++ main.cpp
[731 bash chad@asdf discussion1]$ ./a.out 
�namecategory
[731 bash chad@asdf discussion1]$ 

[firedraco]

Hm, I'm getting this:

int main() {
	std::string temp;
	for(int i = 'a'; i < 'z'; ++i) {
		temp += i;
	}
	std::cout<<temp;
}

abcdefghijklmnopqrstuvwxy

[moorecm]

Try this on line 4:
temp += "" + i;

[hamsterman]

As far as I know, casting an integer to a pointer means that its memory address is the value of the integer, not a string representation of the integer or the integer itself...

which is why there is no 10 in his output.

"" + p.getId() + p.getName() + p.getCategory()
We know that a + has the same precedence as +, so this is equal to
((("" + p.getId()) + p.getName()) + p.getCategory())
which is
( ( ( const char* + int ) + string ) + string )

[firedraco]

Yeah, thats the problem:

You get "Whatever the address of "" is in memory" + some integer offset. Which is garbage...In fact, I got a random debug message >_> "This operation will not affect the quality of the optimized code..." XD

[guestgulkan]

Which turns out to be a bit of dodgy code code because
"" + some_int_value could be pointing to garbage

[hamsterman]

It could, but then there was garbage when he changed 10 to 'A'.
See how much garbage there is yourself
for( int i = 0; i < 80*25-1 ; i++) std::cout << *(""+i);

[Duthomhas]

Avoid throw-away temporaries:

inline std::string createKey( const Product & p ) {
    return std::string( "" + p.getId() + p.getName() + p.getCategory() );
}

This gives your compiler all kinds of optimization options, and significant speed issues where relevant. (This assumes, of course, that the integer problem already discussed above was fixed.)

Of course, you could also cache the key if your user is asking for it a lot more than the constituent values get changed...

[guestgulkan]

On a purely personal preference thing, I prefer the mMember rather than _member or member_ naming scheme.

[LB]

I get an error when I try to compile this code:

string S;
int x = 33;
S += x; //Error
cout << S;

error C2679: binary '+=' : no operator found which takes a right-hand operand of type 'int' (or there is no acceptable conversion)

And it has always been this way for me. Why is everyone else able to do the impossible and add an int to to a string?

[guestgulkan]

You are correct std::string + int will give an error
BUT
You are mistaking what is happeing in the code in original post.
The original post says this:
"" + p.getId() + p.getName() + p.getCategory()
Remembering that "" is a char* - what we have is
char* + p.getId() + p.getName() + p.getCategory()

What happens is this:
1.
The compiler evaluates the functions - so you you will have
char* + int + string + string

2.
char* + int is standard pointer math - the pointer is just moved forward.
so char* + int still evaluates to a char pointer.
So the current result is
char* + string + string.

3.
There is a global overload in the standard library for char* + string to give a string result.
So what we have now is
string + string.

4.
string + string is a string


So it all evaluates to a string.

[LB]

Look at firedraco's post ( http://www.cplusplus.com/forum/general/40922/#msg220812 )
How does his compile at all?

EDIT: Nevermind, I changed some project settings in my compiler and now adding an int to a std::string does convert the int to a char and give no warning message or error. It works like with stringstreams, which is quite scary because I think this functionality should not exist...