Is excessive DataTable usage bad?

Question

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 good 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"?

Answer 1

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.

Answer 2

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.