Is it bad practice to resolve null arguments to default static variables?

Question

First, let me show you an example (written in ActionScript 3.0):

class GameObject {
    public static function MakeFromName( pName:String,
                                         pAtlas:TextureAtlas = null,
                                         pGameData:Object = null ):GameObject {

        // If these arguments are not passed, they default to the static INSTANCE's 
        // currentAtlas and currentData.
        if (pAtlas == null) pAtlas = GameBaseClass.INSTANCE.currentAtlas; // <--
        if (pGameData == null) pGameData = GameBaseClass.INSTANCE.currentData; // <--

        var theSymbolData:Object = pGameData.symbols[pName];
        var theSymbolTextures:Vector.<Texture> = pAtlas.getTextures( pName );

        var newGO:GameObject = new GameObject();
        newGO.MakeFromData( pName, theSymbolTextures, theSymbolData );

        return newGO;
    }

    // MakeFromData defined somewhere down in the code...
}

I'm having this debate with a coworker regarding the above 'null' (basically optional) parameters.

To me, it makes sense to auto-resolve those two parameters (pAtlas and pGameData) to some sort of central resource (in this case, the primary Atlas and Data used in the game). This GameBaseClass goes hand-in-hand with GameObject, so I see no harm in referring to it's singleton-instance's properties. The developer still has the option to supply their own Atlas and Data.

BUT, my coworker believes this ties the classes together too much - it isn't loose enough. I can understand his point if it was actually referring to the derived classes used in the game (i.e: AwesomeGame.INSTANCE.currentAtlas, where AwesomeGame extends GameBaseClass). But it isn't! From his point of view, the developer should be forced to enter every single parameters, nothing optional (in this particular situation).

Is there any way to have the best of both worlds?

The only other way I can think of is just to write two separate methods (one with the 2nd and 3rd arguments, and one without), but that still doesn't address the issue with the GameBaseClass dependency.

Any ideas?

EDIT: In the above example, I used ActionScript 3.0 which does not support assigning non-constant variables. In other words, it allows hardcoded Numbers, Strings (empty ones too), Booleans, and Constants (although I don't think it would work with non-primitive constants, could be wrong though). Since the GameBaseClass.INSTANCE.currentAtlas is a property that could change in the lifetime of the running application, this cannot be inlined as a 'compile-time' default value. Hope that makes more sense!

Answer 1

That is perfectly fine. This is often the compromise between the "passing everything in makes it too complex!" and the "using a static directly makes things inflexible and hard to test!" camps.

Personally, I would only do this if the common (75%+) usage is to use the default instance. Otherwise, people will go with the easiest route (less arguments) which isn't necessarily the correct route. Making people specify the parameters means they'll say "how do I make a TextureAtlas?" rather than using the common one, which is its own incorrect route.

Having these sane defaults encourages users of the class towards doing the right thing most of the time, while not precluding them from doing the right thing when it deviates from the norm (with a little more work).

Answer 2

• If those parameters are not optional, then raise an exception.
• Is they shoudn't be optional, then Intelligent defaults are a good idea.
• Also if GameBaseClass is a top abstraction, you are adhering to the Dependency Inversion Principle. Your class depends on an abstraction not on a concretion.

Answer 3

Yes, it's very bad practice, for several reasons, including at least:

1. Global state is bad in general. By having code that only uses the global state when the argument is null, you allow it to be used correctly and to be tested, but any other code that uses your code with a null argument then becomes problematic, as it inherits this global state. If you have two or more components using your code with null argument and then decide you want to be able to use both at the same time, you're in trouble.

2. It's error-prone. If a caller accidentally passes null, perhaps as a result of a logic bug in the caller (often in the form of an unhandled error condition), your code acts on an unrelated object rather than producing an error.

3. It has cost. Perhaps low cost, but still some cost, in the form of (1) an extra branch to test for null and do something special, and (2) the cost of the extra object/state which well-written callers will never use, but which has to exist anyway at runtime because it might be referenced.

Answer 4

An aside: You have parameters default value set to null and then the first thing you do is null check them. Have you tried method overloading. Ie

public static function MakeFromName(String mystring)
{
    MakeFromName(mystring, GameBaseClass.INSTANCE.currentAtlas, GameBaseClass.INSTANCE.currentData)
}

Answer 5

I don't think your defaults create the coupling. I think you wanting them is a sign that the coupling already exists. There are at least two other signs of tight coupling here:

• You're not even using those parameters directly, you're using members of those parameters, which is a Law of Demeter violation.
• Wanting multiple factory-like methods that call each other.

There are other ways to avoid passing parameters around all over the place:

• Move MakeFromName to another existing class. Perhaps the code that calls it, or TextureAtlas.
• Create one new class for the sole purpose of creating GameObjects. Its constructor could store the Atlas and GameData, then you can create many GameObjects with the same dependencies with factory.MakeFromName(name).
• Invert the dependencies so a GameObject doesn't need to know about the textures and symbols.
• Pass in the textures and symbols later when the GameObject is used, rather than when it is created.
• Move the responsibility for the defaults into the GameObject constructor. In other words, have default symbols and textures instead of default atlases and game data.
• Consolidate the places where you call MakeFromName into one place. If you have one place where the association between atlas, game data, and name is made, rather than being scattered all over the place, you won't care about a couple extra parameters.

I can't really advise you further without seeing all your code. Keep in mind, it's often hard to tell if these suggestions will help without trying them.

As a side note, I'm wondering why you didn't just make those objects a default in the function signature, instead of assigning it to null there and checking for null later. It seems like an unnecessary intermediate step.

Also as a side note, I don't know what language this is, but I'm guessing that Object is its top level base class from which all objects ultimately derive. If so, using it this way for your game data isn't advisable.

Answer 6

Couldn't you do something like that:

class GameObject {
    public static function MakeFromName( pName:String,
                                         pAtlas:TextureAtlas = GameBaseClass.INSTANCE.currentAtlas,
                                         pGameData:Object = GameBaseClass.INSTANCE.currentData ):GameObject {

        var theSymbolData:Object = pGameData.symbols[pName];
        var theSymbolTextures:Vector.<Texture> = pAtlas.getTextures( pName );

        var newGO:GameObject = new GameObject();
        newGO.MakeFromData( pName, theSymbolTextures, theSymbolData );

        return newGO;
    }

    // MakeFromData defined somewhere down in the code...
}

This way your intentions become clear from the signature, if you won't passe the value, it'll fallback to the default value.