Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AbstractTitanTx null ambiguity cleanup #57

Closed
wants to merge 2 commits into from
Closed

AbstractTitanTx null ambiguity cleanup #57

wants to merge 2 commits into from

Conversation

DanielThomas
Copy link
Contributor

I ran into NPEs when I fixed a bug in this class due to some null ambiguity issues. This pull requests removes the use of null where it's not part of a contracted method return:

  • Use Guava Optional for newNodes
  • Collection.get* == null has two possible meanings - the key doesn't exist or the key has a null value, use contains* instead (maybe consider null adverse collections which don't allow nulls in keys or values)
  • Set a few fields to final and removed unneeded null dereferencing on close - clear() is called on those collections anyway

* Use Guava Optional for newNodes
* Collection.get* == null has two possible meanings - the key doesn't exist or the key has a null value, use contains* instead
* Also set a few fields to final and removed unneeded dereferencing on close - clear() is called on those collections anyway
@DanielThomas
Copy link
Contributor Author

I also cleaned up the mixed tab/space indentation and trailing whitespace, but in a separate commit so you don't have to navigate those changes to see the actual work I did.

@mbroecheler
Copy link
Member

Merged the pull request manually. Accepted all changes but since I changed the VertexCache contract to not allow null vertices I left the getExistingVertex method to using node!=null for slightly better performance since this method can be called very frequently.

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.

2 participants