Horrible production code I found today

I didn't write this..I don't know who did, but it hurts my brain:

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...

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

I like the comment there

it says //if not a number, leave as is, but its not performing any operation even if its a number.

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.

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

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:

Ah, I see you realized after your initial post why they added the concatenation.

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.

A particularly 'special' thought process has gone into that snip of code.

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.

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

Fred, I take your horrible code and I raise you this little gem:

The choice of names confuse things significantly.

My buddy just found this:


one line of code

Tim Cooke wrote:The choice of names confuse things significantly.

Good grief, that's horrible!

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".

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?

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)

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

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.

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

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