This week's book giveaway is in the OCAJP 8 forum. We're giving away four copies of OCA Java SE 8 Programmer I Study Guide and have Edward Finegan & Robert Liguori on-line! See this thread for details.
I recieved a few code review comments from one of my designers. Found one of them interesting and am curious to have our forum members views on this.
The review comments says "...There is no justifiable reason for using this other than to pass a reference. The problem with using this is fairly straight forward: a double reference creates more work and impedes performance. this is a pointer to the object. this.something means that the JVM will have to go to the stack via the reference and retrieve the object and then reference the method provided. Since all properties and methods are visible through scope, this.something is creating more work for no reason. Hence the statement: do not use it. Some feel that this.property = incomingPropertyValue is a justifiable reason for using it since the names are the same. This is not justifiable: rename the incoming variable, do not use this.."
Probably I was over-using 'this' in my code. But is 'this' really that bad ?
No. In fact it sounds like your code reviewer has little or no idea what they're talking about. Consider the following:
If you disassemble this this with javap -c Test, you get:
From this we can see that setX1() and setX2() are exactly identical. The use of "this" in a standard setter method makes no difference at all.
It's possible that there are some other contexts in which using "this" might make some difference. But I think your code reviewer has a grossly exaggerated impression of how important this effect might be.
Now if we ignore performance issues, then there may be good reasons to discourage or even forbid people from using local variable names which duplicate field names. This sort of name duplication (hiding) can be very confusing if the reader does not notice it. For simple setter methods it's such a common idiom, and the methods are so extremely simple, that the chance of introducing a bug because of this is fairly small. Code like "this.x = x" is very common, and probably simple enough that it's not a significant risk of bugs. Still, I understand if someone thinks that it's best to always avoid name hiding. Such an attitude is perhaps a bit overly paranoid, but not completely unjustified.
However, if your code reviewer insists on citing performance as the reason for this rule - there's really no justification for this; the reviewer simply doesn't understand how Java works.
Of course, if this is someone you need to work with in the future, then you should be careful about how agressively you point this out. However in terms of your own understanding of how the code works: don't pay much attention to this reviewer in the future. They don't seem to be a reliable source of information. [ October 26, 2005: Message edited by: Jim Yingst ]
Sounds suspect to me. "Early optimizations" are always a bad thing. The way I understand it, the this keyword is a way of explicitly using a reference to the current object that exists anyway, its not a "double reference" (whatever that is), its the same reference. "all properties and methods are visible through scope" is true only because this is used implicitly. I would very much doubt you'll see any performance difference using it in this way or not. There is an easy way to find out for sure - write a couple of performance tests and see.
I would go further than Jim: if the other people you have to work with are as stupid as the "designer" who reviewed your code, I would look for another job where people don't speak out of their ssh le like him.
But in the meanwhile, you should humbly show this reviewer the byte code that Jim used to illustrate the point that there is no performance issue here. Show the byte code and quietly ask the reviewer to point out the place where performance is impeded, because you are having trouble seeing it, but he is so wise and insightful, he will be able to show you the byte code which proves "this" is bad. [ October 26, 2005: Message edited by: Jeff Albrechtsen ]
Full agreement with Jim, Paul and Jeff. Sounds like a learning opportunity for the reviewer. Don't be too hard with him - we all do mistakes...
Anyway, moving to our Performance forum...
The soul is dyed the color of its thoughts. Think only on those things that are in line with your principles and can bear the light of day. The content of your character is your choice. Day by day, what you do is who you become. Your integrity is your destiny - it is the light that guides your way. - Heraclitus
I am also in agreement that particular person had NO idea what he/she was talking about.
Furthermore, _always_ use 'this' in inner classes, even when calling methods. If you don't qualify every call to a class member in an inner class you can never be sure if its calling on the outerclass or the inner class heirachy.
I make my calls either OuterClass.this.method() or InnerClass.this.method(). One it makes it clear which method you are trying to call, two it ensures that if someone later adds a method to the super class of the inner class, your program won't suddenly break because the inner class is calling a different method now...Either that or don't use non-static inner classes.
Joined: Jul 11, 2001
Originally posted by Mr. C Lamont Gilbert: Either that or don't use non-static inner classes.
Or write unit tests for your classes...
Joined: Mar 04, 2005
Thank you very much for all the views.
And thanks Jim about letting me know 'how we ourselves can check for deciding on which way to go'.
Certainly I am not going to be hard on my reviewer. He has made many other good/valuable suggessions and I have accepted them and made changes. For this one, I found it little strange. So thought I might clarify for myself about this.
Alright, so what was your reviewer's reaction?? :roll:
I guess for a lot of developers in the industry, this forum is a useful forum (oops) to post such "controversial" points and get clarification.
That way, one can convince another team member, peer or senior, about the correct way, without appearing arm-twisty.
ASCII silly question, Get a silly ANSI.
Joined: Mar 04, 2005
Replied for that point that "I disagree on that one relating - the use of 'this' with performance. And changing code to remove 'this' would make a code change in about 30 classes - for a reason which doesnot convince me would make any performance improvement". He is fine without a code change on this one.