views:

430

answers:

2

Hi. Im working with a cstring function that is supposed to compare the values of two strings, MyString and m2. I have #include so its definitely not that. these are my errors.

(74) : error C2275: 'MyString' : illegal use of this type as an expression
(74) : error C2660: 'MyString::length' : function does not take 1 arguments
(76) : error C2275: 'MyString' : illegal use of this type as an expression

and this is the code that I am getting them from.

bool MyString::operator ==(const MyString &m2) const
{
    int temp = 0;

    if (length(MyString) = length(m2)) // 74
    {
        if (strcmp(MyString, m2))  // 76
        {
            temp = 1;
        }
        else
        {
            temp = 2;
        }
    }
    if(temp = 2)
    {
        return "true";
    }
    else
    {
        return "false";
    }
}

Any help on this would be appreciated, thanks.

+9  A: 

Problems:

  • Return true or false, not "true" or "false"
  • You are doing compares with = instead of == (You use = for comparison twice in your code)
  • To refer to yourself in a C++ class, you need to use the keyword this not the class name.
  • Line 74 should be:

    if (length() == m2.length()) // 74
    
  • strcmp takes a char* not a MyString.
  • Line 76 should be:

    if (strcmp(this->c_str(), m2.c_str()))  // 76
    

In line 76 this assumes that the type MyString has a function c_str() that returns a pointer to a char[] buffer that is zero terminated.


Structure of function:

The structure of the function is really bad. Consider something more like this:

bool MyString::operator ==(const MyString &m2) const
{
    if(this->length() != m2.length())
      return false;

    return !strcmp(this->c_str(), m2.c_str()));
}

Note: In the above functions this-> can be omitted.

Brian R. Bondy
Don't you think MyString has 'operator char*'?
Mykola Golubyev
If it did, I'd count that as one of the problems. Implicit conversions are generally best avoided
jalf
Arkadiy
@Mykola Golubyev if that was the case he probably would have used strlen above as well.
Brian R. Bondy
+2  A: 

Most of the problems have already been addressed. I just have a suggestion that has not been written above:

Use the free function form of the comparison operator instead of the member function:

bool operator == (MyString const &, MyString const &);

You will have to declare it as a friend if it depends on private data/members but you will get symmetry for the callers. Assuming that (as with std::string) you do have a implicit conversion defined from const char * to your string then the member function implementation of == is not symmetric. Member functions require the left hand side to be of the required type. The compiler will not perform conversions on the left hand side of the comparison:

// assumes MyString( const char* ) is defined and not explicit
// operator== defined as member function

const char* literal = "hola";
MyString str( "hola" );

if ( str == literal ) {} // correct
if ( literal == str ) {} // compilation error

If you implement as a member function, in the first test the compiler will create an unnamed MyString and call the conversion operator. In the second check, the compiler is not allowed to convert literal into a MyString, so it will never find your operator== implementation.

If you provide the comparison as a free function then the compiler will apply the same conversion rules on both sides of the == and the code will compile and work properly.

In general, the same applies to the rest of the operators (excluding operator[] and operator= that must be implemented as member functions). Using the free function version provides symmetry.

David Rodríguez - dribeas