- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
Maybe he's trying to get intellisense to pop-up?
Admin
<FONT face=Arial size=2>Or maybe he/she just learned the wonders of abstaction? You know, factories that create factories that create factories...[:P]</FONT>
<FONT face=Arial size=2>I love this part:</FONT>
<FONT face="Courier New"><FONT size=2>public void addNonPersistentDocumentBO(DocumentBO doc) {
Documents.add(doc.getSelf());
}</FONT>
</FONT>
Admin
Maybe trying to fail-fast on NullPointerExceptions?
Admin
getSelf reminds me of the programming language Self and NewtonScript. Perhaps somebody socialized on these two?
Admin
I like how this.getSelf() is the same as this.this
I think this code is just for job security. At least other WTF code attempts to accomplish something.
Admin
Nothing like consistency.
Admin
I believe one can find - within limits - a "reasonable" explanation for this. Specifically, one could introduce a lightweight "wrapper" DocumentBO class. This returns a different object from getSelf() - namely, the heavyweight wrapped object.
Within limits, of course. First, the code is not prepared to accomodate such a lightweight change, as the base class is already heavyweight. Second, it would make far more sense to let the actual methods do the unwrapping. Third, the code would show rather weird behaviour if the wrapped object is a wrapper, too.
But it might have been the developer's original thinking.
Admin
It can't really be an attempt to fail-fast, since this can never be null in Java.
Admin
<font>public</font> <font>void</font> addNonPersistentDocumentBO(DocumentBO doc) {In this code, "doc" can be null, and doc.getSelf() results in an NPE. Just having Documents.add(doc) would pass the null through to the add() method.Documents.add(doc.getSelf());
}
Admin
this.this is an error, because this is not public.
I am thinking that the original 'programmer' was frustrated with private methods and attributes.
Once you come up with the notion that privacy is just an annoyance, creating a public method for 'this' is just a small misstep away.
Admin
Admin
How does retunrning 'this' from a method give access to private attributes? I'm not sure what language this is in but it looks like Java and if you have a reference to an Object, getSelf isn't going to return anything different that what you have already. In other words :
weird.getSelf == weird will be true as long as getSelf() retuns this.
Admin
The only thing that this getSelf() method could accomplish is allowing for a subclass to return something other than this. Perhaps an encapsulated instance. While this might allow some fancy tricks, I think it's a violation of OO principles. You should need to get the 'real' Object from a wrapper or proxy in well designed system.
It's pretty sketchy if you ask me.
Admin
Should say:
You should not need to get the 'real' Object from a wrapper or proxy in well designed system.
Admin
could this be an attempt at a singleton pattern gone horribly wrong?
just my $0.02
Admin
Call it the 'multiton' pattern?
Admin
I guess I didn't explain myself. There are many programmers who get frustrated with privacy when they are thrust into the object oriented world. One reaction is for them to make sure EVERYTHING is declared public. But 'this' is private. Hmmmm. If you made everything else public, why not make a method that returns a public reference to the private 'this'. Ok so its totally superfluous, but if you start with the mentality of wiping out ALL privacy, I can see how the newbie might think this was a good idea.
Admin
Yeah... never reveal anything to client code that's inherently no secret in the given context. For reasons of security and encapsulation, of course.
Also, I'm pretty impressed with the notion of this.this ... looks really nifty - how would you access it, this.this.this?
Admin
It can't be fail-fast on NullPointerException, because "this" can never be null. You would get a NullPointerException when trying to access whatever method had the getSelf() call.
Admin
Yeah, you didn't explain yourself. And yes, it was a good idea not doing so.
You'd better run a search on "this" matter, to find out what "this" actually is; so you've at least learned something today. ;)
Admin
Well, I guess they could be thinking that this enables the getSelf call to be flexible enough so that in the future it would not return this - but return some other DocumentB0 class. If you are already writing code like this the just that getSelf might not actually return self is not such a big jump.
In reality I think the person that said they are trying to get some form of Intellisense or code matching scheme to pop-up the list of functions. I have actually worked with programmers so hooked on that small pop-up that they will make whatever code adjustments they need just to get that small hit. Worse, they will stop and wait until the box comes up and only them feel like moving on with the rest of the code.
At least the class is named properly - DocumentBO does indeed stink
Admin
What
The
Fuck.
Slap that boy with a wet noodle.
Admin
You mean like the classic C++ standard header include:
#define private public
Admin
You're missing the point, Finix. What Rick seems to be suggesting is that his putative newbie coder hasn't thought through just what this is, and so creates a 'publicizing' method for it just like the rest of the instance variables, not realizing that he/she already has the object reference on hand.
Now, I am not claiming that he is right in this particular case - he is only offering one possible scenario, not an absolute explanation - but given the sort of misunderstandings many new coders have, I can (sad to say) imagine someone doing this.
Admin
I should add one other thing: I am not trying to justify this code, and neither, I think, is Rick. We are simply trying to recreate the line of mis-reasoning that would lead a programmer to do something so absurd as this. This can be useful in the same way that an autopsy can be useful in understanding diseases, or an accident review in understanding how disasters occur; it can tell you where things went wrong, and hopefully how to avoid the same problem in the future (though in this case it is the original coder who ought to be shown the results).
Admin
Maybe the guy likes to expand the stack at every opportunity. It makes those stack trace dumps so much more interesting to look at.
Admin
"understanding diseases" seems all too appropriate in this case. [:P]
Admin
sup Xalan.
What, you can't find the error in the 400 lines of stack trace? You must not be looking hard enough. Oh, it's a NPE in a private method with no obvious ties to any method, let alone my XSL source? Better try randomly stabbing away at the stylesheet to find a workaround...
Admin
getUser() had to be private?
Admin
My best guess is that he's having trouble with inheritance, and thus getting confused. For example, let's make up a totally weird OO syntax and guess what his problem is:
private Point field _position;
public virtual Point property Position
uses _position;
end class;
Shape class Circle is:
private Integer field _radius;
public virtual Integer property Radius
uses _radius;
end class;
Got that? Now imagine he has a Shape variable called MyShape, which he assigns to new Circle(), and he tries to refer to MyShape.Radius. It will fail, right? He needs (MyShape as Circle).Radius instead. Annoying if you're not expecting it.
So... he's been bitten by this so many times that he decides to create a bunch of GetSelf functions to return the proper value with the proper type... except that now he's produced something that either won't compile (if he's trying to create overridden virtual functions that differ only in their return types) or won't solve the problem (because now he needs to use (MyShape as Circle).GetSelf.Radius). Either way he's today's WTF.
Admin
Maybe he'd been doing some Smalltalk development, and was missing calls to self?
Admin
Maybe nobody calls him, and he's getting lonely. [:p]
Admin
I don't know the Document.add() method, but I'd say the first thing to do there is to check if the argument is null for the sake of consistence.
Admin
Perhaps he plans on having subclasses that override getSelf() ?
(No - I don't see the logic in that....)
Admin
It's not a matter of breaking encapsulation "because 'this' is private". First, if you have a reference to the object in order to call getSelf() you get nothing different. Second, "this" is not private: it's simply meaningless outside the definition of nonstatic methods and fields.
Could it be to stumble on a potential NullPointerException as soon as possible? This is what happens, but it doesn't seem done on purpose (why repeat the call? After one getSelf() you can use a temp variable) and it is bad error handling anyway (addNonPersistentDocumentBO() doesn't process exceptions; DocumentBO methods would fail before ever calling getSelf()).
Regarding the other nontrivial (1) theory, allowing overriding implementations to return something different, the choice of method name is at least unfortunate (it should be representative of the possible transformation, not generic: getCompactedDocument(), getNormalizedDocument() or compactForm(), normalizedForm() etc.). And there is no evidence of this purpose: getSelf() isn't called only by clients, but mainly by class code.
Who are the involved offshore contractors? Is it considered a competitive advantage of Galmeida's company to let other companies employ them?
(1) the trivial theory (deep-rooted cluelessness, meaningless stupidity and blind ignorance) is only interesting for the subject's colleagues, teachers and psychologists; but since this is an actually hired person, working for a company that approved this code, improving the quality of the author's work seems difficult.<script src="chrome://greasemonkey/content/scripts/1102161148673"></script><script src="chrome://greasemonkey/content/scripts/1102237157909"></script>
Admin
Here is how i think it happned!
Imagine an programmer not used to OO making a program.
He made a empty class "DocumentBO"
Now he has to do some stuff he dosn't know how to and so asks a more experienced OO coder!
He gives him some code like:
<font> if( this</font>.getUser() != null) {
responsible = <font>this</font>.getUser().getUsername();
}
<font>this</font>.setDescription( <font>this</font>.getConstants().getForwardHeader + <font>this</font>.getDescription())
He insertss it in his main code, not in the class as he should!
As it dosn't work he makes a replacement for this in the class namely getSelf().
and replace "this." with "obj.getSelf()." in his main!
Then as he gets more experienced in OO he figures out he should put the code in a method "forwardDocumentCopy()" in his class but when it is copied he can no longer use "obj.getSelf()"
so he replaces it by "this.getSelf()"
Still the guy must be blind and dumb not to realize his mistakes!
Btw. "this.getSelf()" isn't the same as "this.this" it is the same as "this" as the call "this.getSelf()"
returns "this".
Ps. The guy must have learned OO the same way i started DON'T EVER READ ABOUT experiment
it's much more fun and it is't like there is any good books on the subject! :P
(Good nobody but me have access to the code I made in the start!)
Admin
Maybe the programmer wanted to open a backdoor to later introduce "aspect oriented programming" into the program. By changing getSelf() to return a wrapper instead of this, he could trace method calls, introduce access control etc. relatively easy.
Admin
I think the getSelf function used to return a different object in the beginning. But then the programmer combined both classes into one object and it started to redirect itself to itself. Or maybe he was preparing to adjust this code later on.
However, I do think there is a valid reason to do this in this way. What if you create a new class that inherits from this one? And this new class overrides the function getSelf with some other return value. This time a value that isn't 'this'. In that case, this would be a very good solution to use. Definitely not a WTF in my opinion. But again, this is only a valid excuse when this class gets overridden by another one.
Admin
Too bad he missed the _self private member. That way he could assign _self = this; getSelf() { return _self; }. Which would just be more fun.
:|
Admin
Where is it that teaches programmers to name their objects "Foo Business Object"?
Maybe it's just me, but it seems redudant to name my objects "object".
Admin
"Galmeida was reviewing the deliverables [...]"
Proactive words like that really empower me.
Admin
I wouldn't call it a 'very good' solution or even a 'good' solution. It breaks encapsulation and makes the code very difficult to maintain. I have to maintain code that does something somewhat similar and it is a bug-ridden nightmare partly because it's extremely obfuscated by a lot of unecessary layers. Sometimes you have to do things like this but normally it's a workaround to a design flaw.
My guess is that the developer built this in 'just in case' it was needed. Which is a bad approach by itself.
Another thing about this is, it isn't clear how many times should you recurse on getSelf(). If you need to call getSelf() on the outer most reference, who is to say you should call it on the Object returned by getSelf() and so on?
Admin
No no no. getSelf() should call the protected method getRealSelf() which calls _self.getSelf().getRealSelf().getSelf()._self
The word 'Self' looks funny capitalized.
Admin
True, but the main thing for any project is that you'll end up with something that works, not something that's easy to maintain. Easy maintenance is just an additional bonus but when you try to make easy maintenance your goal, you're just missing the point of software development.
So we have this parent object that defines getSelf as 'this'. But a child object could instead return some completely different object instead. It would not need to call the parent method but then again, you could still call the parent and just discard any result. These kinds of constructs aren't very uncommon, yet OO-purists always say that you should not use these methods because they're harder to maintain.
Problem is, always following the proper OO rules will sometimes just slow down development time, or worse, it could reduce the performance of the final project. OO is just a tool that is useful at many times but sometimes you have to tweak a little to get better results.
It's just something I've learned from my dad and I've had plenty of discussions about this with my teachers. Sure, keeping things strictly OO is better, but if you have a deadline, you need to take some shortcuts sometimes. And maybe that's what has happened here.
Personally, I'd would have defined getSelf as an abstract method in this case, but okay. We're not sure if the programmer really wanted to inherit from this class. Still, it's a technique some just won't like but if it gets the job done, you'd get paid for your job. [:D]
Admin
Agreed! This (or rather this.getSelf() ) sucks donkey balls.
Admin
I'm not into sucking donkey balls but if you like it, go ahead... [:P]
I'm not saying that this isn't a real WTF. I'm just saying that similar code could be seen in some projects that do have a valid reason for existing that way. But in general those methods are declared as abstract methods. They don't just do nothing except returning 'this'. Now, if he needed to do some special actions every time when he needed 'this' then it would be okay too. But as it appears to be right now, I agree. The guy who wrote that code should change his profession to 'Donkey-balls sucker'.
Admin
Don't take this the wrong way but are you currently in school? I just want to understand your perspective on this. Again, just so you understand my perspective (I'm not trying to boast or anything (I wish I could)) I've been developing and maintaining software for more than 6 years.
Maintainability is not a 'nice to have'. It's a necessity. Without it, each software cycle takes longer and longer until the team is constantly putting out fires and each enhancement takes as long as the initial development, if not longer. This is where I find myself now thanks to my esteemed predeccesors.
In short, yes, sometimes you have to drop the philisophical purism and get the job done. I don't buy into all the OO purist ideas on a good day. But I know a bad design when I see one (having maintained many and created a few) and this kind of thing is a bad design. Just because time constraints forced a bad design doesn't make it a good design.
If you can describe a senario where the getCannonicalSelf approach is the only way to solve a problem I might be more open to this being an acceptable approach.
Admin
Perhaps, perhaps, this was done in anticipation of an "impl" design. In the future, "getSelf" could be overriden to return a different object other than 'this'. (read: a class that derives from DocumentBO that provides the real implementation). This could be a rather ingenious method of data hiding. Kind of irrelevant in languages such as C# or Java, but possible nonetheless.
Not that I think that's too likely, just a possibility.
Admin
We've already discussed this possibility.
It's still a bad implementation.
The correct way to this is to make DocumentBO an interface and implement the interface as needed.
This kind of approach is sometimes taken with wrappers like for streams because you want to be able to unwrap. But it's not the normal way to operate on the class. If you should be calling getSelf() all over the place, why have the wrapper?
The more I think of it, I think Katja might be on to something with her comment about abstract methods. Perhaps the writer is trying to reinvent polymorphism and misunderstands how it works in Java.
Admin
Yes, but isn't this == this.this?