Is it considered bad practice to declare variables within a property?

I have the following class:

public class PeopleInfo
{
   public virtual int ID {get; protected set;}
   public virtual Person Person1 {get;set;}
   public virtual Person Person2 {get;set;}

   public virtual List<Person> People
   {
     get
     {
        var p = new List<Person>();
        p.Add(Person1);
        p.Add(Person2);
        return p;
     }
   }
}

Which I'm using with NHibernate. The Person class is being used as a component because the "PeopleInfo" table has more than 1 person in each row. The idea behind the Pe开发者_运维问答ople() property is to provide a read only list that can be looped over. Is there a better way to do this or is this solution considered acceptable?

---

The idea behind the People property is to provide a read only list that can be looped over. Is there a better way to do this or is this solution considered acceptable?

If that is your intention then you have not achieved it; you have provided a mutable list that can be looped over.

Fortunately you provide a different mutable list every time, but you are still providing a mutable list.

I would be inclined to actually provide an immutable list. There are a number of ways to do so. If you actually provide an immutable list then you have the additional benefit that the list can be computed lazily and then cached and re-used indefinitely rather than re-built every time you ask for it.

If you require indexed access then I would make a ReadOnlyCollection and wrap it around a single instance of the list, and then cache and re-use the read-only collection. Note that if you mutate the underlying list, the read-only collection will appear to mutate; it is only a read only list, it is not an immutable list.

If you do not require indexed access then I would indicate that by returning IEnumerable<T> instead of List<T>. You can then return any immutable collection of your choice.

---

If it's readonly, do you need it to be a List ? You could declare it IEnumerable and do:

public virtual IEnumerable<Person> People 
{
    get 
    {
         yield return Person1;
         yield return Person2;
    }
}

---

Properties are generally assumed to return immediately, by convention. Semantically, they are used to represent some sort of 'state' of the object.

You certainly shouldn't go performing any long calculations in properties. Hence, it depends highly on the context whether it's appropriate to declare variables. In general, there's no problem with it, but dynamically generating lists is probably not recommended due to its computational intensity.

MSDN also has a useful page for developers considering using Properties vs. Methods. To paraphrase,

In general, methods represent actions and properties represent data.

---

Is there a better way to do this or is this solution considered acceptable?

Yes, there is a better way: do it right.

public class PeopleInfo {
    public virtual int Id { get; set; }
    public virtual IList<Person> People { get; set; }
}

public class Person {
    public virtual int Id { get; set; }
    public virtual string Name { get; set; }
    public virtual PeopleInfo PeopleInfo { get; set; }
}

public class PeopleInfoMap : ClassMap<PeopleInfo> {
    public PeopleInfoMap() {
        Id(x => x.Id);
        HasMany(x => x.People)
            .Cascade.None()
            .Inverse();
    }
}

public class PersonMap : ClassMap<Person> {
    public PersonMap() {
        Id(x => x.Id);
        Map(x => x.Name);
        References(x => x.PeopleInfo);
    }
}

Your database tables should look like:

PeopleInfo (Id PK)
Person (Id PK, Name, PeopleInfoId FK PeopleInfo.Id)

---

Nothing wrong with it, but your example will return a new instance with each get. I'm not sure if that is what you intend.

---

Declaring a local variable is fine. However, there is a way to shorten your code:

public virtual List<Person> People
{
    get
    {
        return new List<Person> { Person1, Person2 };
    }
}