Как лучше организовать код и вообще улучшить код?

Question

[Swaer]

let resa = document.querySelector('.todo_ul');
let addLi = document.querySelector('.todo_add');
let inputLi = document.querySelector('.todo_input');

// Событие на кнопку "Добавить"
addLi.addEventListener('click',function(e){
  //Отключает перезагрузку страниц
  e.preventDefault();
  let ul = document.querySelector('.todo_ul');
  //В список при нажатии на кнопку "Добавить" добавляется новый элемент списка со значением из инпута
  ul.appendChild(addNewLi(inputLi.value))
  //Очищаем инпут
  inputLi.value = '';
});
//Функция для создания элемента списка
function addNewLi(value){
  if(value === ''){
    return false
  };
  //Создаем элемент списка
  let li = document.createElement('li');
  //Добавляем ему класс
  li.className = 'todo_li';
  //Создаем кнопку 'Check'
  let divInLi = document.createElement('div');
  //Добавляем кнопке класс
  divInLi.className = 'todo_check';
  //Создаем кнопку "Изменить" для изменения значения элемента списка
  let changeButton = document.createElement('input');
  //Добавляем кнопке "Изменить" атрибуты
  changeButton.setAttribute('type','button');
  changeButton.setAttribute('value','Изменить');
  //Добавляем кнопке "Изменить" классы
  changeButton.className = 'todo_li_change li_button';
  //Создаем кнопку "Удалить" для удаления элемента списка
  let deleteButton = document.createElement('input');
  //Добавляем кнопке "Удалить" атрибуты
  deleteButton.setAttribute('type','button');
  deleteButton.setAttribute('value','Удалить');
  //Добавляем кнопке "Удалить" классы
  deleteButton.className = 'todo_li_delete li_button';
  //Создаем кнопку "Сохранить" для сохранения измененого значения в элемент списка
  let saveButton = document.createElement('input');
  //Добавляем кнопке "Удалить" атрибуты
  saveButton.setAttribute('type','button');
  saveButton.setAttribute('value','Сохранить');
  //Добавляем кнопке "Удалить" классы
  saveButton.className = 'save_change_item li_button';
  //Создаем поле для изменения значения элемента списка
  let saveInput = document.createElement('input');
  //Добавляем полю атрибуты
  saveInput.setAttribute('type','text');
  //Добавляем полю классы
  saveInput.className += 'change_item';
  //Создаем текстовое поле, куда будет записанно значение элемента списка
  let p = document.createElement('p');
  //Добавляем полю класс
  p.className = 'li_text';
  //В него записываем значение параметра
  p.innerHTML = value;
  //Все созданные элементы добавлчем в новый элемент списка
  li.appendChild(p);
  li.appendChild(divInLi);
  li.appendChild(changeButton);
  li.appendChild(deleteButton);
  li.appendChild(saveButton);
  li.appendChild(saveInput);
  //Выполняить функцию, а качестве аргумента - нвоый созданый элемент списка
  doEvent(li);
  
  return li
};
//функция для создания событий на элементы внутри элемента списка
function doEvent(toDoItem){
  let toDoLi = toDoItem.querySelector('.todo_check');
  let changeButton = toDoItem.querySelector('.todo_li_change');
  let deleteButton = toDoItem.querySelector('.todo_li_delete');
  let saveButton = toDoItem.querySelector('.save_change_item');
   saveButton.addEventListener('click',saveItem);
   toDoItem.addEventListener('click',changeItem);
   toDoItem.addEventListener('mouseout',itemHidden);
   toDoItem.addEventListener('mouseover',itemShow);
   toDoLi.addEventListener('click',checkBox);
   changeButton.addEventListener('click',changeBox);
   deleteButton.addEventListener('click',deleteBox);
}
//При нажатии на кнопку "Check" добавляем или удаляет класс кнопке "Check" и элементу списка
function checkBox(){
  this.classList.toggle('check');
  this.parentNode.classList.toggle('checked')
}
//При нажатии на элемент списка или его текст добавляем или удаляет класс кнопке "Check" и элементу списка
function changeItem(e) {
  if(e.target.nodeName == 'LI'){
    let check = e.target.querySelector('.todo_check');
    e.target.classList.toggle('checked');
    check.classList.toggle('check');
  }else if(e.target.nodeName == 'P'){
    let check = e.target.parentNode.querySelector('.todo_check');
    e.target.parentNode.classList.toggle('checked');
    check.classList.toggle('check');
  }
}
//При нажатии на кнопку "Измнеить", будут показаны поле ввода со значением из элемента списка и кнопка "Сохранить", для изменения значения элемента списка и его сохранения, кнопки "Изменить" "Удалить" будут скрыты
function changeBox(){
  let save = this.parentNode.querySelector('.save_change_item');
  let saveInput = this.parentNode.querySelector('.change_item');
  let deleteButton = this.parentNode.querySelector('.todo_li_delete');
  let litext = this.parentNode.querySelector('.li_text'); 
  saveInput.value = litext.innerHTML;
  save.style.visibility = 'visible';
  saveInput.style.visibility = 'visible';
  this.className += ' disp_none';
  deleteButton.className += ' disp_none';
}
//При нажатии на кнопку "Сохранить", значение поля ввода будет сохранено в значение элемента списка, поле ввода и кнопка "Сохранить" будут скрыты, кнопки "Изменить" "Удалить" будут снова видны
function saveItem(){
  let saveInput = this.parentNode.querySelector('.change_item'); 
  let litext = this.parentNode.querySelector('.li_text'); 
  litext.innerText = saveInput.value;
  let save = this.parentNode.querySelector('.save_change_item');
  let deleteButton = this.parentNode.querySelector('.todo_li_delete');
  let chandeButton = this.parentNode.querySelector('.todo_li_change');
  save.style.visibility = 'hidden';
  saveInput.style.visibility = 'hidden';
  deleteButton.classList.remove('disp_none')
  chandeButton.classList.remove('disp_none')
}
//Удаление элемента списка при нажатии на кнопку "Удалить"
function deleteBox(){
  this.parentNode.remove()
}
//При наведении на элемент списка будут показаны кнопки "Изменить" "Удалить"
function itemShow(){
  this.querySelector('.todo_li_change').style.visibility = 'visible';
  this.querySelector('.todo_li_delete').style.visibility = 'visible';
}
// При выходе курсора с элемента списка кнопки "Изменить" "Удалить" будут скрыты
function itemHidden(){
  this.querySelector('.todo_li_change').style.visibility = 'hidden';
  this.querySelector('.todo_li_delete').style.visibility = 'hidden';
}

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

