A Person class containing a list of Persons What do you guys think about this? I myself think it is pretty usefull, But how does it look from an OOP perspective?

Here's an example class

public class Person
{
    private string firstName, lastName;
    private int id;
    private static int statId;
    private static List&lt;Person&gt; persons = new List&lt;Person&gt;();
    public Person(string firstName,string lastName)
    {
        this.firstName = firstName;
        this.lastName = lastName;

        //Like this, everytime a new object of Person is created, the ID will be the id of the last created object +1
        id = statId;
        statId++;

        //Everytime you create a new instance of this class, it will be added to the list
        persons.Add(this);

    }
    public static Person GetPersonById(int id)
    {
        foreach(Person per in persons)
        {
            if (per.ID == id)
                return per;
        }
        return null;
    }
    public int ID
    {
        get { return id; }
    }

    public static List&lt;Person&gt; GetPersons()
    {
        return persons;
    }    
    public string FirstName
    {
        get { return firstName; }
        set { firstName = value; }
    }
    public string LastName
    {
        get { return lastName; }
        set { lastName = value; }
    }
}


Now, aside from being able to use this class to store persons, you can also use some methods like GetPersonById and GetPersons / the PersonList property without instantiating it, and whenever you do instantiate it to make a new person, it automatically adds it to the list
But how does it look from an OOP perspective?


Not very good because it combines multiple responsibilities in a single class. 


the one of a data object as the Person is clear, I guess. The other two are
the repository represented by



private static List&lt;Person&gt; persons = new List&lt;Person&gt;();

public static Person GetPersonById(int id)
{
    foreach(Person per in persons)
    {
        if (per.ID == id)
            return per;
    }
    return null;
}

public static List&lt;Person&gt; GetPersons()
{
    return persons;
}




and the factory represented by



private static int statId;
statId++;



This means that the Person should actually be three classes.

On top of all this you cannot test it because the nested factory is a static one and if you try to write a test validating the statId++ you couldn't because you cannot verfiy that it'll do +1 to the last id because if any other test executes at the same time the result might actually be +2 or even more. You also cannot verify that it correctly adds the new instance to the list because agian, if any tests execute at the same time you could have more then one object added to the list.




id = statId;
statId++;



I find this is not how the id increment should be implemented. statId should not be the next index but the last one. What if you decide to add a property like LastId? You'd have to calculate it first with statId - 1 - ugly. Instead use the prefix ++ like id = ++statId and start with int statId = -1; that is usually considered as na invalid index in many APIs. Then the LastId property could just be LastId =&gt; statId;.