What code smell best describes this code?
- by Paul Stovell
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"?
