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

Trie tech debts #1655

Open
fedejinich opened this issue Nov 11, 2021 · 0 comments
Open

Trie tech debts #1655

fedejinich opened this issue Nov 11, 2021 · 0 comments

Comments

@fedejinich
Copy link
Contributor

fedejinich commented Nov 11, 2021

There are many tech debts in the Trie class, I've spotted some of them

public class Trie {
    ...
    private Trie(TrieStore store, TrieKeySlice sharedPath, byte[] value, NodeReference left, NodeReference right, Uint24 
      valueLength, Keccak256 valueHash, VarInt childrenSize) {
        this(store, sharedPath, value, left, right, valueLength, valueHash, childrenSize, exitStatus -> System.exit(exitStatus));
    }

    // full constructor
    private Trie(TrieStore store, TrieKeySlice sharedPath, byte[] value, NodeReference left, NodeReference right, Uint24 
      valueLength, Keccak256 valueHash, VarInt childrenSize, @Nonnull NodeStopper nodeStopper) {
        this.value = value;
        this.left = left;
        this.right = right;
        this.store = store;
        this.sharedPath = sharedPath;
        this.valueLength = valueLength;
        this.valueHash = valueHash;
        this.childrenSize = childrenSize;
        this.nodeStopper = Objects.requireNonNull(nodeStopper);
        checkValueLength();
    }
    ...
    /**
     * get by string, utility method used from test methods
     *
     * @param key   a string, that is converted to a byte array
     * @return a byte array with the associated value
     */
    public byte[] get(String key) {
        // todo(fedejinich) this method is useless, the String to byte[] convention should be done at the tests
        return this.get(key.getBytes(StandardCharsets.UTF_8));
    }
    ...
    /**
     * put string key to value, the key is converted to byte array
     * utility method to be used from testing
     *
     * @param key   a string
     * @param value an associated value, a byte array
     *
     * @return  a new NewTrie, the top node of a new trie having the key
     * value association
     */
    public Trie put(String key, byte[] value) {
        return put(key.getBytes(StandardCharsets.UTF_8), value);
    }
    ...
    @Nullable
    public List<Trie> getNodes(String key) {
        return this.getNodes(key.getBytes(StandardCharsets.UTF_8));
    }
    ...
}

Test code in production

There are two overloads for get() and put() methods that are only used in test cases, those methods should be removed in order to clean test code from production.

Unecesary overloads

getNodes()

There are many overloads for the getNodes() method, to me, this doesn't add any real value, in most cases, those conversions are just a simple Trie.fromKey(key). To simplify the Trie class, I propose only to leave the default findNodes() implementation, but if you push me, I think we should extract this method to the MutableTrie implementation (where we should do most of the trie manipulations).

Trie Constructor Overload

This overload it's unnecessary since the node stopper it's always initialized with System.exit(exitStatus). I propose to delegate that part into the full constructor

@fedejinich fedejinich changed the title useless get() method in Trie class Test methods in Trie class Nov 11, 2021
@fedejinich fedejinich changed the title Test methods in Trie class Trie tech debts Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant