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.

In some code I'm reviewing, I'm seeing stuff that's the moral equivalent of the following:

public class Foo
{
    private Bar bar;

    public MethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public MethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

The field bar here is logically a local variable, as its value is never intended to persist across method calls. However, since many of the methods in Foo need an object of type Bar, the original code author has just made a field of type Bar.

  1. This is obviously bad, right?
  2. Is there a name for this antipattern?
share|improve this question

closed as primarily opinion-based by gnat, MichaelT, Bart van Ingen Schenau, ratchet freak, GlenH7 Sep 8 at 14:06

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.

57  
Well, that's a new one. Hope this class isn't used in a multithreaded context, or you'll have interesting times ahead! –  TMN Aug 31 '12 at 14:32
7  
This is really uncommon? I've seen this many times in my career. –  JSBձոգչ Aug 31 '12 at 14:36
28  
@TMN It isn't thread safe, but what's worse, it isn't even reentrant. If any code in MethodA or MethodB cause MethodA or MethodB to be called in any way (and bar is used again, in the case of the bar.{A,B}() being the offender) you have similar issues even without any concurrency. –  delnan Aug 31 '12 at 14:52
71  
Do we have to have a name for every dumb thing somebody could do? Just call it a Bad Idea. –  Caleb Aug 31 '12 at 15:25
70  
Hard not to call it the dangling privates anti-pattern. –  psr Aug 31 '12 at 17:51

14 Answers 14

This is obviously bad, right?

Yea.

  • It makes the methods non-reentrant, which is a problem if they are called on the same instance recursively or in a multi-threaded context.

  • It means that state from one call leaks to another call (if you forget to reinitialize).

  • It makes the code hard to understand because you have to check for the above to be sure what the code is actually doing. (Contrast with using local variables.)

  • It makes each Foo instance bigger than it needs to be. (And imagine doing this for N variables ...)

Is there a name for this antipattern?

IMO, this is does not deserve to be called an antipattern. It is just a bad coding habit / a misuse of Java constructs / crap code. It is the sort of thing that you might see in an undergraduate's code when the student has been skipping lectures and/or has no aptitude for programming. If you see it in production code, it is a sign that you need to do a lot more code reviews ...


For the record, the correct way to write this is:

public class Foo
{
    public methodA()
    {
        Bar bar = new Bar();  // Use a >>local<< variable!!
        bar.a();
    }

    // Or more concisely (in this case) ...
    public methodB()
    {
        new Bar().b();
    }
}

Notice that I've also fixed the method and variable names to conform to the accepted style rules for Java identifiers.

share|improve this answer
11  
I would say "If you see it in production code, it is a sign that you should reexamine your hiring process" –  Beta Sep 2 '12 at 3:59
    
@Beta - that too, though you should be able to correct bad coding habits like that with vigorous code-reviewing. –  Stephen C Sep 2 '12 at 5:53
    
+1 for the bit about more code reviews. Despite what people think, improving hiring practice will not prevent this kind of problem - hiring is a crap-shoot, the best you can do is load the dice in your favor. –  mattnz Sep 6 '12 at 22:48
    
@StephenC "It makes the methods non-reentrant, which is a problem if they are called on the same instance recursively or in a multi-threaded context." . I know what reentrancy is but can not see a point for this here since the methods aren't using any locks ? Can you explain please . –  Geek Sep 7 '12 at 7:43
    
@Geek - You are focussing too much on the specific example. I am talking about the general case where the method might be called from multiple threads, or recursively. –  Stephen C Sep 7 '12 at 23:52

I would call it unnecessary large scope for a variable. This setting also allows for race conditions if multiple threads access MethodA and MethodB.

share|improve this answer
12  
Specifically, I think "variable scope" is the name of the issue here. This person needs to learn what local variables are and how/when to use them. The positive pattern or general rule is to use the smallest scope available to you for each variable and only widen it as needed. To be clear, this error is not just bad style. Coding a race condition like this is an error. –  GlenPeterson Aug 31 '12 at 15:52

This is a specific case of a general pattern known as global doorknobbing. When building a house, it's tempting to buy only one doorknob and leave it lying around. When someone wants to use a door or cupboard, they just grab that one global doorknob. If doorknobs are expensive, it can be a good design.

Unfortunately, when your friend comes over and the doorknob is simultaneously used in two different places, reality shatters. If the doorknob is so expensive that you can only afford one, then it's worth it to build a system that safely allows people to wait their turn for the doorknob.

In this case the doorknob is just a reference, so it's cheap. If the doorknob was an actual object (and they were reusing the object instead of just the reference), then it might be expensive enough to be a prudent global doorknob. However, when it is cheap, it is known as a cheapskate global doorknob.

Your example is of the cheapskate variety.

share|improve this answer

The primary concern here would be concurrency--If the Foo is being used by multiple threads, you have a problem.

Beyond that, it's just silly--local variables manage their own life cycles perfectly well. Replacing them with instance variables that need to be nullified when they are no longer useful is an invitation to error.

share|improve this answer

It's a specific case of "improper scoping", with a side of "variable reuse".

share|improve this answer

It's not an anti-pattern. Anti-patterns have some property that makes it seem like a good idea, which leads people to do it on purpose; they're planned as patterns and then it goes terribly wrong.

It's also what makes for debates as to whether something is a pattern, an anti-pattern, or a commonly mis-applied pattern that does still have uses in some places.

This is just wrong.

To add a bit more.

This code is superstitious, or at best a cargo-cult practice.

A superstition is something done without a clear justification. It may be related to something real, but the connection is not logical.

A cargo-cult practice is one where you try to copy something that you've learnt from a more knowledgeable source, but you are actually copying the surface artefacts rather than the process (so-named for a cult in Papua New Guinea who would make aircraft control radios out of bamboo hoping to make the WWII Japanese and American planes come back).

In both of these cases there isn't any real case to be made.

An anti-pattern is an attempt at a reasonable improvement, whether in the small (that extra branching to deal with that extra case that has to be dealt with, that leads to spaghetti code) or in the large where you very deliberately implement a pattern that either is discredited or debated (many would describe the singletons as such, with some excluding a write-only - e.g. logging objects or read-only e.g. configuration settings objects - and some would condemn even those) or else where you're solving the wrong problem (when .NET was first brought out, MS recommended a pattern for dealing with disposing when you had both unmanaged fields and disposable managed fields - it does indeed deal with that situation very well, but the real problem is that you've got both types of field in the same class).

