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.

h.fukuda
  • 39
  • 3

3 Answers3

2

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

Community
  • 1
Rob Hitchens
  • 55,151
  • 11
  • 89
  • 145
  • +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 03 '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;
}
max taldykin
  • 2,966
  • 19
  • 27
0

Use the following code instead !

function addTokenHolder(address _tokenHolder) returns (bool success) {
  uint len = tokenHolders.length;
  for(uint i = 0; i < len; ) {
    if (tokenHolders[i] =! _tokenHolder) {
       revert("already exist");
     }
     else {
        tokenHolders.push(_tokenHolder);
      }
  }

return true;
}
Antonio Carito
  • 2,445
  • 3
  • 10
  • 23
A.MARSHALL
  • 1