2

I am using the Google Collections library AbstractIterator to implement a generator. I ran across a problem while doing so; I've reduced it to a more basic type and reproduced the problem. This reduction is obviously overkill for what it does, counting from 1 to numelements via an Iterable.

Essentially in the following code, the uncommented version works, and the commented one does not (provides a null element last, instead of ending on the last number).

Am I doing something wrong, or is this a problem with the library?

private Iterable<Integer> elementGenerator(final int numelements) {
  return new Iterable<Integer>() {
    @Override public Iterator<Integer> iterator() {
      return new AbstractIterator<Integer>(){
        int localcount=0;
        @Override protected Integer computeNext() {
          if (localcount++ == numelements) return endOfData();
          return localcount;
          // return (localcount++ == numelements) ? endOfData() : localcount;
        }
      };
    }
  };
}

I also tried fiddling around with the ?: arrangement (e.g., prefixing the return and comparing to +1 instead), to no avail. I poked around a bit looking for documentation about this, but didn't find anything. Obviously, the ?: syntax is only a convenience, not a necessity, but still...

Kevin Bourrillion
  • 40,336
  • 12
  • 74
  • 87
Carl
  • 7,538
  • 1
  • 40
  • 64
  • The uncommented version does not compile. I take it you mean to switch which return statement is commented rather than just uncommenting the one line? – mR_fr0g Nov 30 '09 at 18:33
  • No, it's either the last line comment or the two lines before. – Andreas Dolk Nov 30 '09 at 18:51
  • The iterator counts from 1 to (numelements-1) - instead of returning numelements it returns endOfData() (but for both version, as far as I can see and reproduce @home... magic.) – Andreas Dolk Nov 30 '09 at 18:56
  • I can't help but look at this code and shudder at the abuse of the post-increment operator. – Powerlord Nov 30 '09 at 19:04
  • It counts from 1 to numelements, so sayeth my JUnit test. It should be - the value is compared, then incremented. So, numelements should be returned last. – Carl Nov 30 '09 at 19:06
  • and by that, I mean `if (localcount++ == numelements)` is identical to `(localcount == numelements)` then incrementing localcount by 1. – Powerlord Nov 30 '09 at 19:07
  • @R: yup, I have dirty laundry. I like to use the pre-fix operator as well, and you should see my abuses in perl. Also, I don't brush my teeth after lunch. – Carl Nov 30 '09 at 19:24

2 Answers2

5

You get a NullPointerException because of the use of the ternary operator, conditional expression, with different numerical types. Java has complex rules when mixing numerical values of different types in ternary expression: JLS Section 15.25.

Given that endOfData() is preceived to return Integer, while localcount is an int, Java unboxes the value of endOfData(). However, given that endOfData() returns a null, the unboxing operation results into a null pointer exception.

You can either continue using the if statement, or declare localcount as Integer.

notnoop
  • 58,763
  • 21
  • 123
  • 144
  • huh; glad someone is more familiar with the spec. Worked like a charm, thanks. Guess that leaves sorting out the problem with the real version. – Carl Nov 30 '09 at 19:02
  • By the way, I recommend using the if statement here. It's easier for people not to miss up in future edits. – notnoop Nov 30 '09 at 19:05
  • In general I agree that ?: decreases readability/maintainability, but when the logic is as simple as this case and will likely remain so, I find the one-liner more readable. Barring some comprehensive use studies (which I would be interested to see, if they exist), seems to boil down to preference for the simple case. – Carl Nov 30 '09 at 19:15
  • I actually prefer ?: expressions as they are more compact. However as you've seen here, this case isn't trivial. In this case, you also have the hidden complexity of unboxing 'localcount' in the equality test. – notnoop Nov 30 '09 at 19:25
0

I expect the problem is with your use of the postincrement operator, in conjunction with the ternary operator as well. Because asides from that, the two snippets should be entirely equivalent - and it's hardly the AbstractIterator's fault if they're not as none of its code is being called at that point.

Andrzej Doyle
  • 102,507
  • 33
  • 189
  • 228