Lazily decode a buffer with a thread-safe Iterator

Question

I have implemented an Iterator for my Pojo class.
The purpose here is to lazily decode multiple Pojos encoded in a buffer (bytes array).
To improve performance, it needs to be thread-safe to be correctly used from multiple Thread (parallel Stream).
The buffer is formed as follows :

1. The first 4 bytes represent an int indicating how many bytes the next element uses in the buffer : n.
2. The following n bytes are encoded representation of a single Pojo.
3. 4 other bytes for the length of the next element.
4. And so on ...

I think it is achieved with this code :

public class StringDecodeIterator implements Iterator<Pojo> {
 private final int size;
 private final PojoDecoder decoder;
 private final byte[] buffer;
 private int offset = 0;
 private int count = 0;
 private StringDecodeIterator(int size, PojoDecoder decoder, byte[] buffer) {
 this.size = size;
 this.decoder = decoder;
 this.buffer = buffer;
 }
 @Override
 public synchronized boolean hasNext() {
 return this.count < this.size;
 }
 @Override
 public Pojo next() {
 return this.decoder.decode(offset(), this.buffer);
 }
 private synchronized int offset() {
 if (!(this.count < this.size)) {
 throw new NoSuchElementException("No more element");
 }
 this.count++;
 int o = this.offset;
 this.offset += Utils.getInt(o) + 4;
 return o;
 }
 }

Here are some explanations/details :

1. size is the number of element to decode : It is a known value; we know how many Pojos are encoded.

2. PojoDecoder decoder is an object that can decode Pojos from a buffer at a given position.

   interface PojoDecoder {
    Pojo decode(int offset, byte[] buffer);
   }
   

3. Utils#getInt(int); just converts 4 bytes to an int from the given position.

4. PojoDecoder#decode(int, byte[]) does not need to be in the thread-safety scope. Same instance can be used from multiple threads, as it does not hold any internal state.

• TorbenPutkonen: I don't thing it makes sense to try to make an Iterator thread safe. An iterator expects the caller to execute hasNext() and next() in series so the thread safety relies completely on the caller putting them in the same synchronized block. So there's not much point in trying to add synchronized keywords to the interator anymore. And if you need to support parallel streams, it's better to use a Spliterator instead.
• Heslacher: Welcome to Code Review! I have rolled back your last edits. Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

In its current form, the code doesn't work:

• unless it's an inner class of some enclosing one, the constructor should not be private;
• Utils.getInt(...) needs a second param: the array to read from.

I'm guessing that both StringDecodeIterator and Utils were originally inner classes of the same enclosing class, that also had a reference to buffer which was used by Utils.getInt(int)... For the future please make sure that the code you post here works in exactly the form it is posted in: it makes reviewing much easier.

The rest are just nitpicks:

• class name: I fail to see any Strings here: how about ByteArrayDecodingIterator?
• generalization suggestion: not sure if it will be useful in your case, but you can make this class and the decoder interface generic, so that you can use it also for any other type apart from just currently hard-coded Pojo.
• naming: IMO getNextOffset() would be slightly more clear regarding its purpose than just offset().
• naming: o is a very ambiguous name, even for a local: how about resultOffset?
• if (!(this.count < this.size)) : why not just if (this.count >= this.size)?
• exception message: I don't think "No more element" brings any value given that the class of the exception is NoSuchElementException. Also, I'm not a native speaker, but I'm pretty sure it should be "No more elements" (plural).
• not sure if it's worth having a utility method Utils.getInt(...) just for
  ByteBuffer.wrap(buffer, offset, Integer.BYTES).getInt() (unless this needs to be blazing fast for some reason and even creating a temporary ByteBuffer is too much of an overhead in your situation).
• magic numbers: use Integer.BYTES instead of 4.

• William: Thanks for your answer, indeed, the class was inner in the past. I've taken your comments into account. About ByteBuffer.wrap(...), in fact, this is what is used in the utils method, I've encapsulated it to simplify the review
• Voo: The much, much bigger problem than all of those listed, is that it's simply impossible to provide a thread-safe iterator, since the underlying API is simply not designed for such a use case. There's an inherent race condition with the HasNext()/Next() sequence.
• morgwai: @Voo indeed, good point. This should however rather be the answer to the OP (that i will gladly upvote), than a comment to this answer, that the OP may not even notice ;-)