TIBCOSoftware / genxdm

GenXDM: XQuery/XPath Data Model API, bridges, and processors for tree-model neutral access to XML.
http://www.genxdm.org/
9 stars 4 forks source link

Use of 'throw new AssertionError("TODO");' is probably non-optimal as a task notation #142

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
See bridgekit.xs. There's functionality that can't be used because, if you try 
to call the methods, it shuts down your JVM. With the useful message: "TODO". 
As an assertion error.

What. True. Fun.

Happens in most of the simple type implementations, if you ask them about their 
derivation, for instance. I think of it as a pleasing sort of shyness, like a 
twelve year-old asked about family, who turns eyes away, blushing, mumbles 
"Don' wan' talk 'bout 'em!" and then produces a high-capacity pistol as if by 
magic, and proves to be remarkably accurate with it.

Bang.

Original issue reported on code.google.com by aale...@gmail.com on 26 Sep 2013 at 9:15

GoogleCodeExporter commented 9 years ago
A review of the code finds over 550 instances of AssertionError, apart from its 
use in PreCondition.

PreCondition is acceptable: it is going to assert the first time that an 
argument fails to meet the contract. Programmers should then be able to catch 
the problem.

In bridges, it typically appears when we have NodeKind-based behaviors, after 
all seven node kinds have been processed. This is acceptable.

There's a lot of similar switch-based processing of an enumeration in 
processors, as well. Again, acceptable to assert for the case where the 
fully-enumerated switch covers all cases, and the default is an assertion.

It is never acceptable to do this to indicate "TODO". Those instances are being 
replaced with UnsupportedOperationException--which is at least catchable, ugly 
as it is. We can look for UnsupportedOperation (will open new defect) to finish 
those implementations.

Original comment by aale...@gmail.com on 24 Oct 2013 at 2:56

GoogleCodeExporter commented 9 years ago
At next commit, our number of instances of AssertionError drops to around 325. 
This is still pretty scary, but it's not as bad as it was, and quite a number 
of those are cases where we do full-cased switching and assert on the default, 
or similar cases of never-happen-ness. I can't say that I think asserting a 
never-happen appeals to me much, but it's an acceptable technique. What the 
checkin *will* do is to insure that we're not doing the really odious trick of 
using an AssertionError as a "TODO" placeholder. That's just evil.

Original comment by aale...@gmail.com on 24 Oct 2013 at 5:17