• Post Reply Bookmark Topic Watch Topic
  • New Topic

Horrible production code I found today  RSS feed

 
fred rosenberger
lowercase baba
Bartender
Posts: 12563
49
Chrome Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I didn't write this..I don't know who did, but it hurts my brain:


 
Rico Felix
Ranch Hand
Posts: 411
5
IntelliJ IDE Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
What the mooing cow is that mess...

I can't seem to find a sense of direction on what that snippet is doing...

Why is there concatenation of an empty string as it serves no purpose... If an exception is thrown it never gets concatenated and even if it gets concatenated then regNo will never be empty as it has parsed the argument successfully which requires the argument passed to the Long constructor to not be an empty string...
 
Jaikiran Pai
Sheriff
Posts: 10447
227
IntelliJ IDE Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator

return regNo.isEmpty()?null:regNo; //trim leading zeroes


I like the comment there

 
Priyanka Batham
Greenhorn
Posts: 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
it says //if not a number, leave as is, but its not performing any operation even if its a number.
 
Brett Spell
Ranch Hand
Posts: 118
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Priyanka Batham wrote:it says //if not a number, leave as is, but its not performing any operation even if its a number.


I believe it's referring to the entire block that follows, which will trim any leading zeroes since Long's toString() method will return the trimmed equivalent of whatever the original string was -- assuming that it could be parsed. If it wasn't a number, though, the original text is returned or null if the original text was an empty string.

But yes, it's pretty obtuse code that's made worse by the lack of white space.
 
Rico Felix
Ranch Hand
Posts: 411
5
IntelliJ IDE Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
In my opinion I think it would have been clearer to use something like


And not silence the exception thrown if one does occur but do something with it such as supply feedback or log it somewhere

Also instead of performing a test at the return statement it could have been done in the try and catch block as follows

 
Brett Spell
Ranch Hand
Posts: 118
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Rico Felix wrote:Why is there concatenation of an empty string as it serves no purpose...


It's poorly implemented, but the concatenation does serve a purpose: if you take the concatenation out then it's an attempt to assign a Long to a String which will cause a compiler error. Adding the concatenation causes the Java compiler to instead interpret it as this (valid) equivalent instead:

 
Brett Spell
Ranch Hand
Posts: 118
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Ah, I see you realized after your initial post why they added the concatenation.
 
fred rosenberger
lowercase baba
Bartender
Posts: 12563
49
Chrome Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My colleague and I figure it is trying to do this...

get some kind of string.
if it is a number, trim off the leading zeros. (we often get values like 000235452)
otherwise, return the original string.

it is just ugly.
 
Tim Cooke
Marshal
Posts: 4044
239
Clojure IntelliJ IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
A particularly 'special' thought process has gone into that snip of code.
 
Mike. J. Thompson
Bartender
Posts: 689
17
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
If they had just put the //trim leading zeroes comment on the line that actually trims the leading zeros then it would at least be vaguely intelligible.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
fred rosenberger wrote:if it is a number, trim off the leading zeros. (we often get values like 000235452)
otherwise, return the original string.
it is just ugly.

Plainly whoever wrote it didn't read the StringsAreBad page.

I will say this however: Number parsing is one of the few occasions where I think using exceptions as a form of control is acceptable, eg:on the premise that you're not re-inventing the wheel. I'm happy to assume that whoever wrote Long knows far more about what a "correct 'long' string" looks like than I do.

Maybe something like that was going through their head...

Winston
 
Tim Cooke
Marshal
Posts: 4044
239
Clojure IntelliJ IDE Java
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Fred, I take your horrible code and I raise you this little gem:

The choice of names confuse things significantly.
 
fred rosenberger
lowercase baba
Bartender
Posts: 12563
49
Chrome Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
My buddy just found this:


one line of code
 
Bear Bibeault
Author and ninkuma
Marshal
Posts: 66306
152
IntelliJ IDE Java jQuery Mac Mac OS X
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Tim Cooke wrote:The choice of names confuse things significantly.

Good grief, that's horrible!
 
fred rosenberger
lowercase baba
Bartender
Posts: 12563
49
Chrome Java Linux
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I swear someone once told me about a custom boolean class written for the company's application that would return "true", "false", or "file not found".
 
Sresh Rangi
Ranch Hand
Posts: 54
5
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
fred rosenberger wrote:I swear someone once told me about a custom boolean class written for the company's application that would return "true", "false", or "file not found".


A classic:

What is Truth?
 
Tim Cooke
Marshal
Posts: 4044
239
Clojure IntelliJ IDE Java
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
A couple more from my career back issues:

This one I witnessed first hand in the codebase belonging to a very well known consumer electronics and digital entertainments company.

(Can't remember exactly the return type but it was some representation of the application as a whole, we weren't quite sure, hence the name)

Here's another one passed on from a colleague at the above company. This gem was from a codebase belonging to another, not quite as big, consumer electronics company he'd worked at previously:

(I've made up the return type here, the fella only told me the method name... at which point I'd heard enough)
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
fred rosenberger wrote:I swear someone once told me about a custom boolean class written for the company's application that would return "true", "false", or "file not found".

Agreed, and I particularly liked DisturbedSaint's reply (thanks for the link Sresh): Yes, No, or Car.

On the other hand: Yes, No, and Maybe, or Yes, No, Dunno, are perfectly valid trios as far as I'm concerned, because they crop up often, and have a close relation to a signum() function.

Take hashcodes. If they're equal, the objects MAY be equal, but if they're not, then they definitely aren't. Admittedly, only two of the three; but I have found cases where it's useful to use a Boolean (as opposed to a boolean), because you can return null as a third option.

Obviously with appropriate docs; and these days, I'd probably opt for an enum.

Winston
 
Paul Clapham
Sheriff
Posts: 22829
43
Eclipse IDE Firefox Browser MySQL Database
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
We had a lot of what I'll call "faux boolean" data elements at the company I used to work at. There wasn't a "boolean" data type in the database so yes-or-no decisions were represented by a one-character string containing either Y or N. And the language we were using wasn't Java.

For example there might be a field which told us whether the customer wanted to see retail prices on their invoices. Yes or No, right? And then along came a customer who wanted to see retail prices but not for cigarettes. So now the field had three values. In other examples there were five or six possible values.

So that's likely how those "custom boolean classes" arise in other applications.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Paul Clapham wrote:So that's likely how those "custom boolean classes" arise in other applications.

Weirdly enough, I've been working on a solution to the "equals" problem for object hierarchies for a while now, and I've come to the conclusion that something as simple as "equal" is NOT simply a "yes/no" answer - at least, not until you're actually ready to return it to the caller.

The fact is that, for any two objects, the only answers that can be gleaned from a superclass are: NOT_EQUAL, TYPE_COMPATIBLE (or EQUAL_SO_FAR) and IDENTICAL.

In the first case you can return false, and in the latter you can return true; but for the middle one you have some more checking to do...

And if we weren't constrained by that darn "true/false" response, we wouldn't have to prefix all our overridden methods with:
if (this == obj)
   return true;

to short-circuit the process.

Winston
 
James Boswell
Bartender
Posts: 1051
5
Chrome Eclipse IDE Hibernate
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
fred rosenberger wrote:I swear someone once told me about a custom boolean class written for the company's application that would return "true", "false", or "file not found".


Ah, it can get even better, I give you The Extra Boolean
 
Consider Paul's rocket mass heater.
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!