Is it ok to use reflection to get property names and values in a base class?

Question

I am using the following code to save an object and all its properties to a db. It saves me from having to create a save method in every business object. The code in my base class is:

    public void Save()
    {
        List<SqlParameter> Params = new List<SqlParameter>();
        foreach (PropertyInfo Property in this.GetType().GetProperties())
        {
            if (Property.CanRead)                
                Params.Add(new SqlParameter(Property.Name,Property.GetValue(this,null)));                
        }
        Execute<int>(SaveProcedure, Params.ToArray());        
    }

Is this a good practice or would I be better off not using reflection and creating a save method in each object I create?

Any other ideas or suggestions are appreciated, thanks!

Answer 1

It seems like you are creating your own basic ORM. Have you considered using any of the fully-fledged mappers specifically written for the .NET framework to perform this task, such as LinqToSql or NHibernate?

Answer 2

As someone whose work relies heavily on reflection, I don't think it is inappropriate. HOWEVER, you should consider its trade-off carefully. Using Reflection opt you out from compile time check and that means no early error detection. So if you really think reflection helps you better manage of code and you don't mind its performance, using reflection should not be a problem.

Another note, everytime when using reflection ensure you create enough tests to help you detect problem early.

Just a though.

Answer 3

Reflection is really only intended for program analysis. Using it has pretty serious side effects for performance and provability of your program. That doesn't mean there aren't cases where reflection is appropriate in production application, but I don't think that this is one of those cases. For storage in a database, I recommend looking up serialization.

Answer 4

From a design point of view i think it's more flexible to have a seperate save for each class. This will allow to choose the properties you same and allow for necessary preprocessing before you store to the DB (i.e. a complex object itself cannot be save.

Answer 5

I would generally classify that as a bad, or at least poor, practice. Reflection cost comes in several degrees, the highest cost of which is invocation (i.e. call method, get or set property/field value, etc.) By saving your object via reflection like that, you are incurring a very high cost. Were talking several orders of magnitude slower than directly accessing the properties.

Reflection cost aside, this is also generally bad practice from an intent standpoint. You are saving ALL properties...but what happens when someone creates a derived type that has properties that do not map to a database table column? You should make your save method virtual or abstract, and implement/extend the method as needed for each entity.

On a final note...handling saving directly on an entity like that is a true maintenance nightmare just waiting to happen. You are asking for a HELL of a lot of trouble as you are mixing concerns and responsibilities into a single class. You would be much better off making your entities/business objects POCO (plain old clr objects), and writing explicit data mappers. This will separate your concerns, and reduce the responsibilities of each class to as few as possible. Rich, self-saving (and loading) entities are convenient, but mostly a dream. In practice, they are usually not worth the small benefit they offer in light of the much greater maintenance difficulties you will have.

I recommend looking into OR mappers (LINQ to SQL, nHibernate, Entity Framework v4, etc.) OR mappers automatically generate SQL for you when you need to load, save, or delete entities. They can eliminate a considerable amount of extra coding required when you don't use an OR mapper, and support a much more adaptable code base.