Comments

[dollar]

В коде не хватает комментариев, как минимум

[Swaer]

dollar, Счас сделаю

Answer 1

[forspamonly2]

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

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

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

кнопки "Изменить" "Удалить" лучше показывать не скриптом, а цсс.

ну и самое главное, вы фактически модель данных держите прямо в хтмл элементах. это приемлемо работает только на примитивных примерах, вроде вашего. в большинстве реальных случаев куда проще иметь разделение на модель данных, которыми оперирует скрипт, и их отображение в виде хтмл. почитайте про паттерн model-view-controller.

Comments

[Swaer]

let resa = document.querySelector('.todo_ul');
let addLi = document.querySelector('.todo_add');
let inputLi = document.querySelector('.todo_input');

// Событие на кнопку "Добавить"
addLi.addEventListener('click',function(e){
  //Отключает перезагрузку страниц
  e.preventDefault();
  let ul = document.querySelector('.todo_ul');
  //В список при нажатии на кнопку "Добавить" добавляется новый элемент списка со значением из инпута
  ul.appendChild(addNewLi(inputLi.value))
  //Очищаем инпут
  inputLi.value = '';
});
//Функция для создания элемента списка
function addNewLi(value){
  if(value === ''){
    return false
  };
  
  let li = createElem('li',{class:'todo_li'})
  let divInLi = createElem('div',{class:'todo_check'})
  
  let changeButton = createElem('input',{type:'button',value:'Изменить',class:'todo_li_change li_button'});
  let deleteButton = createElem('input',{type:'button',value:'Удалить',class:'todo_li_delete li_button'});
  let saveButton = createElem('input',{type:'button',value:'Сохранить',class:'save_change_item li_button'});
  let saveInput = createElem('input',{type:'text',class:'change_item'});
  
  let p = createElem('p',{class:'li_text'});
  
  p.innerHTML = value;
  li = appendChilds(li,p,divInLi,changeButton,deleteButton,saveButton,saveInput)
  
  //Выполняить функцию, а качестве аргумента - нвоый созданый элемент списка
  //doEvent(li);
  
  return li
};
function createElem(tag,props){
  let div = document.createElement(tag);
  for( let key in props){
    div.setAttribute(key, props[key])
  }
  return div
};
function appendChilds(obj,...arg){
  for(let i = 0; i < arg.length; i++){
    obj.appendChild(arg[i]);
  }
  return obj
};

