- 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, just maybe, although highly unlikely, isEnabled() on the parent class is private? Maybe it doesn't return a boolean? Maybe I'll also win the lottery one day.
Admin
On the contrary. This is an excellent piece of code which does its job perfectly.
It says:
The proposed single return statement obfuscates this process and makes it unclear how to add the extra tests; or that they need to be added.
The proposed removal conceals, from an eventual maintainer, the fact that an isEnabled test is needed at all.
The only rational approach is (1) to leave this hygienic code as it is, as insurance for the future, perhaps with a comment saying "Insert additional tests here" - or (2) remove the class altogether.
Admin
Best case, the person who wrote this function did have a use case at some point, but it was removed before they committed it to the source control. Like something in this class would have this function return true even if the super class would return false, but before they committed the code that would allow that edge case, it was removed and the person just forgot to remove this function, and now there's bad code smell.
Worse case (and most likely) this function is in every class in which there's a isEnabled function, and whoever started it just wanted to allow other classes to allow it to return true even if the super class might return disabled.
Admin
I've seen something like this where the function was protected on the base class and made public in the override. That also smells and would be really strange for a getter method like this though.
Admin
Bonus points for using negation to invert the conditional to make it extra confusing.
Admin
I can think of one case where the existence of this method is potentially actually useful: when using AspectJ to intercept the method and do complicated things. The method weaver might actually care about that. Or it might be more correctly implemented; it's hard to be sure without setting up a project. That said, if your codebase has and needs that level of trick, you need to go for a long walk in the country and reconsider the decisions that have lead your life to that point.
AspectJ is a level of trickiness that virtually all Java programmers should avoid. Proper scripting languages are better. (So too is the restricted weaver that Spring uses when it has interfaces available.)
Admin
This is awful: Remy has a new picture.
What happened to the time-worn practice of using pictures from twenty years ago, and then never updating them?
Admin
The old one was only ten years old!
Admin
Clearly it has at least another 10 years of life in it then ;)
Admin
They should have used a ternary. It would make it so much better
Admin
Obviously, this was put in place so that the developer could easily place a breakpoint when this particular class was not enabled…
Just calling the super method directly wouldn’t do it; you’d need a conditional breakpoint.
An override is required; otherwise, the other descendants of the superclass would also trigger the breakpoint.
Should have been removed before committing though!
Admin
That looks like a function that was written because the developer thought 'there MIGHT be a time when we want to do some additional work here so we'll account for that now'. Anyone who hasn't worked with someone like that at some point isn't being sincere :)