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.

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!

share|improve this question
3  
First, don't use null; null sucks. That aside, you can make a factory class whose constructor takes the defaults. –  Doval Aug 7 at 15:38
1  
On an unrelated note, it's a bad idea for your game objects to know how to draw themselves. Physics and graphics are two separate responsibilities. –  Doval Aug 7 at 15:44
    
Checking out the lengthy but interesting video by Tony Hoare now, interesting to hear about the Billion Dollar Mistake. –  bigp Aug 7 at 16:35
    
Regarding the game objects knowing how to draw themselves, I'm not sure I follow. Physics and Graphics are done by other classes, but you still have to tell a game object what texture / physics properties it should be using, no? –  bigp Aug 7 at 16:37
    
If you decide to render your objects in 3D, or have a debug view that turns off sprites and simply draws colored rectangles, or you want alternate textures, you'll have to make changes to the GameObject class. If you keep them separate the changes stay in the Graphics class. –  Doval Aug 7 at 17:03

6 Answers 6

up vote 6 down vote accepted

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

share|improve this answer
7  
As an addition: If the language supports it, it is preferable to use the actual default values rather than null and overriding that within the function. –  Bart van Ingen Schenau Aug 7 at 18:11
    
AS3 does not support inlining "those kinds" of values as default parameters. By that, I mean a property (such as the one in the example) that could potentially change. So constants are acceptable, hardcoded primitives, but that's it! (added in my edit) –  bigp Aug 7 at 23:38
  • 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.
share|improve this answer
1  
It seems you are implying he should raise an exception when using the default. Did you really mean that? –  DougM Aug 7 at 16:29
    
@DougM You are right. I changed it slightly to make my point clear. –  user61852 Aug 7 at 16:42

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.

share|improve this answer

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)
}
share|improve this answer
    
In the example's language I was showing (ActionScript 3.0), it doesn't support 'overloading' (just overriding from an extended class's method), so I would basically have to write it as a new named method. MakeFromName & MakeFromNameWithParams perhaps? Not sure if the extra function-call would outweigh the null-checking. –  bigp Aug 7 at 23:18
    
@bigp Ahh interesting. Optional param but no overloading, weird. Are you allowed to assign GameBaseClass.INSTANCE.currentAtlas or does it have to be constants. I've added a language tag to lure any action-script aficionados. –  Nathan Cooper Aug 8 at 7:55
    
Thanks for the additional tag, didn't even know it was part of this stackexchange site's tag list! :) And yes, defaults have to be constants in order to be inlined in the method signature. –  bigp Aug 8 at 14:09

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.

share|improve this answer
    
I edited my question to include details about the ActionScript 3.0's limitation in regards to the function-signature, since you weren't the first to point that out :). The reason I'm using Object is because the data is JSON parsed content, which is internally converted (out of my control) to an Object. So I just store the root as is. It has crossed my mind to create classes to represent each node in the object-hierarchy (of the said JSON data), but it just seems redundant and the performance gains would probably be negligible, considering this is only used at initialization. –  bigp Aug 7 at 23:30

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.

share|improve this answer
    
Not in C#. C# only allows constants (and some value type trickery). I'm not sure if this language (scala?) can do that. –  Telastyn Aug 7 at 19:08
    
@Telastyn Mmmm, the example is clearly not in C#, I assumed it was a theoretical discussion about a new language. Not familiar with Scala here, so I didn't recognize the code. –  RMalke Aug 7 at 19:10
    
Actually no, can't use that in the parameter default values. The language is ActionScript 3.0, but same limitation applies, can only use constants (basically nothing that could get altered at runtime). –  bigp Aug 7 at 19:22

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.