Removing array elements in a for loop

Question

I need to loop over an array (anti-pattern, I know) and delete elements if they satisfy a condition.

function withdraw(uint256 amount) external {

    WithdrawalRequest[] storage requests = withdrawalRequests[msg.sender];

    for (uint i=0; i < requests.length; i++) {

        // See below for the error this line throws
        if (bytes(requests[i]).length == 0) { continue }

        if (block.timestamp >= requests[i].withdrawableAt) {

            // do some stuff...

            // Best approach?
            delete requests[i];

        }
    }

    // do some more stuff...
}

Per this answer on removing array elements, it seems like the "Delete and Leave Gaps" would be my best approach, since I don't have to mutate the array length in the middle of the loop by moving the current element to the end and chopping it. I'd just have to check the number of bytes of requests[i]:

if (bytes(requests[i]).length == 0) { continue }

But that's also throwing me the error TypeError: Explicit type conversion not allowed from "struct Contract.WithdrawalRequest storage ref" to "bytes storage pointer".

How would you recommend removing an array element in a for loop?

score 0 · Accepted Answer · answered May 15 '20 at 09:30

0

You don't need to continue.

A deleted element simply has all its fields reset to their zero values.

For example, a uint field is reset to 0, a bool field is reset to false, and so forth.

You can still access that element (and BTW this is true for both arrays and mappings).

So you may as well just change this:

if (block.timestamp >= requests[i].withdrawableAt)

To this:

if (block.timestamp >= requests[i].withdrawableAt && requests[i].withdrawableAt > 0)

Alternatively, you can add to struct WithdrawalRequest a field bool valid, set it to true whenever you add an element to the array, and check if it is true inside that if statement.

And yes - your design IS an anti-pattern; you should consider changing it.

If you don't care about the order of elements in the array, then I would recommend that instead of deleting an element, you copy the last element in the array into the entry to be deleted, and then decrement the length of the array by 1.

answered May 15 '20 at 09:30

goodvibration

26,003
5
46
86

Wrt "copy the last element in the array into the entry to be deleted", wouldn't that element then never be iterated over? – slider May 15 '20 at 10:31
@slider: It wouldn't, indeed. You should reiterate it in such case. One way to do it is by decrementing i (your loop counter) by 1, but you should be careful with this approach, because when i == 0, it will become max uint256, and then your loop will exit. So you should probably think of a different approach. There's a general solution for this type of problems, which combines the use of a mapping and an array. There are several such answers on this website, including some of my own, for example, here. – goodvibration May 15 '20 at 10:36
Yeah, already getting disorderly. I may just consider a mapping. Thanks again! – slider May 15 '20 at 10:42

Removing array elements in a for loop

1 Answers1