"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

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 2

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.