ООП в моём тестовом задании, code review?

Question

[faleaksey]

Всем привет! Решил попробовать свои силы на должность junior frontend developer...
Прислали тз, в котором описывается следующее:
Написать приложение, функции которого следующие:
1) Добавлять новых работников и выводить их списком
2) Элементы формы создавать динамически, так как потребуется их кастомизация
3) Выполнять простую проверку на заполнение полей
4) Возможность удалять работников из списка
6) Выводить и обновлять количество работников
5) По желанию выполнять ajax запрос при добавлении и обновлять список (используя firebase)

Вроде реализовал всё кроме 5 пункта... пока решил остановиться и получить code review, от вас дорогие тостерчане!

Жду здоровой критики и что можно улучшить :)

песочница

// получаем доступ к ключевым элементам
var _FORM = document.getElementById('form');
var _USERS = document.getElementById('users');
var _COUNTER = document.getElementById('counter');

// создание класса формы
var Form = function () {

    // массив с работниками 
    let users = [
        {
            name: 'Fedor Petrov',
            jobs: 'Google'
        }
    ];

    function Form(e) {
        var _this = this;
        this.formWrap = document.createElement('div');
        this.formWrap.classList = 'js-form';
        _FORM.append(this.formWrap);
        _this.createInput(e.input);
        _this.createButton(e.button);
        _this.initListUsers(users);
    }

    // создание полей input
    Form.prototype.createInput = function createInput(e) {
        for (let i = 0; i < e.length; i++) {
            this.input = document.createElement('input');
            this.input.type = e[i].type;
            this.input.classList = 'js-input';
            this.input.name = e[i].name;
            this.input.placeholder = 'Введите ' + e[i].content + ':';
            this.input.id = e[i].id;
            this.input.value = '';
            this.formWrap.append(this.input);
        }
    }

     // создание полей button
    Form.prototype.createButton = function createButton(e) {
        for (let i = 0; i < e.length; i++) {
            this.button = document.createElement('button');
            this.button.type = e[i].type;
            this.button.classList = 'js-button';
            this.button.name = e[i].name;
            this.button.id = e[i].id;
            this.button.value = '';
            this.button.innerText = e[i].content;
            this.formWrap.append(this.button);
            this.button.addEventListener('click', this.createUser, false);
        }
    }

    // выводим при загрузке работников из массива
    Form.prototype.initListUsers = function initListUsers(user) {
        for (let i = 0; i < user.length; i++) {
            this.list = document.createElement('div');
            this.list.classList = 'js-user';
            this.names = document.createElement('span');
            this.jobs = document.createElement('span');

            this.names.append(user[i].name);
            this.jobs.append(user[i].jobs);

            this.list.append(this.names);
            this.list.append(this.jobs);

            _USERS.append(this.list);
        }
        _COUNTER.innerText = user.length;
    }

    // создаём работника и заносим его в массив
    // и выводим его на экран
    Form.prototype.createUser = function createUser() {
        let valName = document.getElementById('name');
        let valJobs = document.getElementById('jobs');
        this.user = {
            name: valName.value,
            jobs: valJobs.value
        }

        // проверяем чтобы все поля были введены
        if (this.user.name === '' || this.user.jobs === '') {
            alert('Заполните все поля');
        } else {
            users.push(this.user);

            let lastUser = users[users.length - 1];

            this.list = document.createElement('div');
            this.list.classList = 'js-user';
            this.names = document.createElement('span');
            this.jobs = document.createElement('span');
            this.names.append(lastUser.name);
            this.jobs.append(lastUser.jobs);
            this.list.append(this.names);
            this.list.append(this.jobs);

            setTimeout(() => {
                _USERS.insertBefore(this.list, _USERS.children[0]);
                valName.value = '';
                valJobs.value = '';
                _COUNTER.innerText = users.length;
            }, 300);
        }
    }


    return Form;
}();


var init = function (e) {
    new Form(e);
}


init({
    input: [
        {
            name: 'name',
            content: 'Имя',
            type: 'text',
            id: 'name'
        },
        {
            name: 'jobs',
            content: 'Должность',
            type: 'text',
            id: 'jobs'
        }
    ],
    button: [
        {
            name: 'submit',
            content: 'Добавить работника',
            type: 'submit',
            id: 'submit'
        }
    ],
});

Comments

[sim3x]

А можете скрин с ТЗ прислать со словами "добовлять", "создовать"?

