# Solidity for loop optimization

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
*/
uint len = arr.length;
for(uint i; i < len; ++i) {
if (arr[i] == token) {
arr[i] = arr[arr.length - 1];
arr.pop();
break;
}
}
}
```
``````
• why are addresses store inside an array and not a map? Oct 10, 2022 at 14:56

• 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.

 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.

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.

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