Repository navigation
Base58: protect alphabet from mutation - #4374
schildbach wants to merge 1 commit into
Conversation
Make the constant `ALPHABET` private and provide an accessor `alphabet()` to use instead. Add tests to verify the alphabet cannot be mutated any more.
msgilligan
left a comment
msgilligan
left a comment
There was a problem hiding this comment.
If we want to return an (immutable/unmodifiable) List I have some requested changes.
Alternatively, we could return a String which is immutable. We should also be able to use a String internally. I'll make a separate PR to explore this option.
| return CharBuffer.wrap(ALPHABET) | ||
| .chars() | ||
| .mapToObj(c -> (char) c) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
If we replace Collectors.toList() with our utility StreamUtils.toUnmodifiableList() the returned list will be unmodifiable and will throw UnsupportedOperationException if modification is attempted.
| alphabet1.add(0, '₿'); | ||
| List<Character> alphabet2 = Base58.alphabet(); | ||
| assertEquals(previousChar, alphabet2.get(0).charValue()); | ||
| } |
There was a problem hiding this comment.
If StreamUtils.toUnmodifiableList() is used as suggested above, this test can use assertThrows to test for unmodifiability.
See PR #4378 |
For the API method For the internal storage, I'm more relaxed about what type to use. |
An alphabet is an ordered collection/sequence of characters. So is Yes, Strings are often serialized/output in their (ordered) entirety, but they are still a collection/sequence of characters.
We could make |
2 participants
Make the constant
ALPHABETprivate and provide an accessoralphabet()to use instead.Add tests to verify the alphabet cannot be mutated any more.