Дерево файловой структуры (на JS). Ваша критика по коду

Написал на JS обычное дерево файловой структуры. (добавление, редактирование, удаление папки), после каждого события объект перезаписывается в localStorage и при перезагрузки страницы отрисовка идет уже измененного объекта.

Хотелось бы получить от вас критику по коду, как говорится - “разнесите меня в щепки”, уровень js у меня - junior, хочу развиваться дальше, поэтому прошу вашей критики.

Рабочий вариант по ссылке - http://kustov-work.ru/

HTML

<div class="wrapper">
        <div id="file_structure">

        </div>
    </div>

JS

window.onload = function() {

    const parent_tree = document.querySelector('#file_structure');

    $.getJSON("ajax/tree.json",function(result) {
        let tree; // создаем переменную которая будет ссылкой на объект дерева

        if (localStorage.getItem('tree') != null) { // проверка наличия объекта в localStorage
            tree = JSON.parse(localStorage.getItem('tree')); // возвращенный объект из localstorage
            console.log('объект подгруженный из localStorage -', tree);
        } else {
            tree = result; // запись результата JSON в переменную
            console.log('объект подгруженный из JSON -', tree);
        }

        drawingTree(tree, parent_tree);

        parent_tree.onclick = function(e) {
            let el = e.target;
            let el_parent = findAncestor(el ,'li');
            let el_parent_id = el_parent.id;
            let arr_path = el_parent_id.split('.'); // разбиваем полученный id на массив, это будет путь до текущего вложенного объекта
            let path_obj = findFolder(arr_path, tree); // путь до ключа объекта

            /*событие для добавления новой папки*/
            if (el.classList.contains('add_folder')) {
                let nameFolder = prompt('Введите название папки');

                if (nameFolder === null || nameFolder === '') {
                    return;
                } else {
                    let el_list = el_parent.querySelector('ul');
                    let elem_li = document.createElement('li');

                    if (el_list === undefined || el_list === null) {
                        el_list = document.createElement('ul');
                        el_parent.append(el_list);
                    }
                    addElementInObj(path_obj, nameFolder, el_list, elem_li);
                }

            /*событие для изминения имени папки*/
            } else if (el.classList.contains('edit_folder')) {
                let editNameFolder = prompt('Введите новое название папки');
                if (editNameFolder === null || editNameFolder === '') {
                    return;
                } else {
                    let el_span = el_parent.querySelector('span');
                    editElementInObj(path_obj, el_span, editNameFolder);
                }

            /*событие для удаления папки*/
            } else if (el.classList.contains('remove_folder')) {
                let el_list = el_parent.parentElement;

                el_parent.remove();
                /*проверка, если в ul не осталось потомков li, то удаляем родительский ul*/
                if (el_list.querySelectorAll('li').length === 0) {
                    el_list.remove();
                }

                removeElementInObj(path_obj, tree);
            }
            loacalStorageWrite(tree);
            console.log('Измененый объект, после события -', tree);
        }
    });

    function loacalStorageWrite(obj) {
        localStorage.clear(); // очищаем localStorage
        let serialObj = JSON.stringify(obj);
        localStorage.setItem('tree', serialObj);
    }
    /*Добавление новой папки в объект и отображеие на странице*/
    function addElementInObj(path, nameFolder, el_list, el_li) {
        let id = generateID(path);

        elemCreate(el_list,el_li,nameFolder, id[0]);

        path.dother['div'+id[1]] = {
            nameFolder: nameFolder,
            id: id[0],
            dother: {}
        };
    }
    /*изинения имени папки (в объекте)*/
    function editElementInObj(path, el_span, editNameFolder) {
        path.nameFolder = editNameFolder;
        el_span.innerHTML = editNameFolder;
    }
    /*удаление ключа папки*/
    function removeElementInObj(path , obj) {
        delete path.id;
        delete path.nameFolder;
        delete path.dother;

        removeEmptyObj(obj);
    }

    function removeEmptyObj(obj) { // удаляет пустой ключ объекта
        for (var key in obj) {
            if (isEmpty(obj[key])) {
                delete obj[key];
                return false;
            }
            removeEmptyObj(obj[key]['dother']);
        }
    }
    /*генерируем id в зависимости от вложенности текущей папки*/
    function generateID (path) {
        let this_id = path.id; // текущий id
        if (typeof path.dother === 'undefined') { // если по нашему пути еще нет созданных дочерних папок, то создаем для них объект
            path.dother = {};
        }
        /*определяем номер последней дочерней папки по нашему пути*/
        /*в дальнейшем этот номер используем для создания следующей папки с новым номером, чтобы объект не перезаписывался*/
        let arr_dother = Object.keys(path.dother);
        let number_dother;
        if (arr_dother.length === 0) {
            number_dother = 0;
        } else {
            let arr_number_dother = arr_dother[arr_dother.length - 1].split('v');
            number_dother = arr_number_dother[1];
        }

        let count_dother = Number(number_dother) + 1;

        let id_result = this_id + '.' + count_dother; // генерация id
        let arr = [id_result,count_dother]; // id для папки/номер для ключа div в объекте

        return arr;
    }

    /*поиск нужного родителя*/
    function findAncestor (el, cls) {
        while ((el = el.parentElement) && !el.matches(cls));
        return el;
    }

    /*ищет путь по объекту до нужного ключа*/
    function findFolder(array, obj) {
        let newLinkObject = Object;
        for (var i = 0; i < array.length; i++) {
            if (isEmpty(newLinkObject)) {
                newLinkObject = obj['div'+array[i]];
            } else {
                newLinkObject = newLinkObject['dother']['div'+array[i]];
            }
            if (array.length === i+1) {
                return newLinkObject;
            }
        }
        return newLinkObject;
    }

    /*проверка объекта на пустоту*/
    function isEmpty(obj) {
        for (var key in obj) {
            return false;
        }
        return true;
    }

    /*отрисовка дерева в DOM*/
    function drawingTree(obj,parent) {

        let obgKey = obj;
        let element = document.createElement('ul');
        if (obgKey != null) {
            for (var key in obj) {
                if (Object.keys(obj[key]).length !== 0) {
                    let elem_li = document.createElement('li');

                    elemCreate(element,elem_li,obj[key]['nameFolder'],obj[key]['id']);
                    parent.append(element);

                    if (typeof obj[key]['dother'] !== "undefined" && obj[key]['dother'] !== null) {
                        drawingTree(obj[key]['dother'],elem_li);
                    } else {
                        continue;
                    }
                }
            }
        } else {
            return false;
        }
    }

    /*создает элемент дерева в DOM*/
    function elemCreate(element,elem_li,name,id) {
        let wrap = document.createElement('div');
        wrap.innerHTML = '<div class="item_img">'+
                            '<img src="../img/folder.png" alt="папка" title="папка"/>'+
                         '</div>'+
                         '<div class="item_name">'+
                            '<span>'+name+'</span>'+
                         '</div>', wrap.classList = 'item_wrap';
        elem_li.append(wrap);

        elem_li.id = id;
        element.append(elem_li);

        btnCreate(wrap,'button', '', 'add_folder');
        btnCreate(wrap,'button', '', 'edit_folder');
        btnCreate(wrap,'button', '', 'remove_folder');
    }

    function btnCreate(parent,type, text, class_name) {
        let btn_add = document.createElement(type);
        btn_add.innerHTML = text;
        btn_add.classList.add(class_name);
        parent.append(btn_add);
    }
}

