feat(bookmarks): Permit references to tags from previous invocations #7
Conversation
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
src/main/java/com/github/olivergondza/saxeed/internal/BookmarkImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/olivergondza/saxeed/internal/BookmarkImpl.java
Outdated
Show resolved
Hide resolved
| private TagImpl(TagImpl parent, TagName name) { | ||
| this.parent = parent; | ||
| this.name = Objects.requireNonNull(name); | ||
| this.attrs = null; // No SAX attrs, setting attributes right away | ||
| // No SAX attrs, setting attributes right away | ||
| this.attrs = null; | ||
| this.attributes = new LinkedHashMap<>(); | ||
| this.namespaces = null; | ||
| this.generated = true; | ||
| this.bookmark = initBookmark(); | ||
| init(parent); | ||
| } |
There was a problem hiding this comment.
The constructor TagImpl(TagImpl parent, TagName name) initializes the attributes and namespaces fields to empty collections or null. This can lead to potential NullPointerException issues if these fields are accessed without null checks elsewhere in the code. Consider initializing these fields to empty collections to avoid such issues.
| public TagImpl(TagImpl parent, TagName name, Attributes attrs, Map<String, String> namespaces) { | ||
| this.parent = parent; | ||
| this.name = Objects.requireNonNull(name); | ||
| this.attrs = Objects.requireNonNull(attrs); | ||
| // Create defensive copy in either case | ||
| this.namespaces = namespaces.isEmpty() ? null : new LinkedHashMap<>(namespaces); | ||
| this.generated = false; | ||
| this.bookmark = initBookmark(); | ||
| init(parent); | ||
| } |
There was a problem hiding this comment.
The constructor TagImpl(TagImpl parent, TagName name, Attributes attrs, Map<String, String> namespaces) does not perform a null check on the namespaces parameter before calling namespaces.isEmpty(). This can lead to a NullPointerException if namespaces is null. Consider adding a null check for the namespaces parameter before using it.
src/main/java/com/github/olivergondza/saxeed/internal/TransformationHandler.java
Show resolved
Hide resolved
src/main/java/com/github/olivergondza/saxeed/internal/TransformationHandler.java
Show resolved
Hide resolved
olivergondza
force-pushed
the
chained-transformation
branch
from
e7f5381
to
2720eb2
Compare
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
|
|
||
| BookmarkImpl bookmark = (BookmarkImpl) o; | ||
|
|
||
| // If either is omitted, they are not equal. This is to prevent that omitted bookmark would match real tag based | ||
| // on value clash. | ||
| if (omitted || bookmark.omitted) { | ||
| return false; | ||
| } | ||
|
|
||
| return Objects.equals(value, bookmark.value); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(value, omitted); | ||
| } |
There was a problem hiding this comment.
The equals and hashCode methods are not consistent. The equals method considers both value and omitted fields, while the hashCode method only considers the value field. This inconsistency can lead to unexpected behavior when instances of BookmarkImpl are used in collections that rely on these methods.
Recommendation: Ensure that both equals and hashCode methods consider the same fields. Either include omitted in the hashCode method or exclude it from the equals method.
olivergondza
force-pushed
the
chained-transformation
branch
from
64122c9
to
68e5c35
Compare
olivergondza
force-pushed
the
chained-transformation
branch
from
68e5c35
to
9ff2f7f
Compare
No description provided.