2
DataTable dt= new Datatable();
try {
    SqlCommand Cmd = new SqlCommand("sp_getData",SqlCon);
    SqlCommand.CommandType= CommandType.StroedProcedure;
    SqlCon.Open();
    sqlDataReader dr=  cmd.ExecuteReader();
    dt.Load(dr);
    SqlCon.Close();
}
catch(Exception ex) {
}
finally{
    dt.Dispose() //
}

return dt;

Is this code legitimate ?... My method is returning that datatable object , so after calling dispose will that retain the value ??.. please explain..

Dmitriy Khaykin
  • 5,238
  • 1
  • 20
  • 32
  • 1
    Did you try to get as many problems as possible in that piece of code? Please read up on `IDisposable` and `using blocks`. You are leaving your locals undisposed and you dispose the only thing you are returning. – nvoigt Feb 19 '14 at 07:10
  • anyone out der ??.. Please pour in your seuggestions.... – Sreejith Unnikrishnan Feb 19 '14 at 07:11
  • This is not legitimate. See this pose - http://stackoverflow.com/questions/18869079/datatable-dispose-will-make-it-remove-from-memory – Shakti Prakash Singh Feb 19 '14 at 07:33
  • Do one more change! Put your SqlCon.Close() inside the finally, because if you get some error for example with the ExecuteReader, your connection will not close. – Oscar Bralo Feb 19 '14 at 07:45

3 Answers3

4

No. You will be returning an already-disposed object.

If you are returning an IDisposable it is perfectly normal to expect it to be the caller's responsibility to dispose of the object when they are through with it. Therefore, you should return the DataTable object as is, undisposed, and let the calling code dispose of it.

Normally this would be done with a using block. For example:

class MyDisposableObject : IDisposable { /*...*/ }

public MyDisposableObject MakeMeAnObject() { return new MyDisposableObject(); }

public void Main()
{
    using(var o = MakeMeAnObject())
    {
        o.Foo = 1;
        o.Bar = 2;
        o.FooBar();
    }
}

Note I do see some local IDisposable objects in your snippet that you are not disposing but should be. You are also swallowing exceptions.

lc.
  • 113,939
  • 20
  • 158
  • 187
3

this will give you what you want:

    public DataTable getDataTable()
    {
        using (SqlConnection sqlCon = new SqlConnection("connectionString"))
        using (SqlCommand cmd = new SqlCommand("sp_getData", sqlCon))
        {
            try
            {
                cmd.CommandType = CommandType.StoredProcedure;
                sqlCon.Open();
                using (SqlDataReader dr = cmd.ExecuteReader())
                {
                    DataTable dt = new DataTable();
                    dt.Load(dr);
                    return dt;
                }
            }
            catch (Exception ex)
            {
                MessageBox.Show(ex.Message);
            }
            return null;
        }
    }
azpc
  • 500
  • 9
  • 11
1

This will return a disposed object which isn't what you want.

One thing you can do is pass a delegate, which will allow you to work on the DataTablr without returning it, and still dispose within your original data method.

Pseudo-code:

public void GetSomeData(Action<DataTable> processDataTableAction)
{
    try
    { 
        ... ( get the data )
        processDataTableAction(dt);
    }
    catch
    {
        // handle exceptions
    }
    finally
    {
        dt.Dispose();
    }
}

Then to call the function elsewhere in code:

GetSomeData(dt => {
    // do stuff with dt
});
Dmitriy Khaykin
  • 5,238
  • 1
  • 20
  • 32