Name for this antipattern? Fields as local variables [closed]

Question

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?

Answer 1

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.

Answer 2

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

Answer 3

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.

Answer 4

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.

Answer 5

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

Answer 6

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).

Answer 7

(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

Answer 8

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.

Answer 9

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.

Answer 10

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.

Answer 11

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.

Answer 12

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.)

Answer 13

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.

Answer 14

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.