Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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
    
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
add comment

4 Answers

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.

Sometimes you see an object in which an instance variable is set only in certain circumstances. Such code is difficult to understand, because you expect an object to need all of its variables. Trying to understand why a variable is there when it doesn’t seem to be used can drive you nuts.

Use Extract Class to create a home for the poor orphan variables. Put all the code that concerns the variables into the component. You may also be able to eliminate conditional code by using Introduce Null Object to create an alternative component for when the variables aren’t valid...

share|improve this answer
add comment

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
add comment

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
add comment

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
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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