Изначальный JSON

{
    "div1": {
        "nameFolder": "папка",
        "id": "1"
    }
}

1 Симпатия
  1. где вебпак? делать такое без модулей… не 2002 и даже не 2012 на дворе
  2. на кой бок jQuery если тут использована ровно 1 функция и я вижу что ты умеешь и без него. Есть фетч, есть аксиос и еще 100500 вариантов как сделать аякс
  3. где промисы? неужели тебе удобно делать отступы?
  4. почему названия то камелкейс, то нижнее подчеркивание?
  5. блокирующие операции
  6. .onclick
  7. path.dother[‘div’+id[1]] - думаешь сколько времени уйдет на понимание, когда таких моментов у тебя станет несколько десятков?
  8. а вот это while ((el = el.parentElement) && !el.matches(cls)); и само по себе много времени на понимание займет.
  9. почему не используешь строчные выражения?
  10. wrap.innerHTML = ‘
    ’+ … , wrap.classList = … - супер очевидно что после сплошного куска хтмл у тебя еще маленький кусочек жс.

    В сам алгоритм не вникал, долго.
    Из кода вижу что у тебя три проблемы:

    1. пишешь код для себя и читаешь его только ты сам
    2. устаревшие стандартны кода, нет использования современных инструментов
    3. плохо понимание работы асинхронноости и незнания ключевых особенностей написания асинхронного кода (иначе откуда onclick и блокирующие операции?), возможно незнания или непонимание основных паттернов.

    Такое, я бы на работу жсником не взял, даже как трейни, но для старта норм.
    Поставь еслинт со строгими настройками, например от аирбнб, остальное только опытом и опытом.

