How about calling this byteArray instead?

Could a for (Xxx xxx : yyy) work here?

Unfortunately the generated proto code does not implement Iterable. I could iterate over the LongArray, but then I would still need an index variable to grab the correct value from the ProtoLongArray.

Here you can use new CoreException ("blah blah {} and {}", one, two); and it will become blah blah one and two like when using a logger.

How about returning a new one every time? That would let you not have any transient variable. Less risky if you try to de-serialize a Java based Atlas and then re-save it... You would get a NPE here, no?

Nit: Extra Line

Could you elaborate more on when do you think the references should be equal? Maybe an example in the comment might make it better to understand

Good idea. A lot of the equals/hashCode implementations are still in flux.

instead of assigning initialPrime to hash how about

final int initialPrime = 31;
final int hashSeed = 37;

int hash = hashSeed * initialPrime + this.keys.hashCode();
hash = hashSeed * hash + this.values.hashCode();

return hash;

This makes a lot more sense haha.

Another optimization :P

final int initialPrime = 31;
final int hashSeed = 37;

final int hash = hashSeed * initialPrime + this.keys.hashCode();
return hashSeed * hash + this.values.hashCode();

Instead of extracting in a local variable you can also use a lambda

atlasMetaData.getCodeVersion().ifPresent(value -> {
  builder.setCodeVersion(value);
});

Good tip. I am new to using Optional. Thanks!

how about

return builder.build().toByteArray();

I had that intermediate variable there just to make it very clear to future readers of the code what we are returning (since the builder is declared pretty far away from this return statement).

I'm actually going to build on your suggestion (no pun intended) by refactoring the name of 'builder' to be a little more descriptive, something like 'protoMetaDataBuilder'.

Best of both worlds that way!

How about

return dictionaryBuilder.build().toByteArray();

This way you don't need to instantiate a local variable before returning.

Just checking - data version field was left out intentionally here?

No it should be there haha. Since I check against it in equals(), I should be using it in the hash computation. Good catch.

Nit: Consider adding a link to DEFAULT_VALUE constant in the javadoc by using @link(#[your-variable]) notation. This way if it ever changes, it will get updated automatically.

Nit: Not sure this warrants an author update :P

Nice and clean!

Nit: not part of your changes, but consider updating the CoreException to follow insertion format:
throw new CoreException("Cannot add. Invalid index {} is bigger than the size {}", index, size());

Nice commenting!

All the exceptions get handled the same way - there is no difference what kind of Exception it is, we can't recover from it. Might be a good spot to just catch Exception.

Same comment here and in all below occurrences.

Nit: Not sure some of these changes warrant an author update.

I thought a bit about this. Where should the line be? My internal thought was if I added at least one new method, or significantly changed a method, I added an author tag - if anything to document from whom the code in the class is coming from.

Looking at LongArray, I think this is probably a case where I was too overzealous with the tag. But I think for the classes where I started adding new interfaces, including equals() and hashCode() implementations, that tag is a good hint for future users of the class.

Another solution may be to simply add an author tag to the methods I add, instead of the class itself.

This is a tricky area - everyone has different opinions on what's the right convention. This is only my opinion, so take that for what it's worth :) - I try to answer both of these questions with a yes before adding an author tag:

1. Did I make a change that can have downstream/user impact and/or add significant functionality on top of the existing codebase?
2. Do I feel comfortable explaining this class and its functionality and/or debugging/being responsible for any bugs that are a result of my changes?

In your case case, most of the author tags are spot on! I'd keep all of them as is, including the ones with equals/hashcode implementations. Not a fan of the fine-grained per-method author tags as that just brings more chaos to the code. I just nit-picked two classes that stood out to me as a bit overzealous (PackedAtlas and LongArray). Again, feel free to ignore :)

Got it. I'll revert the per-method tags in a future commit. All this makes sense. Yeah I do agree that PackedAtlas and LongArray are overzealous haha.

Should this be instanceof LongToLongMultiMap?

Yeah... copy paste strikes again.

preemptive nit: extra line
:)

Consolidate these exceptions and add a message.

Why would those be needed if they are not serialized?

You already had them defined in EXCLUDED_FIELDS in PackedAtlasSerializer, but the values were hardcoded. I just added named constants so that there would be no hardcoded values in PackedAtlasSerializer that depended on the names of variables in PackedAtlas.

Here's a side-by-side:

Old PackedAtlasSerializer

private static final StringList EXCLUDED_FIELDS = new StringList("bounds", "serialVersionUID",
            "logger", "$SWITCH_TABLE$", PackedAtlas.FIELD_SERIALIZER, "FIELD_");

New PackedAtlasSerializer

private static final StringList EXCLUDED_FIELDS = new StringList(PackedAtlas.FIELD_BOUNDS,
            PackedAtlas.FIELD_SERIAL_VERSION_UID, PackedAtlas.FIELD_LOGGER, "$SWITCH_TABLE$",
            PackedAtlas.FIELD_SERIALIZER, PackedAtlas.FIELD_PREFIX);

Isn't loadSerializationFormat always dependent on the file that it tries to load from? To my mind only the saveSerializationFormat would be needed, no?

You need a flag to be set at load call time so that lazily deserialized Atlases know which format to select. However, you make a good point in a comment below. The loadSerializationFormat does not need to be a field of PackedAtlas. It can just be a flag in the PackedAtlasSerializer.

This would return a byte[] correct? In case the field is big, would it be possible to stream the resource, one byte buffer at a time using an InputStream instead? That would save memory. Only if it is easy though, as the tradeoff might not be worth it..

I may be misunderstanding your question. But looking at the code in Resource, is this not already happening?

default byte[] readBytesAndClose()
{
        final List<Byte> byteContents = new ArrayList<>();
        int kyte;
        try (InputStream input = new BufferedInputStream(read()))
        {
            while ((kyte = input.read()) >= 0)
            {
                byteContents.add((byte) (kyte & BYTE_MASK));
            }
        }
        catch (final IOException e)
        {
            throw new CoreException("Unable to read the bytes from {}.", this, e);
        }
        final byte[] contents = new byte[byteContents.size()];
        for (int index = 0; index < byteContents.size(); index++)
        {
            contents[index] = byteContents.get(index);
        }
        return contents;
}

It looks to me like it is already using a BufferedInputStream to read the data from the underlying resource.

Were you instead asking to have the ProtoAdapter.deserialize() method take an InputStream instead of a byte[] as input? That would be possible. I believe protobuf provides a parseFrom overload that takes an InputStream instead of a byte[].

I would keep that load format local to the PackedAtlasSerializer here as it is very dependent on the resource being read.

Yep! Good idea.