When a constructor calls a helper function, should the helper mutate the object or return part of the object?

Question

I have a private method that is called from the constructor to instantiate a bunch of objects.

I can do it two ways.

    private Monster[,] createMonsters()
    {
        Monster[,] myMonsterArray = new Monster[8, 6];

        for (int i = 0; i < 8; i++)
        {
            myMonsterArray[0, i] = new Monster(SpriteFactory.getMonster1Sprite());
            myMonsterArray[1, i] = new Monster(SpriteFactory.getMonster2Sprite());
            myMonsterArray[2, i] = new Monster(SpriteFactory.getMonster3Sprite());
            myMonsterArray[3, i] = new Monster(SpriteFactory.getMonster1Sprite());
            myMonsterArray[4, i] = new Monster(SpriteFactory.getMonster2Sprite());
            myMonsterArray[5, i] = new Monster(SpriteFactory.getMonster3Sprite()); 
        }

        return myMonsterArray;
    }

called with

monsterArray = createMonsters();

or

    private void createMonsters()
    {
        monsterArray = new Monster[8, 6];

        for (int i = 0; i < 8; i++)
        {
            monsterArray[0, i] = new Monster(SpriteFactory.getMonster1Sprite());
            monsterArray[1, i] = new Monster(SpriteFactory.getMonster2Sprite());
            monsterArray[2, i] = new Monster(SpriteFactory.getMonster3Sprite());
            monsterArray[3, i] = new Monster(SpriteFactory.getMonster1Sprite());
            monsterArray[4, i] = new Monster(SpriteFactory.getMonster2Sprite());
            monsterArray[5, i] = new Monster(SpriteFactory.getMonster3Sprite()); 
        }


    }

called with

createMonsters();

Is either better or worse?

Answer 1

As far as I see no-one has mentioned temporal coupling yet, so here it is.

The second one (which returns void) has this code smell. Consider the following constructor code:

createMonsters();
...
doSomethingWithMonsters();

A maintainer easily could mix up the order of the called methods which breaks your class and might turn out only at runtime. It's not possible the following:

Monster[,] monsters = createMonsters();
...
doSomethingWithMonsters(monsters);

Changing the order of the two methods simply would not compile.

Another advantage of the first one is that your field could be final (or readonly, I'm not too familiar with C# but I guess it has something like that). The following constructor code also does not compile in Java if the monsters field is final:

doSomethingWithMonsters(monsters);
monsters = createMonsters();

Furthermore, having the array return value makes the code easier to read. It's obvious from the signature what the method does, readers don't have to check the method body to figure out its side-effects. Finally, you could call createMonsters() from other methods without side-effects.

Anyway, having the separate factory class (as a constructor parameter) looks even better, it's less coupled, +1 to Simon.

See also: Clean Code by Robert C. Martin, G31: Hidden Temporal Couplings, p302

Answer 2

It depends

I personally find a method that returns it more flexible and more of a separation-of-concerns approach. And by doing so, you could even move the method into another class, such as a Factory class that perhaps can provide you with other monster arrays.

However, it is perfectly reasonable to use a method that returns void if:

• The reason for why you have this method in the first place is that you want to extract some code in your constructor into it's own method
• Your myMonsterArray is in a way "tightly coupled" with your class (i.e. it wouldn't make much sense to have the monster array without also having your class - think of a 8x8 chess grid without the chess game itself for example)

In the end, which way you choose is up to you.

Other comments

Instead of numbering your SpriteFactory.getMonster1Sprite() methods, use SpriteFactory.getMonsterSprite(1), this would allow you to write:

for (int i = 0; i < 8; i++)
{
    for (int a = 0; a < 6; a++)
    {
        myMonsterArray[a, i] = new Monster(SpriteFactory.getMonsterSprite(a % 3));
    }
}

Magic numbers: Instead of using 8 and 6 (and perhaps even 3) in the double-loop above, retreive the length of your array with myMonsterArray.GetLength(0) and myMonsterArray.GetLength(1).

Answer 3

Consider it is just an initializer and will only be used this once, there is no need to make it explicitly return the data when you can just set it directly.

Therefore the second approach would be my approach as well.

Naming

Method names are UpperCamelCase in C# so this

createMonsters

should become

CreateMonsters

Indices

You have your indices swapped: you define it as [8, 6] but you use it as [6, 8] which will lead to IndexOutOfRangeExceptions

Answer 4

I would pick the method where you return an array of objects, because that makes it easier to unit test that method should you feel the need.

Answer 5

TLDR: The first approach is better, pure functions are easier to understand.

I would personally go with the first approach. As a general rule I prefer pure functions. While in this case I don't think it would make much difference, I have very rarely ever found that I was happier with a mutating function like this. I think pure functions are much easier to reason about, and reduce complexity. In general the creation of the object is best left to the function while the assignment should be left to the constructor.

As with all code, if there was a single rule that could be easily applied, we wouldn't need programmers, as computers already excel at applying basic rules. What makes a programmer valuable is that there are many tools/patterns that can be applied, and they always have various trade offs. Our job is to understand where the tools/patterns give good value and apply them intelligently.