resa.addEventListener('click', function(e){
  
  console.log(e.target.className)
  if(e.target.tagName === 'P'){
    let check = e.target.parentNode.querySelector('.todo_check');
    check.classList.toggle('check');
    e.target.parentNode.classList.toggle('checked');
  }else if(e.target.tagName === 'LI'){
    let check = e.target.querySelector('.todo_check');
    check.classList.toggle('check');
    e.target.classList.toggle('checked');
  }else if(e.target.classList.contains("todo_li_change")){
    let qSelector = e.target.parentNode;
      let save = qSelector.querySelector('.save_change_item');
      let saveInput = qSelector.querySelector('.change_item');
      let deleteButton = qSelector.querySelector('.todo_li_delete');
      let litext = qSelector.querySelector('.li_text'); 
      saveInput.value = litext.innerHTML;
      save.style.visibility = 'visible';
      saveInput.style.visibility = 'visible';
      e.target.className += ' disp_none';
      deleteButton.className += ' disp_none';
  }else if(e.target.classList.contains('save_change_item')){
    let saveInput = e.target.parentNode.querySelector('.change_item'); 
    let litext = e.target.parentNode.querySelector('.li_text'); 
    let save = e.target.parentNode.querySelector('.save_change_item');
    let deleteButton = e.target.parentNode.querySelector('.todo_li_delete');
    let chandeButton = e.target.parentNode.querySelector('.todo_li_change');
    
    litext.innerText = saveInput.value;
    save.style.visibility = 'hidden';
    saveInput.style.visibility = 'hidden';
    deleteButton.classList.remove('disp_none')
    chandeButton.classList.remove('disp_none')
  }else if(e.target.classList.contains('todo_li_delete')){
    e.target.parentNode.remove()
  }else if(e.target.classList.contains('todo_check')){
    e.target.classList.toggle('check');
    e.target.parentNode.classList.toggle('checked')
  }
})

Как-то так получилось

[forspamonly2]

если честно, лучше не стало. для простой задачи код тоже должен быть простым.

в качестве благотворительности, я вам провёл почти полноценный код ревью. замечания и варианты "как надо" поместил по тексту вашего исходника в многострочных комментариях /* ... */ свои варианты не проверял - тестировать это мне не на чем. и самое главное, перечислены изменения в плане реализации, а не в плане подхода - кроме всего этого, вам надо менять подход и модель данных выделять.

/* эти три let вполне могут быть const */
let resa = document.querySelector('.todo_ul');
let addLi = document.querySelector('.todo_add');
let inputLi = document.querySelector('.todo_input');

// Событие на кнопку "Добавить"
addLi.addEventListener('click',function(e){
	//Отключает перезагрузку страниц
	e.preventDefault();  /* перезагрузки изначально не должно быть, вы наверное неподходящий тип кнопки выбрали? */
	let ul = document.querySelector('.todo_ul'); /* ссылка на этот элемент уже получена выше под именем resa */
	//В список при нажатии на кнопку "Добавить" добавляется новый элемент списка со значением из инпута
	ul.appendChild(addNewLi(inputLi.value)) /* разумнее само заталкивание иметь внутри соответствующего метода, а не тут */
	//Очищаем инпут
	inputLi.value = '';
});
/*
в результате, ваш обработчик может выглядеть так:

addLi.addEventListener('click',function(e){
	addNewLi(inputLi.value));
	inputLi.value = '';
});

*/

//Функция для создания элемента списка
/* это всё я уже советовал заменить на шаблон с кусочком html */
function addNewLi(value){
	if(value === ''){
		return false
	}; /* в таких местах точка с запятой не нужна */

	let li = createElem('li',{class:'todo_li'}) /* а в таких наоборот нужна.  много мест. все помечать не буду */
	let divInLi = createElem('div',{class:'todo_check'})

	let changeButton = createElem('input',{type:'button',value:'Изменить',class:'todo_li_change li_button'});
	let deleteButton = createElem('input',{type:'button',value:'Удалить',class:'todo_li_delete li_button'});
	let saveButton = createElem('input',{type:'button',value:'Сохранить',class:'save_change_item li_button'});
	/* класс li_button почти наверняка лишний, вы спокойно можете в css привязаться к родительскому классу, особенно если тэг на button поменяете */
	let saveInput = createElem('input',{type:'text',class:'change_item'});

	let p = createElem('p',{class:'li_text'});

	p.innerHTML = value;  /* это у вас натуральная XSS уязвимость.  что будет если пользователь введёт <script>вредоносный код</script> ?*/
	li = appendChilds(li,p,divInLi,changeButton,deleteButton,saveButton,saveInput)

	//Выполняить функцию, а качестве аргумента - нвоый созданый элемент списка
	//doEvent(li);

	return li
};
function createElem(tag,props){
	let div = document.createElement(tag);
	for( let key in props){
		/* в таких местах должна быть проверка if(props.hasOwnProperty(key)), а то могут непонятно от кого унаследованные поля вылезти  */
		/* или for(const key of Object.keys(props)) */
		div.setAttribute(key, props[key])
	}
	return div
};
function appendChilds(obj,...arg){  /* правильно children */
	for(let i = 0; i < arg.length; i++){ /* тут был бы уместен какой-нить arg.forEach(x => obj.appendChild(x)) */
		obj.appendChild(arg[i]);
	}
	return obj
};

