0
struct User {
       string username;
       string password;
       address userid;
        bool isAdmin;
}

mapping (uint => User) users;

function checkIfUserExists(address userid) public returns(bool) {
    for (uint o = 0; o < totalUsers; o++) {
        if (users[o].userid == userid) {
            return true;
        } else{
            return false;
        }
}

This Always returns false?? even if userid exists in the mapping..

2
  • Your logic is flawed. You should get rid of the else and return false outside (at the end of) the loop. Also, your understanding of mapping seems completely wrong. How exactly are you hoping to iterate it the way you do??? There is obviously no point in a mapping here. You should be using an array. Commented Jun 8, 2019 at 10:32
  • 3
    To be honest, a proper implementation based on a mapping wouldn't even need a for loop. The key should be your input address userid, and you can return the result in a single attempt. For this purpose, simply add a bool isValid to your structure, and set it true whenever you add a user. Then, in your function, simply return users[userid].isValid. Commented Jun 8, 2019 at 10:39

3 Answers 3

1

following some simple style guide will make your question more easier to understand and solve!, as goodvibration in comments said, that is not the best way to store a user and check if he/she exists

struct User {
    string username;
    string password;
    address userid;
    bool isAdmin;
}

mapping (uint => User) users;

function checkIfUserExists(address _userid) public returns(bool) {

    for (uint index = 0; index < totalNumberOfUsers; index++) {
        if (users[index].userid == _userid) {
            return true;
        }
    }
    return false;
}
0

Just chiming in for the benefit of others who come across this.

@Majd is right about the logic error. Even so, there are additional errors in this.

The for loop is an anti-pattern. Its mere existence is not a problem, but the fact that there could be no upper limit to the number of users, therefore the number of iterations, therefore the gas cost of this is a well-known anti-pattern. More here: https://blog.b9lab.com/getting-loopy-with-solidity-1d51794622ad

It doesn't make sense to store a username, password and userid on the chain. This information would be visible to everyone, and it adds cost. It's not necessary to do so because Ethereum transaction signing ensures that all everyone who uses the contract is reliably authenticated.

Instead, map their addresses to user profiles.

If there is a requirement to collect additional information or grant permission to join the system, then a bit for isUser, is appropriate. In most cases that would be unwanted de-anonymization and unnecessarily intrusive. It may be perfectly fine to assume that all users exist. That would reduce the struct to a single property - some users are admins.

pragma solidity 0.5.1;

contract UserExample {

    struct UserStruct {
        // string username;   // use the address
        // string password;   // don't store this on chain
        // address userid;
        bool isUser;          // only needed if there is user initialization
        bool isAdmin;
    }

    mapping(address => UserStruct) public userStructs;

    function isUser(address user) public view returns(bool) {
        return userStructs[user].isUser;
    }
}

See also: Are there well-solved and simple storage patterns for Solidity?

Hope it helps.

-1

The following code works as you need:

pragma solidity 0.5.0;

contract UserManager {
   struct User {
      string username;
      string password;
      address userid;
      bool isAdmin;
   }

   mapping (uint => User) users;
   uint totalUsers=0;

   function checkIfUserExists(address userid) public view returns(bool) {
       for (uint o = 0; o < totalUsers; o++) {
           if (users[o].userid == userid) {
               return true;
           } else{
               return false;
           }
       }
   }

   function addUser(address userid) public {
       User memory user = User("m", "n", userid, true);

       users[totalUsers] = user;
       totalUsers++;
   }

}

I think your implementation was like below which returned false always.

pragma solidity 0.5.0;

contract UserManager {
   struct User {
      string username;
      string password;
      address userid;
      bool isAdmin;
   }

   mapping (uint => User) users;
   uint totalUsers=0;

   function checkIfUserExists(address userid) public view returns(bool) {
       for (uint o = 0; o < totalUsers; o++) {
           if (users[o].userid == userid) {
               return true;
           } else{
               return false;
           }
       }
   }

   function addUser(address userid) public {
       User memory user = User("m", "n", userid, true);
       totalUsers++;
       users[totalUsers] = user;

   }

}

As a result, in the array in index 0, there was no entry which returned false always.

Hope this will help you.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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