Is excessive use of this in C++ a code smell

Question

I'm dealing with a large code base that uses the following construct throughout

class MyClass
{
public:
  void f(int x);
private:
  int x;
};


void MyClass::f(int x)
{
'
'
  this->x = x;
'
'
}

Personally, I'd always used and hence prefer the form

class MyClass
{
public:
  void f(int x);
private:
  int _x;
};


void MyClass::f(int x)
{
'
'
  _x = x;
'
'
}

The reasons I prefer the latter are that it is more succinct (less code = fewer potential bugs), and that I don't like having multiple variables of the same name in scope at the same time where I can avoid it. That said, I am seeing the former usage more and more often these days. Is there any upside to second approach that I am unaware of? (e.g. effect on compile time, use with templated code, etc...) Are the advantages of either approach significant enough merit a refactor to the other? Reason I ask, that while I don't like the second approach present in the code, the amount of effort and associated risk of introducing further bugs don't quite merit a refactor.

Answer 1

I agree. I don't like that naming convention - I prefer one where there is an obvious distinction between member variables and local variables. What happens if you leave off the this?

Answer 2

I always use the m_ naming convention. Although I dislike "Hungarian notation" in general, I find it very useful to see very clearly if I'm working with class member data. Also, I found using 2 identical variable names in the same scope too error prone.

Answer 3

class MyClass{
public:  
  int x;  
  void f(int xval);
};
//
void MyClass::f(int xval){  
  x = xval;
}

Answer 4

In my opinion this tends to add clutter to the code, so I tend to use different variable names (depending on the convention, it might be an underscore, m*, whatever).

Answer 5

This usage of 'this' is encouraged by Microsoft C# coding standards. It gives a good code clarity, and is intended to be a standard over the usage of m_ or _ or anything else in member variables.

Honestly, I really dislike underscore in names anyway, I used to prefix all my members by a single 'm'.

Answer 6

Your version is a bit cleaner, but while you're at it, I would:

1. Avoid leading underscore: _x is ok until somebody chooses _MyField which is a reserved name.
2. Make the attribute private or protected: the change is safe if it compiles, and you'll ensure your setter will be used.
3. The this-> story has a use, for example in templated code to make the field name dependent on your type (can solve some lookup issues).

A small example of name resolutions which are fixed by using an explicit this-> (tested with g++ 3.4.3):

#include <iostream>
#include <ostream>

class A
{
public:
  int g_;
  A() : g_(1) {}
  const char* f() { return __FUNCTION__; }
};

const char* f() { return __FUNCTION__; }
int g_ = -1;

template < typename Base >
struct Derived : public Base
{
  void print_conflicts()
  {
    std::cout << f() << std::endl; // Calls ::f()
    std::cout << this->f() << std::endl; // Calls A::f()
    std::cout << g_ << std::endl; // Prints global g_
    std::cout << this->g_ << std::endl; // Prints A::g_
  }
};

int main(int argc, char* argv[])
{
   Derived< A >().print_conflicts();
   return EXIT_SUCCESS;
}

Answer 7

Field naming has nothing to do with a codesmell. As Neil said, field visibility is the only codesmell here.

There are various articles regarding naming conventions in C++:

• http://stackoverflow.com/questions/589535/naming-convention-for-public-and-private-variable
• http://stackoverflow.com/questions/383850/private-method-naming-convention
• http://stackoverflow.com/questions/603378/c-namespace-usage-and-naming-rules

etc.

Answer 8

A lot of people use this because in their IDE it will make a list of identifiers of the current class pop up.

I know I do in BCB.

I think the example you provide with the naming conflict is an exception. In Delphi though, style guidelines use a prefix (usually "a") for parameters to avoid exactly this.

Answer 9

class MyClass
{
public:
  int m_x;
  void f(int p_x);
};


void MyClass::f(int p_x)
{
  m_x = p_x;
}

...is my preferred way using scope prefixes. m_ for member, p_ for parameter (some use a_ for argument instead), g_ for global and sometimes l_ for local if it helps readability.

If you have two variables that deserve the same name then this can help a lot and avoids having to make up some random variation on its meaning just to avoid redefinition. Or even worse, the dreaded 'x2, x3, x4, etc'...

Answer 10

If you have a problem with naming conventions you can try to use something like the folowing.

class tea
{
public:
    int cup;
    int spoon;
    tea(int cups, int spoons);
};

or

class tea
{
public:
    int cup;
    int spoon;
    tea(int drink, int sugar);
};

I think you got the idea. It's basically naming the variables different but "same" in the logical sense. I hope it helps.

Answer 11

Using 'this' in this manner is IMO not a code smell, but is simply a personal preference. It is therefore not as important as consistency with the rest of the code in the system. If this code is inconsistent you could change it to match the other code. If by changing it you will introduce inconsistency with the majority of the rest of the code, that is very bad and I would leave it alone.

You don't want to ever get into a position of playing code tennis where somebody changes something purely to make it look "nice" only for somebody else to come along later with different tastes who then changes it back.

Answer 12

I don't like using "this" because it's atavistic. If you're programming in good old C (remember C?), and you want to mimic some of the characteristics of OOP, you create a struct with several members (these are analogous to the properties of your object) and you create a set of functions which all take a pointer to that struct as their first argument (these are analogous to the methods of that object).

(I think this typedef syntax is correct but it's been a while...)

typedef struct _myclass
{
   int _x;
} MyClass;

void f(MyClass this, int x)
{
   this->_x = x;
}

In fact I believe older C++ compilers would actually compile your code to the above form and then just pass it to the C compiler. In other words -- C++ to some extent was just syntactic sugar. So I'm not sure why anyone would want to program in C++ and go back to explicitly using "this" in code -- maybe it's "syntactic Nutrisweet"

Answer 13

My personal feeling is that fighting an existing coding convention is something you should not do. As Sutter/Alexandrescu puts it in their book 'C++ coding conventions': don't sweat the small stuff. Anyone is able to read the one or the other, whether there is a leading 'this->' or '_' or whatever.

However, consistency in naming conventions is something you typically do want, so sticking to one convention at some scope (at least file scope, ideally the entire code base, of course) is considered good practice. You mentioned that this style is used throughout a larger code base, so I think retrofitting another convention would be rather a bad idea.

If you, after all, find there is a good reason for changing it, don't do it manually. In the best case, your IDE supports these kind of 'refactorings'. Otherwise, write a script for changing it. Search & replace should be the last option. In any case, you should have a backup (source control) and some kind of automated test facility. Otherwise you won't have fun with it.