/*
всё добавление тудушки может выглядеть так:

function addNewLi(value){
	if(!value) return;

	const li = document.createElement('li');
	li.outerText = `
		<li class="todo_li">
			<p class="li_text"></p>
			<div class="todo_check"></div>
			<button class="todo_li_change">Изменить</button>
			<button class="todo_li_delete">Удалить</button>
			<button class="save_change_item">Сохранить</button>
			<input type="text" class="change_item">
		</li>`;
	li.querySelector('.li_text').textContent = value;

	resa.appendChild(li);
}
 */

resa.addEventListener('click', function(e){ /* когда кода много, обработчики надо разобрать на отдельные именованные по смыслу методы.  но этот код почти весь лишний  */

	console.log(e.target.className)
	if(e.target.tagName === 'P'){
		let check = e.target.parentNode.querySelector('.todo_check');
		check.classList.toggle('check'); /* дублирующиеся по смыслу классы у чекбокса и родителя, достаточно родительского, а в цсс использовать селектор .todo_li.checked .todo_check */
		e.target.parentNode.classList.toggle('checked');
	}else if(e.target.tagName === 'LI'){ /* простановка галки повторяется аж три раза, явно зря */
		let check = e.target.querySelector('.todo_check');
		check.classList.toggle('check');
		e.target.classList.toggle('checked');
	}else if(e.target.classList.contains("todo_li_change")){
		let qSelector = e.target.parentNode;
		let save = qSelector.querySelector('.save_change_item');
		let saveInput = qSelector.querySelector('.change_item');
		let deleteButton = qSelector.querySelector('.todo_li_delete');
		let litext = qSelector.querySelector('.li_text');
		saveInput.value = litext.innerHTML;
		save.style.visibility = 'visible'; /* все переключения видимости должны быть сделаны в css. тут надо только добавлять и снимать тудушке класс .edited, а в цсс при наличии класса показываются инпут и кнопка сохранить, а при отсутсвии - кнопки редкатирования/удаления и параграф с текстом тудушки */
		saveInput.style.visibility = 'visible';
		e.target.className += ' disp_none';
		deleteButton.className += ' disp_none';
	}else if(e.target.classList.contains('save_change_item')){
		let saveInput = e.target.parentNode.querySelector('.change_item');
		let litext = e.target.parentNode.querySelector('.li_text');
		let save = e.target.parentNode.querySelector('.save_change_item');
		let deleteButton = e.target.parentNode.querySelector('.todo_li_delete');
		let chandeButton = e.target.parentNode.querySelector('.todo_li_change');

		litext.innerText = saveInput.value;
		save.style.visibility = 'hidden';
		saveInput.style.visibility = 'hidden';
		deleteButton.classList.remove('disp_none')
		chandeButton.classList.remove('disp_none')
	}else if(e.target.classList.contains('todo_li_delete')){
		e.target.parentNode.remove()
	}else if(e.target.classList.contains('todo_check')){
		e.target.classList.toggle('check');
		e.target.parentNode.classList.toggle('checked')
	}
})

/*
итого, делегированная обработка событий получается примерно такая:

resa.addEventListener('click', function(e){
	const el = e.target;
	const li = el.closest(".todo_li");

	if(!li) return;

	if(el.matches(".todo_li_change")) {
		li.querySelector('.change_item').value = li.querySelector('.li_text').textContent;
		li.classList.add('edited');
	}
	else if(el.matches(".save_change_item")) {
		li.querySelector('.li_text').textContent = li.querySelector('.change_item').value;
		li.classList.remove('edited');
	}
	else if(el.matches(".todo_li_delete")) {
		li.remove();
	}
	else {
		li.classList.toggle('checked');
	}
});
*/

[Swaer]

forspamonly2, Спасибо большое, теперь новые подходы знаю, я сейчас как раз хотел сделать тудушку как могу, и что-то новое узнать, а потом переделать с помощью MVC подхода