[faleaksey]

sim3x, это моя опечатка)

[sim3x]

faleaksey, ок
Безграмотность в ТЗ иногда неплохой красный флаг

[AngReload]

А сколько времени на такое задание?

[Антон Спирин]

Вангую, что работодатели ждут более-менее полноценное, масштабируемое приложение, реализованное на одном из современных популярных фреймворков. Простите, но язык не поворачивается назвать вашу реализацию приложением.
Детский сад. Для новичка пойдет, для кандидата на junior frontend вряд ли.

Вроде реализовал всё кроме 5 пункта...

И четвертого.

[faleaksey]

Антон Спирин, в тз упоминалось без использования фреймворков

[faleaksey]

AngReload, 2 дня

Answer 1

[Антон Спирин]

1. Вы инкапсулируете модуль Form, но при этом он зависит от глобальных переменных, а не получает нужные параметры при создании экземпляра.
2. Сам объект Form это ни что иное, как наглядная демонстрация антипаттерна God Object . Почему, не имеющий никакого отношения к форме, список является ее частью остается загадкой. Как и то почему в модуле инкапсулировано состояние приложения.
3. Насчет аргумента "e" уже не раз написали. Насколько я понимаю, вы видели на просторах интернета, что так часто называют аргумент функции, но, видимо, не поняли почему. Один аргумент имеет говорящее название user, но и оно вводит в заблуждение, так как на вход ожидается массив пользователей.
4. Почему не использованы возможности ES6 остается загадкой.
5. Попробуйте сами догадаться, что не так с этим отрывком кода.

users.push(this.user);

let lastUser = users[users.length - 1];

6. У вас почти все переменные в методах объявлены свойствами объекта, при том, что в этом нет никакой необходимости и это может стать причиной ошибок в дальнейшем. Почему не использованы локальные переменные остается загадкой.

Answer 2

[grinat]

Это не ооп, ты просто собрал функции и засунул их в класс. Типа такого надо:

class User (){
    construct ()
   save() {
       return ajax-запрос
   }
}
class List () {
  this._list = []
  consruct(id) {
  }
  addItem(user)
  render () {
      doucment.getElemntBy(id).innerHtml = ''
      this._list.forEach(user => {
         // добавление новой строки
     })
  }
   
  fetcList () {
     return аяк-запрос.then(users => {
        this._lsit = []
         users.forEachv(user => {
             this.addItem(user)
        })
    })
   }
}
new Subsriber {
  action,
  cb
}
class Emitter {
    this._evts = []
    subsctibe(action, cb) {
       this.evts.push(new Subsriber (action, cb))
    }
    emit(action, value) {
        this.evts.forEach({action} => {
       if (act === action) {
           cb(value)
       }
     })
    }
}
class Form (){
  consruct(id, emmiter) {
    doucment.getElemntBy(id).addEventListener('sumbit', () => this.onSubmit)
  }
  addElement(elem) {
     this.elements.push(el)
   }
  onSubmit () {
      const user = new User()
      for (let elem of this.elements) {
          user[elem.name] = elem.value
     }
     user.save().then(добавленный юзер => {
           this.emmiter.emit('submit', user)
     })
  }
  }

  render () {
      doucment.getElemntBy(id).innerHtml = ''
      this.elements.forEach(elem => {
         doucment.getElemntBy(id).insertBefore(elem)
     })
  }
}
// инициализация этого говна
const list = new List(listId)
const emmiter = new Emiiter()
const form = new Form(formId, emmiter)
form.addElement(document.createElemnt('input'))

// теперь отрисовка
form.render()
list.render()

// подписываемся на события формы
emmiter.subscribt('sumbit', user => {
    list.addItem(user)
    if (не грузим с сервера) {
      // перерисуем
         list.render()
    } else {
        // перерисуем
         list.fetchList().then(() => ist.render())
     }
})

Answer 3

[Павло Пономаренко]

// создание полей input
    Form.prototype.createInput = function createInput(e) {
        for (let i = 0; i < e.length; i++) {
            this.input = document.createElement('input');
            this.input.type = e[i].type;
            this.input.classList = 'js-input';
            this.input.name = e[i].name;
            this.input.placeholder = 'Введите ' + e[i].content + ':';
            this.input.id = e[i].id;
            this.input.value = '';
            this.formWrap.append(this.input);
        }
    }

Тут вместо this.input нужна более локальная let input