«Классы никогда не должны выполнять работу с зависимостями в своих конструкторах». - PullRequest
12 голосов
/ 26 августа 2010

Итак, цитата взята из «Внедрение зависимостей в .NET» .Имея это в виду, неправильно ли спроектирован следующий класс?

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!
    }

    ...
}

Так что этот класс FallingPiece несет ответственность за управление текущей падающей фигурой в игре тетрис.Когда часть достигает дна или другого места, вызывает событие, сигнализирующее о том, что затем, через фабрику, генерируется еще одна новая часть, которая снова начинает падать сверху.() метод, который будет генерировать кусок, но IMO немного противоречит идее, что конструктор переведет ваш объект в допустимое состояние.

Ответы [ 6 ]

15 голосов
/ 26 августа 2010

В общем, правила большого пальца именно таковы: правила большого пальца. Неизменяемые законы, от которых никогда не следует отступать - я вполне уверен, что вы найдете случаи, когда работа с зависимостями в конструкторе имеет больше смысла, чем в противном случае.

Имея это в виду, давайте вернемся к вашему конкретному дизайну:

Так что этот класс FallingPiece имеет ответственность за контроль текущий падающий кусок в тетрисе игра. Когда кусок достигает дна или какое-то другое место, вызывает событие сигнализируя это, а затем, через фабрика, генерирует еще один новый кусок который снова начинает падать сверху.

Мне кажется странным, что FallingPiece запускает генератор фигур после его завершения.

Я бы разработал что-то вроде этого:

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();
    }
}

По крайней мере, с этим дизайном, GameEngine полностью отвечает за указание Генератору, когда создавать его части, что кажется более идиоматичным, чем у FallingPiece с такой обязанностью.

5 голосов
/ 26 августа 2010

Да, я бы сказал, что что-то тут не разработано. Трудно сказать, поскольку вы не показываете, как используется FallingPiece или как он вписывается в систему, но мне не имеет смысла, почему ему нужно получить currentPiece в своем конструкторе.

Поскольку похоже, что currentPiece может меняться со временем, создается впечатление, что вы используете один экземпляр FallingPiece в качестве контейнера-одиночки для текущего падающего элемента. В этом случае вы должны получить второй, третий и так далее кусочки каким-то образом. Почему бы просто не сделать то же самое для первого произведения?

Вы говорите, что когда фигура достигает дна, возникает событие, которое запускает генерацию новой фигуры, которая начинает падать. Разве вы не должны сами запустить это событие, чтобы начать первую часть?

В целом:

Общая причина (на мой взгляд), что работа с зависимостями не должна выполняться в конструкторе, заключается в том, что конструирование объекта должно быть связано только с настройкой класса, чтобы иметь возможность делать то, что ему нужно. Наличие классов do , которые они делают, должно выполняться через вызовы API объекта после его создания. В случае FallingPiece, когда у него есть IPieceGenerator, он может делать все, что ему нужно. На самом деле сделать это (начать падение части) следует, вызвав метод в его API или (как это, похоже, имеет место) запустить событие, на которое он будет реагировать.

3 голосов
/ 27 августа 2010

Как уже отмечали другие, это утверждение действительно практическое правило. Однако это довольно строгое правило, поскольку оно дает нам более одного преимущества:

  • Применяет SRP к конструктору; другими словами, он сохраняет конструктор настолько чистым, насколько это возможно, потому что он делает только одну вещь.
  • Упрощает модульное тестирование, поскольку создание SUT не требует сложных Настройка прибора .
  • Это делает вопросы дизайна более очевидными .

Это третье преимущество, по моему мнению, здесь в игре. Как указано, этот конструктор кричит о нарушении SRP. Какова ответственность класса FallingPiece? Сейчас кажется, что и создают новые фигуры и представляют падающую фигуру.

Почему FallingPiece не является реализацией IPiece?

В целом, IPiece звучит для меня так, будто это должен быть конечный автомат. Сначала он движется и им можно манипулировать, но когда он достигает дна, он становится заблокированным для других частей, пока не может быть удален. Вероятно, было бы целесообразно вызывать события по мере изменения состояния.

С объектно-ориентированной точки зрения я думаю, что на игровом поле должна быть коллекция экземпляров IPiece, которые знают, где они находятся и что они могут сделать.

В целом, я думаю, что правило о том, что не нужно работать в конструкторе, стоит, так как оно привлекает наше внимание к вопросам проектирования.

2 голосов
/ 26 августа 2010

Если вы используете инжектор конструктора, это нормально.

Должно быть очевидно, почему это не будет работать с инжектором сеттера.Я считаю, что цитата вполне верна с точки зрения последнего.

2 голосов
/ 26 августа 2010

Я бы сказал, что этот класс спроектирован правильно, потому что список параметров конструктора требует IPieceGenerator. Следовательно, это не противоречит идее, однако, если бы это был вызов статического метода, не содержащегося в списке параметров, я бы сказал, что он не соответствует этому правилу.

1 голос
/ 26 августа 2010

Оставляя currentPiece ноль, не обязательно означает, что объект остается в унифицированном состоянии. Например, вы можете преобразовать currentPiece в свойство и лениво инициализировать его при первом обращении к нему.

Это никак не повлияет на общедоступный интерфейс, но сохранит легкий вес конструктора, что в целом хорошо.

Добро пожаловать на сайт PullRequest, где вы можете задавать вопросы и получать ответы от других членов сообщества.
...