Constructor - Noobie Question

[»Barney«]

Hello everyone, I am not a professional developer but I am working on it.

I need your help, please.

I have this code, which is the constructor of the User class.

 Spoiler

public User(string username, string password)
        {
            if (IsValidUsername(username)) 
                this.UserName = username;
            else throw new UsernameException();

            this.Password = password;

            int uid = new Random().Next(500);
            foreach (User u in users)
                if (u.UserID == uid)
                    uid = new Random().Next(500);
            
            this.UserID = (uint) uid;

            users.Add(this);
        }

Look at the last line.
users.Add(this);

users is a list<user> [public static List<User> users = new List<User>();]

by using 'this', I want to say the value created by the constructor.

Is it ok like this, or is it really really bad?

[Schlüsselbein]

Is it ok like this, or is it really really bad?

Imho, its really bad practice. Why would a class contain Objects of itself? Create a class called Userbase or so which holds user and information about them.

Another thing i noticed:

int uid = new Random().Next(500);
foreach (User u in users)
if (u.UserID == uid)
uid = new Random().Next(500);

uid should be a unique identifier? Its not guaranteed to be unique when doing it like this.
Example:
user1 id: 15
user2 id: 12
user3 id: 19
user4 id: 07
first generated uid = 12
iterating over list:
compate with uid of first user -> ok
compate with uid of second user -> match -> not ok!
new generated uid = 15
compare with uid of third user -> ok
compare with uid of 4th user -> ok
-> everything seems fine, but uid of user1 equals the newly generated uid.

[»Barney«]

Don't worry, I will change the way to create uid's, it was not final, I just wanted it to work for now. I still have a lot of work to do.

[»jD«]

You can't do that, otherwise you could end up in a endless loop. By design, constructors are called when allocating a class to a variable (eg. var blah = new User()) so the variable (blah) would contain your new User instance. Why not add it to "users" from the code that creates the instance?

EG.

User _user = new User(username, password);
users.Add(_user);

-jD