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

Allow null values for Graphs.Edge #9

Closed
michaelamaura opened this issue Dec 15, 2018 · 3 comments
Closed

Allow null values for Graphs.Edge #9

michaelamaura opened this issue Dec 15, 2018 · 3 comments

Comments

@michaelamaura
Copy link

michaelamaura commented Dec 15, 2018

As far as I can tell the io.lacuna.bifurcan.Graphs.Edge<V, E> class serves as a standard implementation for IEdge<V, E>.

As in many graph instances the edges won't have a special value, it should be allowed to set the value to null. But now, the #hashCode method will throw a NullPoinrterException.

It should either be changed to:

@Override
public int hashCode() {
  if (hash == -1) {
    hash = from.hashCode() ^ to.hashCode() ^ (vale == null ? 0 : value.hashCode());
  }
  return hash;
}

or to:

@Override
public int hashCode() {
  if (hash == -1) {
    hash = Objects.hash(from, to, value);
  }
  return hash;
}

If either from or to is null a NullPointerException should be thrown by the constructor.

@ztellman
Copy link
Member

ztellman commented Jan 8, 2019

Sorry for the late response, I've been traveling. Thanks for catching this, since from or to could also potentially be null I've changed it to this:

public int hashCode() {
      if (hash == -1) {
        hash = Objects.hashCode(from) ^ Objects.hashCode(to) ^ Objects.hashCode(value);
      }
      return hash;
    }

@michaelamaura
Copy link
Author

michaelamaura commented Jan 8, 2019

Hey, great thanks for fixing this :-)

BTW: Great library, has been really helpful on a project I'm working on :-)

Just out of interest: Can you tell me the advantage from xoring the different hashCodes over calling Objects#hash? Is it the array allocation?

@ztellman
Copy link
Member

ztellman commented Jan 8, 2019

There is some overhead with Objects.hash, but I believe my original rationale was to have equivalent hashes for undirected edges. On reflection, however, that doesn't also hold true for the equality check, so it probably makes sense to have DirectedEdge and UndirectedEdge classes.

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

No branches or pull requests

2 participants