What code smell best describes this code?
Posted
by
Paul Stovell
on Programmers
See other posts from Programmers
or by Paul Stovell
Published on 2012-03-13T08:57:36Z
Indexed on
2013/10/27
16:00 UTC
Read the original article
Hit count: 768
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"?
© Programmers or respective owner