Deleting index of an array in terms of optimization for looping

Question

I have a dapp where an admin can upload a number of cakes (20 to 30). People can come into the site and decide whether they want to buy a cake uploaded by admin.

So for smart contract, I made an array of uint to store each id of Cake whenever admin uploads. Once the buyer bought one of the cakes, it would delete that id of Cake in the array. I decided to delete it to avoid loops as much as possible since it would cause out of gas while I iterate the array to return the remaining list of cakes to buyers. How does it sound in terms of optimization?

Below is my implementation. Are there any better solutions or is this enough to continue?

struct Cake {
    uint id;
    address buyer;
}

mapping (uint => Cake) public cakes;
uint[] public cakeIds;

function addNewCake(uint _id) public {
    cakes[_id] = Cake(_id, 0x0);

    cakeIds.push(_id);
}

function buyCake(uint _id) public {      
    Cake storage cake = cakes[_id];       
    cake.buyer = msg.sender;

    removeCakeInArray(_id);
}

function removeCakeInArray(uint _id) private {
    for (uint i = 0; i <= getNumOfCakes(); i++) {            
        if (cakeIds[i] == _id) {
            remove(i);                 
        }    
    }
}

function remove(uint index) private {
    if (index >= getNumOfCakes()) return;

    for (uint i = index; i < getNumOfCakes() - 1; i++){
        cakeIds[i] = cakeIds[i+1];
    }
    cakeIds.length--;
}

function getAllUnsoldCakes() public view returns (uint[]) {
    uint length = getNumOfCakes();  
    uint[] memory ids = new uint[](length);

    for (uint i = 0; i < length; i++) {       
        uint cakeId = cakeIds[i];
        Cake memory cake = cakes[cakeId];

        if (cake.buyer == 0x0) {
            ids[i] = cake.id;             
        }
    }

    return ids;
}

function getNumOfCakes() public view returns (uint) {
    return cakeIds.length;
}

Answer 1

Seconding Edmond's concerns about the loops. In my opinion, the need to enumerate the list is questionable. The contract could work on an append-only basis with a simple flag to indicate "sold" and events that chronical additions and removals. Clients would then be able to compute, for themselves, which cakes remain available for sale and the contract would merely enforce a rule that no cake can be sold twice. Similarly, ordering lists can usually be external to storage contracts.

In the case that another contract might be interested in successfully navigating the list, you might need to make this information efficiently discoverable without reliance on events. This would include the "delete" concern.

In that case, consider the Mapped Struct with Delete pattern described over here: Are there well-solved and simple storage patterns for Solidity? with an elaborated description over here: https://medium.com/@robhitchens/solidity-crud-part-1-824ffa69509a

Hope it helps.

Answer 2

These loops, particularly in remove(), are a red flag: Your array can grow to an unbounded length, including if someone increases it maliciously, at which your contract will hit gas limits and become unusable. This is only OK if you can put explicit bounds on the length of the list - for example if someone tries to add an 11th unsold cake, revert the transaction, because the cake shop shelves are full.

If you really need to be able to retrieve a list of unsold cakes with a solidity function call, a safer approach is to use a linked list, as this can be updated for the same cost regardless of the length of the list. This still gives you a gas cost to worry about when you read the list, but that may be OK if it's only going to be used reading data from a node, rather than being called as part of a transaction.

However, do you really need to be able to get a list of unsold cakes with a contract function call? It may be better to transfer that work elsewhere - for example, log events when you add or sell a cake, then read those events from your dapp javascript and use that to build the list you display to the user.