1 Симпатия

Спасибо за критику.

Из этих пунктов у меня возникают вопросы, если не трудно, поясни более развернуто.

  1. Зачем здесь вебпак ? какие npm тут устанавливать ? чистый js, html, css - только если компиляторы настроить и минифицировать файлы, ну и плюс препроцессоры? Просто этот проект для себя и дальнейшего развития у него не будет.

  2. Согласен, просто до этого работал только с Jquery и теперь очень много мусора в голове и нет как такого фундамента знаний JS (почитал и перепиал с помощью XMLHttpRequest)

  3. Про промисы услышал впервые (буду читать). О каких отступах идет речь?

  4. Теперь буду писать все в одном стиле.

  5. Нет понимание, что такое блокирующие операции. Хотелось бы узнать подробнее.

  6. Что с onclick не так?

  7. Тут у меня не хватает опыта, чтоб придумать более удобный вариант.

  8. Хм, а как мне искать нужного родителя ? Не писать ведь .parentElement.parentElement и т.д., а метод closest() не везде поддерживается.

  9. Что ты имеешь ввиду под строчными выражениями ? и где я их должен использовать? (пример)

  10. Я так понял, что это сарказм? ) Перепишу добавления класса отдельной строкой.

Вот на счёт 8-го пункта кому как.
Я ещё не читая код в начале топика, начал читать твой фидбэк. И прям сразу понял, что это имплементация closest. Заглянул в код, так там она выделена в отдельную функцию, да ещё и с комментарием!!! И было не понятно?!
Ну вы, батенька, придираетесь :)

3: Про отступы - Callback Hell

6: onclick -> AddEventListener

9: Шаблонные строки

От себя: btnCreate, судя по названию, занимается созданием кнопок (чем и занимается в коде), но по своему содержимому может создавать не только кнопки.
Называть переменные array и obj - плохо

1 Симпатия
  1. вебпак не только для нпм, вебпак в принципе стандарт на сегодня для написания браузерного жс, вебпак дает все то, что нельзя в быстрые сроки добавить в стандарты + заменяет кучу инструментов. С нпм у вас тут тот же аксиос, но даже не будет его, все равно вебпак нужен не только для нпм.
  2. XMLHttpRequest сейчас не используют, используй fetch или axios, для их понимания тебе понадобятся промисы.
  3. Промисы, в том числе и асинк\авейты (нужно знать и первое и второе), это колбеки в другом формате, позволяют избегать вложенности там где они ненужны.

Там где замечания про время, это не время чтения жсом, это время чтения другим программистом. Про остальное уже отписали.

  1. Не надо мешать тёплое с мягким. Webpack — это только сборщик проектов. Ближайшие аналоги Grunt и Gulp. Да-да они устроены по другим принципам, а галп вообще устарел. Я их привожу для аналогии и понимания назначения вебпака.

Это вообще не про Webpack это задача компайлеров. Например Babel

Да, у него есть куча настроек. Но опять же, львиную долю всё равно выполняют плагины к нему

так в основном говорят люди не умеющие работать с вебпаком.

Да ну ладно?! Покажи мастер класс. Как чистым вебпаком скомпайлить ES6+ с нестандартными фичами в ES5?

