ZOOKEEPER-3663: Clean Up ZNodeName Class #1193
Conversation
belugabehr
force-pushed
the
ZOOKEEPER-3663
branch
from
0735b33
to
d14d7eb
Compare
| */ | ||
| public int getZNodeName() { | ||
| public Optional<Integer> getSequence() { |
There was a problem hiding this comment.
it looks like this method is called only in tests.
you can drop the 'public' modifier
There was a problem hiding this comment.
All of the other getters are also public, so I think it should be consistent with them.
| if (idx >= 0) { | ||
| private final String name; | ||
| private final String prefix; | ||
| private final Optional<Integer> sequence; |
There was a problem hiding this comment.
The difference from int to Optional in terms of space/memory usage is considerable.
Is it really worth to change it ?
This is only a 'recipe' so not a big deal, but I wonder if is it better to keep it simpler.
There was a problem hiding this comment.
@eolivelli I always prefer using Optional. It's always very clear as to its usage and the caller must deal with it. Better than forcing the user to correctly check for a special negative value that indicates 'no sequence available'. This is not performance-critical code, so readability and usability should be prioritized.
|
@eolivelli Thank you always for your reviews. |
|
@belugabehr |
|
Committed to master branch. Thanks @belugabehr ! |
Author: David Mollitor <dmollitor@apache.org> Reviewers: eolivelli@apache.org, andor@apache.org Closes apache#1193 from belugabehr/ZOOKEEPER-3663
Author: David Mollitor <dmollitor@apache.org> Reviewers: eolivelli@apache.org, andor@apache.org Closes apache#1193 from belugabehr/ZOOKEEPER-3663
Author: David Mollitor <dmollitor@apache.org> Reviewers: eolivelli@apache.org, andor@apache.org Closes apache#1193 from belugabehr/ZOOKEEPER-3663
Author: David Mollitor <dmollitor@apache.org> Reviewers: eolivelli@apache.org, andor@apache.org Closes apache#1193 from belugabehr/ZOOKEEPER-3663
No description provided.