Is it ok to use C# Property like this...

Answer 1

Properties should really be as lightweight as possible.

When other developers are using properties, they expect them to be intrinsic parts of the object (that is, already loaded and in memory).

The real issue here is that of symmetry - the property get and set should mirror each other, and they don't. This is against what most developers would normally expect.

Having the property load up from database is not recommended - normally one would populate the class via a specific method.

Answer 2

In my opinion, that's an awful design. Using a property getter to do some "magic" stuff makes the system awkward to maintain. If I would join your team, how should I know that magic behind those properties?

Create a separate method that is called as it behaves.

Greets Flo

Answer 3

"Correct" is often in the eye of the beholder. It also depends how far or how brilliant you want your design to be. I'd never go for the design you describe, it'll become a maintenance nightmare to have the CRUD actions on the POCOs.

Your main issue is the absense of separations of concerns. I.e., The data-object is also responsible for storing and retrieving (actions that need to be defined only once in the whole system). As a result, you end up with duplicated, bloated and unmaintainable code that may quickly become real slow (try a LINQ query with a join on the gettor).

A common scenario with databases is to use small entity classes that only contain the properties, nothing more. A DAO layer takes care of retrieving and filling these POCOs with data from the database and defined the CRUD actions only ones (through some generics). I'd suggest NHibernate for the ORM mapping. The basic principle explained here works with other ORM mappers too and is explained here.

The reasons, esp. nr 1, should be a main candidate for refactoring this into something more maintainable. Duplicated code and logic, when encountered, should be reconsidered strongly. If the gettor above is really getting the database data (I hope I misunderstand that), get rid of it as quickly as you can.

Overly simplified example of separations of concerns:

class Data 
{
    public string Prop1 {get; set;}
    public string Prop2 {get; set;}
}

class Dao<T>
{
    SaveEntity<T>(T data)
    {
        // use reflection for saving your properies (this is what any ORM does for you)
    }
    IList<T> GetAll<T>()
    {
        // use reflection to retrieve all data of this type (again, ORM does this for you)
    }
}

// usage:
Dao<Data> myDao = new Dao<Data>();
List<Data> allData = myDao.GetAll();
// modify, query etc using Dao, lazy evaluation and caching is done by the ORM for performance
// but more importantly, this design keeps your code clean, readable and maintainable.

EDIT:
One question you should ask your co-worker: what happens if you have many Data (rows in database), or when a property is a result of a joined query (foreign key table). Have a look at Fluent NHibernate if you want a smooth transition from one situation (unmaintainable) to another (maintainable) that's easy enough to understand by anybody.

Answer 4

I don't think that another developer who will work with the same code will be happy to see :

d.Prop1 = d.Prop1; 

Personally I would never do that.

Also it is not the best idea to use property to load data from DB. I would have method which will load data from DB to local variable and then you can get that data using property. Also get/set logically must work with the same data. It is strange to use get for getting data from DB but to use set to work with local variable.

Answer 5

This is pretty terrible, imo.

1. Properties are supposed to be quick / easy to access; if there's really heavy stuff going on behind a property it should probably be a method instead.
2. Having two utterly different things going on behind the same property's getter and setter is very confusing. d.Prop1 = d.Prop1 looks like a meaningless self-assignment, not a "Load data from DB" call.
3. Even if you do have to load twenty different things from a database, doing it this way forces it to be twenty different DB trips; are you sure multiple properties can't be fetched in a single call? That would likely be much better, performance-wise.

Answer 6

If I were you I would write a serialize / deserialize function, then provide properties as lightweight wrappers around the in-memory results.

Take a look at the ISerialization interface: http://msdn.microsoft.com/en-us/library/system.runtime.serialization.iserializable.aspx

Answer 7

This would be very hard to work with,

If you set the Prop1, and then get Prop1, you could end up with different results eg:

//set Prop1 to "abc"
d.Prop1 = "abc";

//if the data source holds "xyz" for Prop1
string myString = d.Prop1;
//myString will equal "xyz"

reading the code without the comment you would expect mystring to equal "abc" not "xyz", this could be confusing.

This would make working with the properties very difficult and require a save every time you change a property for it to work.

Answer 8

As well as agreeing with what everyone else has said on this example, what happens if there are other fields in the Data class? i.e. Prop2, Prop3 etc, do they all go back to the database, each time they are accessed in order to "return the value stored in the database via a query". 10 properties would equal 10 database hits. Setting 10 properties, 10 writes to the database. That's not going to scale.