1

I am really struggling to think of a way to optimize this code. Can this be one more efficiently?

    /**
        @dev Utility function to remove a given address from a list of addresses
        @param arr A list of addresses
        @param token Address to remove
    */
    function _remove(address[] storage arr, address token) internal {
        uint len = arr.length;
        for(uint i; i < len; ++i) {
            if (arr[i] == token) {
                arr[i] = arr[arr.length - 1];
                arr.pop();
                break;
            }
        }
    }
    ```
1
  • 2
    why are addresses store inside an array and not a map?
    – Julissa DC
    Commented Oct 10, 2022 at 14:56

3 Answers 3

2
  • You're loading len into the memory to save gas, you might as well use it here: arr[i] = arr[len - 1];
  • You can uncheck the ++i operation, which won't overflow anyway: {++i}

Source

Other than this, the code seems quite efficient already.

[Edit] Some people suggest using a mapping. First, it really depends on the context (which we don't have here, as an array may serve other purposes in the contract). Second, arrays in solidity behave similarly to mappings, albeit with some different functionality. You can see this answer if you're interested.

0

That approach looks really good and efficient since it doesn't seem to be anything else that you can do to optimize it.

You need to remove an element from a storage array, but first, you need to find it, you don't have any more options than to do a linear search to find it first. You could get more complicated and use a mapping to hold the index of each token in the storage array to find it in constant time next time, but that would be more complicated and it will still cost a bit. So you would need to weigh your options and calculate to see if in your case it would be more efficient to go with this other approach.

Otherwise, your approach looks reasonable. Unless you can actually put the tokens themselves in a mapping, so in case you need to get/delete them you can do it in constant time.

One more tip:

  1. Finding the token by doing a linear search. The worst case here is that the token is not even there, so you lose a lot of processing by reading from storage. If the element is in the last position, no need to 'move' it using arr[i] = arr[arr.length - 1];, but simply pop it.
0

I am not sure of the exact end goal here. But I am sure the for loop is a 99% wrong choice.

I would strongly advise reading on iterable mapping. (If you really need to use an array of addresses) https://medium.com/rayonprotocol/creating-a-smart-contract-having-iterab

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.