Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
2  
I was set off by the fact that you have camelCase method names, whereas I thought that they would be PascalCase in C#. –  skiwi Apr 26 at 9:59
    
Huh is it? - I had been noticing that the syntax was like that, in the API, and I wasn't sure why it was the case. –  dwjohnston Apr 26 at 10:16
    
This is the first C# project I've worked on in a while, just to add it to my portfolio. –  dwjohnston Apr 26 at 11:01
add comment

5 Answers 5

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

share|improve this answer
1  
Yep, temporal coupling is the first thing that went through my mind when I read that question. This is a beautiful case for this. –  Pierre-Luc Pineault Apr 27 at 7:31
add comment

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

share|improve this answer
    
The length of an array is retrieved with myArray.Length ...or did you think it was Java? ;) –  Mat's Mug Apr 26 at 15:05
    
@Mat'sMug No, I'm aware it's C#. GetLength is what MSDN says and also what I used in my C# Sudoku Solver - have I missed anything? –  Simon André Forsberg Apr 26 at 15:23
    
Nevermind, brainfart. Length wouldn't be useful for multidimentional arrays.. catxh you in chat later! :) –  Mat's Mug Apr 26 at 15:26
1  
An additional reason, related to the ones you cite, for preferring the version that returns the values: you could then extract an abstract superclass and defer creating the values to the subclass, and have the resulting variable remain private rather than needing to be changed to protected. A simpler variant of the Factory version you suggest. –  Jules Apr 26 at 17:19
add comment

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

share|improve this answer
add comment

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.

share|improve this answer
3  
private methods should not be unit tested considering they are not a part of the exposed API and thus, by definition, their internal definition is not reliable. –  Jeroen Vannevel Apr 26 at 17:47
1  
Many frameworks provide the ability to test private methods. Unit testing is usually done by the developers who are well aware of the functionality. In my opinion unit testing of private members will help to improve the quality. –  Sandeep Apr 27 at 6:04
add comment

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.

share|improve this answer
add comment

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.