F
F
faleaksey2018-12-20 17:26:17
JavaScript
faleaksey, 2018-12-20 17:26:17

OOP in my test case, code review?

Hello! I decided to try my hand at the position of junior frontend developer...
They sent me a technical task that describes the following:
Write an application whose functions are as follows:
1) Add new employees and display them in a list
2) Create form elements dynamically, since they will need to be customized
3) Perform a simple check for filling in the fields
4) Ability to remove employees from the list
6) Display and update the number of employees
5) Optionally execute an ajax request when adding and update the list (using firebase)
It seems that I implemented everything except 5 points ... so far I decided to stop and get code review, dear toasters from you!
I look forward to healthy criticism and what can be improved :)
sandbox

// получаем доступ к ключевым элементам
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'
        }
    ],
});

Answer the question

In order to leave comments, you need to log in

2 answer(s)
A
Anton Spirin, 2018-12-20
@faleaksey

1. You encapsulate the Form module, but it depends on global variables instead of getting the right parameters when it is instantiated.
2. The Form object itself is nothing more than a clear demonstration of the God Object antipattern . Why, having nothing to do with the form, the list is part of it remains a mystery. As well as why the application state is encapsulated in the module.
3. About the argument "e" has already been written more than once. As far as I understand, you have seen on the Internet that the argument of a function is so often called, but, apparently, you did not understand why. One argument has a self-explanatory name user, but it is also misleading, since an array of users is expected as input.
4. Why ES6 features are not used remains a mystery.
5. Try to guess what is wrong with this piece of code.

users.push(this.user);

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

6. You have almost all the variables in the methods declared as properties of the object, despite the fact that there is no need for this and this can cause errors in the future. Why local variables are not used remains a mystery.

P
Pavlo Ponomarenko, 2018-12-20
@TheShock

// создание полей 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);
        }
    }

Here instead this.inputwe need a more locallet input

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question