Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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

share|improve this question

closed as primarily opinion-based by MichaelT, durron597, Snowman, gnat, Ixrec Sep 18 '15 at 19:10

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.If this question can be reworded to fit the rules in the help center, please edit the question.

    
in getContext method, did you mean to check for _context == null to dispose? I would expect an opposite - checking for not-null prior to disposing – gnat Mar 13 '12 at 9:24
    
What is this code trying to achieve? – Dipan Mehta Mar 13 '12 at 9:24
    
FWIW, those two definitions of temporal coupling are at odds. I have always thought temporal coupling was defined as described in the ploeh (Mark Seeman) link (I realize that has nothing to do with your question per se . . .). – Phil Sandler Oct 28 '13 at 1:46
    
Does this smell have a name? Or is it simply "buggy code"? I think I would just call it buggy code. It almost a race condition, but I'm not sure I would call it that either. – Phil Sandler Oct 28 '13 at 1:50

I don't know of a catchy name for it, but I'd say that that code "relies on side-effects" - GetContext() isn't actually getting anything, it's changing something way off over there that affects how subsequent code will run.

That said, this code smell can evolve into an antipattern called sequential coupling. The difference is that the antipattern usually applies to calls from outside the object, rather than inside.

Sequential coupling usually manifests more like this:

obj = Cls()
obj.Start()
obj.LoadParams()
obj.InitEviron()
obj.Run()

where running these in a different order (or omitting one) would produce unexpected results.

share|improve this answer

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

share|improve this answer

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.

share|improve this answer
    
Thanks, that was a bug in the sample that I've fixed. Is it "perfect code" now that the == is a !=? – Paul Stovell Mar 13 '12 at 9:17
1  
No. re-instantiating the datacontext in every call is wastefull. At least it wont fall over now though – Tom Squires Mar 13 '12 at 9:21
    
In this example (for the sake of the example) creating a new DataContext every call is a requirement. – Paul Stovell Mar 13 '12 at 9:27

Not the answer you're looking for? Browse other questions tagged or ask your own question.