Skip to content

Conversation

@iconara
Copy link
Member

@iconara iconara commented Oct 23, 2015

I had a hard time explaining what I though must be changed in #91 so I made the changes myself.

  • Setters have been removed and replaced with constructor arguments. Adding setters introduces a possibility to change the internals of the objects at any time, something that they don't really support. Since there is no need to change these objects after creation there shouldn't be a way to do it.
  • Replace the two extension registries with a single registry. I think this is a design mistake in the C implementation that spills over into everything else. I don't see any point in having separate registries. It complicates everything. The new registry also uses Java collections internally, because using Ruby collections in Java code makes the code very hard to read. The new implementation encapsulates things properly so that the actual implementation is not spread out over three or four different classes.
  • I've also removed superflous comments and simplified code.

Unfortunately the tests don't pass in this branch, Factory#registered_types_internal, Packer#registered_types_internal and Unpacker#registered_types_internal have inconsistent interfaces. The consumers of these methods expect slightly different things in the returned values. This seems to be a consequence of the aforementioned design decision in the C implementation, and I consider it a bug. I can't fix it without changing the C code and the design, something I'm not comfortable doing.

@tagomoris this is how I think the JRuby implementation should work, but as I mentioned it requires changes to the C implementation. It's up to you to decide how to proceed.

Comments are at best outdated, but most of the time they confuse more than they help. Whenever you feel the urge to write a comment, think about how you can rewrite the code to make it not need a comment to explain what it does.
By extracting the code below a comment to a method that explains what the code does the comment is not needed and the code is self-explaining.
What this comment says is better explained by the exception thrown by the method.
…ternals

Use Java collections for the registry and cache, and use a class for the four-tuple containing the registered entries.
Make it possible to look up extensions in ExtensionRegistry by type ID, and add the features needed to make it work for the unpacking side of things.

Eliminate duplicated code in Factory#registered_types_internal and Unpacker#registered_types_internal by moving it to ExtensionRegistry.

There are a few tests that fail because of this change. The reason is that Factory#registered_types_internal, Packer#registered_types_internal and Unpacker#registered_types_internal have inconsistent interfaces. The consumers of these methods expect slightly different things in the returned values. I consider this a bug, but I can't change it without changing the C code, something I'm not comfortable with.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants