axent13 / life Goto Github PK

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

В тестах ты напрямую правишь _cells - почему не используешь changeCellState?

По сути ты тестируешь не только, что у клетки изменен класс, но и что метод возвращает обновленные классы (потому что убери свой return и метод продолжит делать, что надо, но тесты отвалятся). Это кстати в целом говорит о плохом API твоего класса - один метод должен делать одну вещь.

Если надо глобально ставить - добавить команды в README
Но лучше не заставлять ставить глобально ничего - просто добавь в зависимости и в package.json в скриптах до локальных бинарников указывать:

  "scripts": {
    "build": "./node_modules/.bin/webpack-dev-server --config webpack/webpack.config.js",
    "test": "./node_modules/.bin/karma start karma/karma.conf.js"
  }

Добавить на DOM-элементы классы с префиксом js, на которые вешаются обработчики, или с которыми работает JS-код. (js-start-button, js-stop-button и т.д) и заодно избавиться от id в верстке и в коде.

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

Функция по генерации разметки поля слишком усложнена, лучше воспользуйся шаблонизатором и легко уйдешь от ужасающих прибавлений строк, которые ты генерируешь с if-условиями к тому же. Боюсь представить, что начнется, если бизнес-логику усложнить (а в реальных проектах она постоянно усложняется и нельзя писать код только под текущие требования).

Это немного странно, что ты требуешь от пользователей твоей модели сначала вызывать nextCellStates(), а потом бегать по возвращаемому массиву и вручную вызывать changeCellState()

Можно как вариант в nextCellStates() все-таки менять сами _cells, и сохранять в свойстве модели последние клетки, которые были изменены. В идеале вообще историю изменений в модели хранить :)

Много шаблонного кода, можно сделать более читаемо и лаконично (но главное чтобы читаемость была высокой, замысловатые конструкции избегай)

Я про changeCellStateEvent, startButtonEvent, pauseButtonEvent

У приложения должны быть кнопки "Начать заново", "Пауза" и инпуты "Ширина" и "Высота" - по unfocus инпутов поле должно перерендерится сразу же.

При открытии станицы запускается initializeInterval (бесконечный цикл) который ожидает действия от пользователя. Это лучше сделать наоборот. Т.е пользователь запускал бы игру или останавливал при клике на соотв. кнопки, при этом вызывались бы нужные функции из controller.

Она всегда будет проходить, тот момент что ты хотел проверить надо было по другому оформлять (импорт делать между замокиванием метода initializeInterval и проверкой его вызовов)

И генерить события, которые бы контроллер слушал и пробрасывал в модель, а в ответ слушал изменения модели и пробрасывал во вью.
Вся работа с DOM должна быть только во VIew, тогда нам например будет гораздо проще целиком уйти от jquery и пересесть на angular или react например

В этом, например, прелесть реакта, так как он позволяет сделать View как чистую функцию от Model, как, например, f(x) в математике говорит, что f зависит только от x, так и тут View = f(Model).
Это позволяет, в том числе сделать сильное разделение кода (то есть модель точно должна быть самодостаточной, а у тебя сейчас модель зависит от вью, так как она не понимает, идет игра сейчас или нет) и вью должна можно научить отображать другую модель, если у нее похожее API

index.html, styles.css,favicon.ico - им не место в корне, убери куда нибудь в статику.

• создание новой клетки в drawField;
• инициализацию поля вынести из конструктора модели в метод класса.

Прям совсем

Проверку на вхождение в диапазон (yPos >= 0 && yPos < this._fieldWidth) вынести в отдельную функцию.

 _isElementInsideField(xPos, yPos) {
    if (yPos >= 0 && yPos < this._fieldWidth) {
      if (xPos >= 0 && xPos < this._fieldHeight) {
        return 1;
      }
    }

    return 0;
  }

входной параметр cell у метода changeCellState() подразумевает, что клетка - это объект jQuery (ты как минимум используешь у этого параметра toggleClass и attr). Но тогда параметр должен называться $cell

Сейчас много вариантов использования моделей не покрыты тестами. Например, у метода _isElementInsideField не проверяется, что будет, если передать параметрами -1, -1 (то есть вне поля, но по другой, минимальной, границе).
Вот тебе шутка в тему: http://pikabu.ru/story/testirovshchik_v_bare_3046939

Она вообще только с чистыми данными уметь работать должна по идее :)
У тебя немного делает, но все таки ты зоны ответственности смешиваешь и это плохо внутри changeCellState

У тебя сейчас на самом деле модель вообще не нужна :) у тебя и без модели View нормально генерит начальное состояние и потом его изменяет.
По-хорошему, в контроллере лучше менять только модель, а потом брать состояние из модели (список клеток например) и передавать во View. А View в свою очередь должна уметь только заново себя рендерить по полному слепку текущего поля.

При нажатии на кнопку "пауза" интервал должен быть очищен из памяти.

По хорошему сделать экспорт функции, которая все запускает, и внизу файла запустить отдельным вызово. Оттестить как раз эту (и другие, если создашь) функцию

Файлы приложения в src, файлы сборки в build (и заигнорить содержимое папки, компилируемые автоматом файлы в репо не добавляются обычно). Конфиги лучше тоже в отдельные папки webpack и karma

Кстати this.emit('event'); лучше переименуй, тебе 5 секунд жалко придумать нормальное название?)

Сейчас покрыт только nextCellStates в моделях, но у модели есть еще два метода, плюс есть вьюхи и контроллеры :) Плюс есть такая штука, как интеграционные (или end-to-end) тесты :)

Не проверяется создание модели и вью, не проверяется вызов методов, которые вешают обраотчики на события, не проверяется инициализационный вызов метода у вью

_field_width - нижнее подчеркивание в начале не говорит о том, что использовать underscore_style_naming
Надо в любом случае _camelCaseNaming

changeCellState(xPos, yPos) {
    if (this._isElementInsideField(xPos, yPos) === 1) {
      if (this._cells[xPos][yPos] === 1) {
        this._cells[xPos][yPos] = 0;
      } else {
        this._cells[xPos][yPos] = 1;
      }
    }

    return this;
  }

Заменить на

changeCellState(xPos, yPos) {
    if (this._isElementInsideField(xPos, yPos) === 1) {
        this._cells[xPos][yPos]  =  this._cells[xPos][yPos] === 1 ? 0 : 1;
    }

    return this;
  }

По хорошей практике методы, у которых нет прямого предназначения что-то возвращать, лучше бы возвращали this. Например,
model.changeCellState() - лучше чтобы возвращал this
model.nextCellStates() - лучше чтобы возвращал this
model.getCells() - возвращала массив клеток

Это описано у Крокфорда кстати

Сейчас каждый модуль имеет просто список функций и общие переменные. Сделай плиз все по канонам ООП с использованием классов, создавая инстансы модели, вьюхи и контроллера и вызывая их методы жизненного цикла

Очень много циклов for, очень много не вынесенных условий, отсупы должны быть 2 пробела, проверить остальные все правила из стандартов и исправить самому :)

controller._isPaused = true; - это откуда-то взялось в тестах, насколько помню, осталось со старой версии контроллера, когда он еще хранил стейт игры

Избавиться от циклов for (переписать на forEach, map filter и т.п.), где это возможно

Тебе в рамках unit тестов надо проверять только функционал самого класса, соответсвенно, все, что относится к вложенным классам лучше мокать, так что тут я бы советовал замокать модель и вью и проверять, что nextStep() просто дергает нужные методы у нужных вложенных сущностей.
А еще если бы ты сделал модель и вью передаваемыми сверху, то у тебя бы в тестах все было гораздо красивей и четче :) И API твоего контроллера стало бы гибче :)

У тебя метод изменяет разметку:

 const $gameField = $('#game-field');
 $gameField.html(resultingField);

но это никак не оттестировано

Я так понимаю, этот вызов никак не влияет на тесты changeCellState - а если бы и менял, то стоило бы API твоего класса пересмотреть :)