As such, an anti-pattern is something that a smart person who knows the language, problem domain and available libraries well will deliberately do, that still has (or is argued to have) a downside that overwhelms the upside.

Since none of us start out knowing a given language, problem domain and available libraries well, and since everyone can miss something as they go from one reasonable solution to another (e.g. start storing something in a field for a good use, and then try to refactor it away but not complete the job, and you'll end up with code like in the question), and since we all miss things from time to time in learning, we have all created some superstitious or cargo-cult code at some point. The good thing, is they're actually clearer to identify and correct than anti-patterns. True anti-patterns are either arguably not anti-patterns, or at have some attractive quality, or at least have some way of luring one into them even when identified as bad (too many and too few layers are both uncontroversially bad, but avoiding one leads to the other).

share|improve this answer
1  
But people do think that this is a good idea, probably because they think that it's a form of code reuse. –  JSBձոգչ Sep 2 '12 at 18:33
    
Beginners often seem to think that declaring two variables somehow requires twice the space, and therefore prefer to reuse variables. This shows a misunderstanding of the memory model (free store vs. stack, possibility of reuse of stack locations) and optimizations that all compilers are able to perform. I'd argue that a beginner might therefore consider variable reuse a good idea, so it could be classified as an antipattern. –  Philipp Sep 3 '12 at 21:23
    
That makes it a superstition, not an antipattern. –  Jon Hanna Sep 3 '12 at 21:27

(1) I would say it's not good.

It's performing manual house keeping that the stack can do automatically with local variables.

There is no performance benefit since "new" is called each method invocation.

EDIT: The memory footprint of the class is larger because the bar pointer is always taking up memory over the lifetime of the class. If the bar pointer was local, then it would only use memory for the lifetime of the method call. The memory for the pointer is just a drop in the ocean, but it's still an unnecessary drop.

The bar is visible to other methods in the class. Methods that don't use bar shouldn't be able to see it.

State-based errors. In this particular case state base errors should only occur in multi-threading since "new" is called each time.

(2) I don't know if there's a name for it since it's actually several issues, not one.
--manual housekeeping when automatic is available
--unnecessary state
--state that lives longer that it's lifetime
--visibility or scope is too high

share|improve this answer

I'd call it the "Wandering Xenophobe". It wants to be left alone but winds up not knowing quite where or what it is. Thus it's a bad thing as others have stated.

share|improve this answer

Going against the grain here but while I don't consider the given example to be good coding style as is in its current form, the pattern does exist while doing unit testing.

This fairly standard way of doing unit testing

public class BarTester
{
    private Bar bar;

    public Setup() { bar = new Bar(); }
    public Teardown() { bar = null; }

    public TestMethodA()
    {
        bar.A();        
    }

    public TestMethodB()
    {
        bar.B();
    }
}

is just a refactoring of this equivalent of OP's code

public class BarTester
{
    private Bar bar;

    public TestMethodA()
    {
        bar = new Bar();
        bar.A();
        bar = null;
    }

    public TestMethodB()
    {
        bar = new Bar();
        bar.B();
        bar = null;
    }
}

I would never write code as given by OP's example, it screams refactoring, but I consider the pattern to be neither invalid nor an antipattern.

share|improve this answer
    
In that case the fixture is instantiated independently for each test, and the problems related to variable reuse or concurrency don't occur. In the general case (outside unit tests) it's not known whether at most one method is called on each object. –  Philipp Sep 3 '12 at 21:18
1  
@Philipp - I don't dispute the reuse or concurrency problem but OP's question was about the pattern wich is what is commonly used in unit-testing. The fact that the fixture is instantiated for each test is an implementation detail of the test framework. Even in unit-testing, the pattern is only safe when the test framework is used as it is supposed to be used. The same reasoning applies when using this pattern in production code. –  Lieven Keersmaekers Sep 4 '12 at 6:22
    
There is no need to set bar to null, JUnit anyway creates a new test instance for every test method. –  oberlies Jul 4 '13 at 8:48

This code smell is referred to as Temporary Field in Refactoring by Beck/Fowler.

http://sourcemaking.com/refactoring/temporary-field

Edited to add more explanation by request of moderators: You can read more about the code smell in the referenced URL above, or in the hard copy of the book. Since the OP was looking for the name for this particular antipattern/code smell, I thought that citing a heretofore unmentioned reference to it from an established source that's over 10 years old would be helpful, though I fully realize that I'm late to the game on this question, so it's unlikely the answer will get voted up or accepted.

If it's not too personally promotional, I also reference and explain this particular code smell in my forthcoming Pluralsight course, Refactoring Fundamentals, which I expect will be published in August 2013.

By way of adding more value, let's talk about how to fix this particular issue. There are several options. If the field is truly only used by one method, then it can be demoted to a local variable. However, if multiple methods are using the field to communicate (in lieu of parameters), then this is the classic case described in Refactoring, and can be corrected by replacing the field with parameters, or if the methods that work with this code are closely related, Extract Class may be used to pull the methods and the field(s) out into their own class, in which the field is always in a valid state (perhaps set during construction) and represents the state of this new class. If going the parameter route, but there are too many values to pass around, another option is to introduce a new Parameter Object, which can be passed instead of all the individual variables.

share|improve this answer
    
It is important to note that Temporary Fields are often required for an Extract Class refactoring. But someone who is aware of this probably wouldn't check in the code in this intermediate state. –  oberlies Jul 4 '13 at 8:55

I'd say when you get right down to it, this is really a lack of cohesion, and I might give it the name "False Cohesion". I'd coin (or if I'm getting it from somewhere and forgetting about it, "borrow") this term because the class appears to be cohesive in that its methods appear to operate on a member field. However, in reality they do not, meaning the apparent cohesion is actually false.

As for whether it's bad or not, I'd say it clearly is, and I think Stephen C does an excellent job of explaining why. However, whether it deserves to be called "anti-pattern" or not, I think it and any other coding paradigm could do with a label, since that generally makes communication easier.

share|improve this answer

Apart from the problems already mentioned, using this approach in environment without automatic garbage collection (e.g. C++) would lead to memory leaks, as the temporary objects are not freed after use. (While this may seem to be a bit far fetched, there are people out there who would just change 'null' to 'NULL' and be happy that the code compiles.)

share|improve this answer

This looks to me like a horrible misapplication of the Flyweight pattern. I've unfortunately known a few developers who have to use the same "thing" multiple times in different methods and simply pull it out into a private field in a misguided attempt at saving memory or improving performance or some other weird pseudo-optimization. In their mind they are trying to solve a similar problem as Flyweight is meant to address only they're doing it in a situation where it isn't actually a problem that needs solving.

share|improve this answer

I've encountered this many times, usually when refreshing code previously written by someone who picked up VBA For Dummies as their first title and went on to write a relatively large system in whatever language they'd stumbled into.

To say it's really bad programming practice is right, but it might just have been a result of not having internet & peer reviewed resources such as we all enjoy today that might have guided the original developer along a "safer" path.

It doesn't really deserve the "antipattern" tag though - but it's been good to see the suggestions of how the OP could be recoded.

share|improve this answer
1  
Welcome to Stack Exchange Programmers, thanks for entering your first answer. It looks like you had a down vote, possibly because of a reasons listed in the FAQ programmers.stackexchange.com/faq. The FAQ gives great suggestions on making effective (and up voted) answers. Sometimes people are kind enough to add a note about the down vote to indicate if it is wrong in some way, duplicate, opinion without evidence, or a me too that repeats an earlier answer. Most of us have had down voted, closed, or removed answers, so don't be concerned. It is just part of getting started. Welcome. –  DeveloperDon Oct 3 '12 at 2:41

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