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

Всем привет! Решил попробовать свои силы на должность 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'
        }
    ],
});
  • Вопрос задан
  • 1992 просмотра
Решения вопроса 2
rockon404
@rockon404
Frontend Developer
1. Вы инкапсулируете модуль Form, но при этом он зависит от глобальных переменных, а не получает нужные параметры при создании экземпляра.
2. Сам объект Form это ни что иное, как наглядная демонстрация антипаттерна God Object . Почему, не имеющий никакого отношения к форме, список является ее частью остается загадкой. Как и то почему в модуле инкапсулировано состояние приложения.
3. Насчет аргумента "e" уже не раз написали. Насколько я понимаю, вы видели на просторах интернета, что так часто называют аргумент функции, но, видимо, не поняли почему. Один аргумент имеет говорящее название user, но и оно вводит в заблуждение, так как на вход ожидается массив пользователей.
4. Почему не использованы возможности ES6 остается загадкой.
5. Попробуйте сами догадаться, что не так с этим отрывком кода.
users.push(this.user);

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

6. У вас почти все переменные в методах объявлены свойствами объекта, при том, что в этом нет никакой необходимости и это может стать причиной ошибок в дальнейшем. Почему не использованы локальные переменные остается загадкой.
Ответ написан
Комментировать
@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())
     }
})
Ответ написан
Комментировать
Пригласить эксперта
Ответы на вопрос 1
// создание полей 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
Ответ написан
Комментировать
Ваш ответ на вопрос

Войдите, чтобы написать ответ

Войти через центр авторизации
Похожие вопросы