What code smell best describes this code?

Question

Suppose you have this code in a class:

private DataContext _context;

public Customer[] GetCustomers() 
{
    GetContext();

    return _context.Customers.ToArray();
}

public Order[] GetOrders() 
{
    GetContext();

    return _context.Customers.ToArray();
}

// For the sake of this example, a new DataContext is *required* 
// for every public method call
private void GetContext()
{
    if (_context != null) 
    {
        _context.Dispose();
    }

    _context = new DataContext();
}

This code isn't thread-safe - if two calls to GetOrders/GetCustomers are made at the same time from different threads, they may end up using the same context, or the context could be disposed while being used. Even if this bug didn't exist, however, it still "smells" like bad code.

A much better design would be for GetContext to always return a new instance of DataContext and to get rid of the private field, and to dispose of the instance when done. Changing from an inappropriate private field to a local variable feels like a better solution.

I've looked over the code smell lists and can't find one that describes this. In the past I've thought of it as temporal coupling, but the Wikipedia description suggests that's not the term:

Temporal coupling
When two actions are bundled together into one module just because they happen to occur at the same time.

This page discusses temporal coupling, but the example is the public API of a class, while my question is about the internal design.

Does this smell have a name? Or is it simply "buggy code"?

Answer 1

I would say it is a Temporary Field. The context is created outside of the method where it is needed and stored as a field.

Answer 2

If you really must instantiate the context in every call (I dont know why you would need to do this) then make it explicit

public Customer[] GetCustomers() 
{
    using (var context = new DBContext())
    {
         return context.Customers.ToArray();
    }
}

public Order[] GetOrders() 
{
    using (var context = new DBContext())
    {
        return context.Customers.ToArray();
    }
}

That still isn't perfect code as it's tightly bound to the DBContext class and impossibly to isolate for unit testing

Answer 3

Its just buggy code.

    if (_context == null) 
    {
        _context.Dispose();
    }

If ever _context == null then you will be calling a method on a null object so it will just fall over. There is no reason to reinstanciate the datacontext for every call.