Это неконструктив и отклонение от темы. Думаю вам будет интереснее пообщаться в отдельной теме. Тут, пожалуйста, по поставленному вопросу в корневом топике.

Мой комментарий никак не связан с тем что писали другие авторы (ни дополняет ни опровергает их точку зрения).

let tree; // создаем переменную которая будет ссылкой на объект дерева

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

Суть этого кода - инициализация начального дерева. Я бы сделал отдельной функцией чтобы очертить зону ответственности кода.

if (localStorage.getItem('tree') != null) { // проверка наличия объекта в localStorage
    tree = JSON.parse(localStorage.getItem('tree')); // возвращенный объект из localstorage
    console.log('объект подгруженный из localStorage -', tree);
} else {
    tree = result; // запись результата JSON в переменную
    console.log('объект подгруженный из JSON -', tree);
}

parent_tree.onclick всегда используй addeventListener или библиотечные функции для навешивания обработчиков. Все равно придется их использоваться в других хоть сколько то нетривиальных приложениях.

let el = e.target;

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

let el_parent_id = el_parent.id;
let arr_path = el_parent_id.split('.'); // разбиваем полученный id на массив, это будет путь до текущего вложенного объекта

если хочешь хранить данные в DOM, используй data-attribute https://developer.mozilla.org/ru/docs/Web/Guide/HTML/Using_data_attributes. Это более ожидаемо, чем кодирование через класс или айдишник.

(nameFolder === null || nameFolder === '') Тут дело вкуса, но я бы упростил до if (nameFolder)

el_list === undefined || el_list === null очень правильно что используешь строгое сравнение. Иронично, но единственный допустимый (опять таки на мой вкус) случай когда можно использовать нестрогое сравнение, это для одновременного сравнения на null. el_list == null это эквивалент el_list === undefined || el_list === null.

const a = null
const b = undefined
const c = ''
const d = false
const e = 0

console.log(a == null) // true
console.log(b == null) // true
console.log(c == null) // false
console.log(d == null) // false
console.log(e == null) // false
/*событие для добавления новой папки*/
/*событие для изминения имени папки*/
/*событие для удаления папки*/

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

По стратегии.

Представить дерево в виде структуры данных это хорошая и правильная идея. А вот добавлять семантику айдишникам - не очень.

Нужно бить код на более независимые куски (т.е. функции), чем писать одной простыней.

Идеи findFolder можно выразить проще, но это придет с опытом. Просто знай что это возможно и пробуй (может нужна другая структура данных).

В целом направление мышления правильное. Продолжай.

1 Симпатия

создайте отдельную тему как порекомендовал @dmitry, то что вы написали это чистой воды демагогия, игра словами. Не делайте так, это очень запутывает новичков, которых тут полно.

Спасибо за критику.

Вот тут у меня вопрос ).

let el = e.target;

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

let el = e.target

будет менять своё значение по событию клика, почему ее нужно записывать в const? Она ведь не постоянная?

Тут несколько перспектив как можно смотреть на вопрос. Я смотрю (и в среднем программисты смотрят) на поведение let и const в контексте функции или блока кода. Т.е. мы видим:

onclick = function () {
// ...
let someVar = someVal
// ...
}

и это сигнализирует что в теле функции someVar будет перезаписана/изменена. Хоть на практике someVal и будет разным при разных вызовах onclick. Другими словами мы смотрим на поведение let и const в контексте языка, а не программы.

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

Хотел уточнить, если названия функции писать в camelCase, а переменные записывать с нижним подчеркиванием, то это будет плохая практика? Есть ли какие-то негласные стандарты ? Хочу выбрать стиль написания и придерживаться его.

parent_tree.onclick всегда используй addeventListener

Если я буду использовать addeventListner, то нужно ли после его использования удалить этот обработчик через removeEventListener ?

Суть в том что код должен быть в одном стиле, практика весьма гласная, используется повсеместно, нельзя смешивать два разных стиля кода. Конкретный стиль зависит от тебя, мы в компании например используем CamelCase, но нет никаких особых причин кому-то не использовать нижние подчеркивания, кроме личных предпочтений.

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