"Classes should never perform work involving Dependencies in their constructors."

Question

So, the quote comes from "Dependency Injection in .NET". Having that in consideration, is the following class wrongly designed?

class FallingPiece { //depicts the current falling piece in a tetris game
    private readonly IPieceGenerator pieceGenerator;
    private IPiece currentPiece;

    public FallingPiece(IPieceGenerator pieceGenerator) {
        this.pieceGenerator = pieceGenerator;
        this.currentPiece = pieceGenerator.Generate(); //I'm performing work in the constructor with a dependency!
    }

    ...
}

So this FallingPiece class has the responsibility of controlling the current falling piece in a tetris game. When the piece hits the bottom or some other place, raises an event signaling that and then, through the factory, generates another new piece that starts falling again from above.

The only alternative I see is to have an Initialize() method that would generate the piece, but IMO that goes a bit against the idea of having the constructor put your object in a valid state.

Answer 1

I would say this class is properly designed, because the constructor's parameter list requires the IPieceGenerator. Therefore it does not go against the idea, however if it were a call to a static method not contained within the parameter list than I would say it fails to meet that rule.

Answer 2

If you are using constructor injection, this is fine.

It should be obvious why this would not work with setter injection. I believe the quote is quite valid in terms of the latter.

Answer 3

Yes, I'd say something isn't designed right there. It's hard to say since you don't show how FallingPiece is used or how it fits in the system, but it doesn't make sense to me why it needs to get a currentPiece in its constructor.

Since it looks like currentPiece can change over time, it seems like you're using a single instance of FallingPiece as a singleton container for the current falling piece. In that case, you must be getting the second, third and so on pieces some way. Why not simply do the same for the first piece?

You say that when a piece hits the bottom, an event is raised that triggers the generation of a new piece to start falling. Shouldn't you just fire that event yourself to start the first piece?

In general:

The general reason (as I see it) that work with dependencies shouldn't be done in the constructor is that the construction of an object should be purely about setting up the class to be able to do what it needs to do. Having the classes actually do what they do should be accomplished through calls to the object's API after it is created. In the case of FallingPiece, once it has an IPieceGenerator it has the ability to do everything it needs to do. Actually doing that (starting a piece falling) should be done by calling a method on its API or (as seems to be the case here) firing an event that it will respond to.

Answer 4

In general, rules of thumbs are exactly that: rules of thumb. Not immutable laws that one should never deviate from -- I'm pretty sure you'll find cases where doing things with your dependencies in your constructor makes more sense than otherwise.

With that in mind, let's revisit your particular design:

So this FallingPiece class has the responsability of controlling the current falling piece in a tetris game. When the piece hits the bottom or some other place, raises an event signaling that and then, through the factory, generates another new piece that starts falling again from above.

Seems weird to me that FallingPiece triggers the piece generator after its finished.

I'd design it something like this:

class PieceGenerator
{
    public FallingPiece NextFallingPiece()
    {
        FallingPiece piece = new FallingPiece(NextPiece());
        return piece;
    }
    // ...
}

class GameEngine
{
    public PieceGenerator pieceGenerator = new PieceGenerator();

    public void Start()
    {
        CreateFallingPiece();
    }

    void CreateFallingPiece()
    {
        FallingPiece piece = pieceGenerator.NextFallingPiece();
        piece.OnStop += piece_OnStop;
        piece.StartFalling();
    }

    void piece_OnStop(FallingPiece piece)
    {
        piece.OnStop -= piece_OnStop;
        if (!GameOver())
            CreateFallingPiece();
    }
}

At least with this design, the GameEngine is fully responsible for telling the Generator when to create its pieces, which seems more idiomatic than the FallingPiece having that duty.

Answer 5

Leaving currentPiece null doesn't necessarily mean leaving object in unitialized state. You can, for example, convert currentPiece into property and initialize it lazily when it's first accessed.

This won't affect public interface in any way, but will keep constructor lightweight, which is generally a good thing.

Answer 6

As others have pointed out, that statement is indeed a rule of thumb. It is, however, a fairly strong rule because it provides us with more than one benefit:

• It applies the SRP to the constructor; in other words, it keeps the constructor as clean as possible because it does only one thing.
• It makes unit testing easier because creating the SUT doesn't involve complex Fixture Setup.
• It makes design issues more apparent.

This third benefit is, in my opinion, in play here. As given, this constructor screams of an SRP violation. What is the responsibility of the FallingPiece class? Right now it seems to be both creating new pieces and representing a falling piece.

Why isn't FallingPiece an implementation of IPiece?

All in all, IPiece sounds to me like it should be a state machine. At first it's moving and can be manipulated, but when it hits bottom it becomes locked to the other pieces until it can be removed. It would probably make sense for it to raise events as it changes state.

From an object-oriented perspective I think it sounds like the game board should have a collection of IPiece instances that knows where they are and what they can do.

In summary, I think the rule about not doing work in the constructor stands, as it draws our attention to design issues.