2

I'm developing a web application using Ethereum SmartContract. And I have a question.

I want to add a address to array of addresses without duplication. But I can't do without duplication. It adds all address evenif array contains same value. Below is code.

function addTokenHolder(address _tokenHolder) returns (bool success) {
  uint len = tokenHolders.length;
  for(uint i = 0; i > len; i++) {
    if (tokenHolders[i] == _tokenHolder) return false;
  }
  tokenHolders.push(_tokenHolder);
  return true;
}

Please advise.

1

What Max said about >.

In my opinion, it's usually best to shy away from for loops for this sort of thing. Each iteration will cost gas. The bigger the list, the higher the cost, so it won't scale; either because the transaction costs become unacceptable, or because the block gas limit is actually exceeded and the transaction can't run at any price.

Here's a scalable way using a mapping for random access. We're just setting a bit to note known addresses so we can avoid duplication.

This gives a flat cost for checking and inserting at any scale.

Hope it helps.

contract Unique {

    address[] public tokenHolders;

    // scalable way with no iteration

    mapping(address => bool) public tokenHolderKnown;

    function scalableAddTokenHolder(address tokenHolder) returns(bool succes) {
        if(!tokenHolderKnown[tokenHolder]) {
            tokenHolders.push(tokenHolder);
            tokenHolderKnown[tokenHolder] = true;
            return true;
        }
        return false;
    }

}

Update: Some simple storage patterns here: Blog: Simple Storage Patterns in Solidity

  • +1, using mapping to simulate set is more efficient and idiomatic. Moreover, building code around mappings often allows to extract looping and iteration from contract to client side. – max taldykin Mar 3 '17 at 17:31
  • I'm so sorry for replying you. Thank you very much. I understood well! – h.fukuda Mar 24 '17 at 10:02
0

Your loop never loops because of the looping condition.

Try to reverse condition in i > len:

for(uint i = 0; i < len; i++) {
    if (tokenHolders[i] == _tokenHolder) return false;
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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