Is excessive DataTable usage bad?

I was recently asked to assist another team in building an ASP .NET website. They already have a significant amount of code written -- I was specifically asked build a few individual pages for the site.

While exploring the code for the rest of the site, the amount of DataTables being constructed jumped out at me. Being a relatively new in the field, I've never worked on an application that utilizes a database as much as this site does, so I'm not sure how common this is. It seems that whenever data is queried from our database, the results are stored in a DataTable. This DataTable is then usually passed around by itself, or it's passed to a constructor. Classes that are initialized with a DataTable always assign the DataTable to a private/protected field, however only a few of these classes implement IDisposable. In fact, in the thousands of lines of code that I've browsed so far, I have yet to see the Dispose method called on a DataTable.

If anything, this doesn't seem to be goo开发者_如何学JAVAd OOP. Is this something that I should worry about? Or am I just paying more attention to detail than I should? Assuming you're most experienced developers than I am, how would you feel or react if someone who was just assigned to help you with your site approached you about this "problem"?

Datatables can be used for good and evil.

Acceptable Use

I would find the following to be an acceptable use of a datatable or a datarow:

public class User
{
    private DataRow Row { get; set; };
    public User(DataRow row) { this.Row = row; }

    public string UserName { get { return (string)Row["Username"]; } }
    public int UserID { get { return (int)Row["UserID"]; } }
    public bool IsAdmin { get { return (bool)Row["IsAdmin"]; } }
    // ...
}

The class above is ok because it maps a DataRow to a typesafe class. Instead of working with strings and untyped datarows, now you have real datatypes and intellisense to assist you. Additionally, if your database schema changes, you can modify a column name in your object, instead of modifying the column name everywhere its used. Finally, you can map ugly column names like "dtaccount_created" to a property named "AccountCreated".

Of course, there's really no good reason to write this wrapper class, since Visual Studio will automatically generate typed datasets for you. Or, as an alternative, a good ORM like NHibernate allows you to define classes similar to the one above.

Whether you you should use plain old ADO.NET, typed datasets, or a full fledged ORM depends on the requirements and complexity of your application. Its hard to say whether your team is doing the right thing withotu actually seeing some sample code.

Additionally, I occasionally find it useful to databind lists and grids with a datatable, because changes to the underlying datarow will automatically cause the GUI to refresh. If you create your own type-safe wrapper, then you need to implement the IPropertyChanging and IPropertyChanged interfaces manually.

Unacceptable Use

Unfortunately, I've seen programmers use datatables for ad hoc containers, alternatives to classes, etc. If you see your team doing this, throw rocks at them. That style of programming just doesn't work in a statically typed language, and its going to make development a nightmare.

Main problem with datatables: they aren't typed, so you can't do anything useful with them without giving them a string and casting whatever mystery object they contain into the right type. Additionally, refactoring a column name is nearly impossible to automate since they are based on strings, so you can't rely on intellisense to help you write correct code, and you can't catch errors at compile time.

I say trust your instinct: if you think design is flakey, it probably is.

This is definitely something you should worry about - see a related post on the importance of Disposing DataTables.

DataTables are finalizable: if you're not actively disposing them, they're hanging around much longer than Gen0 collections and killing memory.

To measure the extent of damage in your application, you can take a memory dump using WinDbg and look at the sheer number of DataTable instances (!dumpheap -stat -type System.Data.DataTable) then look at the largest data tables in memory.

This is a common pitfall in ASP.NET applications, and one that can land you in serious trouble. If you're using shared (cached) instances of DataTables, take care to note that view filters change the original instance, they don't generate a new copy.

Also make sure that queries populating the DataTables have some reasonable limit on the number of rows returned, otherwise changes to your data can suddenly baloon memory and destabilize your application pool.

At a very high level, Software system architecture can be characterized as using one of several "Enterprise Level Patterns", Transaction script, Table Model, Domain Model, or Service Layer. If the system you are reviewing is using the Table Model pattern, then you would expect to see more use of DataTables and DataSets than in, say, a system that was designed using Domain Model or one of the other patterns.

However, as software system design methodologies have evolved over the past few years, it has generally been understood that complex systems do not fare well using Transaction script or Table Model architectures. This is generally because in systems designed using these patterns, functionality is generally much more intertwined, and interrelated, and as complexity grows, the amount of functional, or module interdependence grows exponentially, and becomes too hard to manage very quickly. So, depending on how complex your particular system is, yes, you should be suspicious if DataSets and/or DataTables are used throughout multiple layers of the system. This may be a sign that the system designer was/is using Table Model (consciously or unconsciously) where he/she should be using Domain Model or Service Layer architecture.

yes I would be careful here...

I had to look after a vb.net web app for about 2 months before I could re-write it all in C# .... I love C#, VB makes me want to hurl...

Anyway, in the old app the previous developer had loaded data from the database into a datatable and then passed the datatable around a few methods which did absolutely nothing to the datatable, only for it to be assigned to a gridview. I was in utter disbelief.

To make matters worse, there were times where he would actually dump the DataTable into a session... for no reason at all.

DataTables etc are great, but only use them if you 'really' need to use them. The developer was soo bad that on search page he actually dumped all 5000 products from the database into a datatable and then performed a search on the datatable instead of performing the search in a stored procedure (ie. on SQL SERVER)

Using a DataTable can be a lazy/inefficient way of storing data. There is significant overhead in doing so. You are right to be concerned, although the developer(s) may have a real issue hearing how poorly they have designed this app. Will management be behind you, in your goal of creating a better quality product? Will the associated delay in development be something they can accept?