1
\$\begingroup\$

For a homework assignment, I have to create a "TO-DO" list, which will add a list item to the unordered list dynamically using JavaScript. The input element is also added using JavaScript, having to generate a unique id that changes with each site visit. I'm looking for any suggestions that will improve the efficiency of the code.

/*
 - get user input
 - create <li> element
 - append that to <ul> list
*/

function Form_Processes() {

	this.count = 0;
	this.input_id = generate_id();

	this.addToList = function() {
		var itemToAdd = document.getElementById(this.input_id).value;
		if(!(itemToAdd === "")) {
			this.count += 1;
			var list_tag = document.getElementById('list');
			var li_tag = document.createElement('li');
			li_tag.id = "li_" + this.count;
			var li_text = document.createTextNode("- " + itemToAdd);
			li_tag.appendChild(li_text);
			list_tag.appendChild(li_tag);

			//clear textbox for next input
			document.getElementById(this.input_id).value = "";
		}
	}

	this.removeFromList = function() {
		var numsOfElements = document.getElementById('list').childElementCount;
		if(numsOfElements > 0) {
			var itemToRemove = document.getElementById('li_' + this.count);
			itemToRemove.outerHTML = "";
			this.count -= 1;
		} else {
			console.log('ERROR: Not Enough Child Elements to remove (script.js:25)');
		}
	}

	function generate_id() {
  		var text = "";
  		var possible = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_";

  		for (var i = 0; i < 8; i++){
    		text += possible.charAt(Math.floor(Math.random() * possible.length));
  		}
  		return text;
	}

}

var f = new Form_Processes();

function execute(x) {
	if(x == 1) {
		f.addToList()
	} else if(x == 2) {
		f.removeFromList();
	}
}

function load_form() {
	var x = document.getElementById('in');

	//create input tag
	var input_tag = document.createElement('input');
	input_tag.type = "text";
	input_tag.name = "user_input";
	input_tag.id = f.input_id;
	input_tag.placeholder = "Enter here...";

	x.appendChild(input_tag);

	//insert line break
	var break_tag = document.createElement('br');
	x.appendChild(break_tag);

	var button1 = createButton("button", "add_list", 1, "Add To List");
	var button2 = createButton("button", "remove_list", 2, "Remove From List");

	x.appendChild(button1);
	x.appendChild(button2);

}

function createButton(type, name, e, text) {
	//create buttons
	var button_tag = document.createElement('button');
	button_tag.type = type;
	button_tag.name = name;
	button_tag.onclick = function() { execute(e); }

	var button_text = document.createTextNode(text);
	button_tag.appendChild(button_text);

	return button_tag;
}
* {
	font-family: "Times New Roman", Times, serif;
}

body {
	background-color: pink;
}

fieldset {
	border: none;
}

input[type=text] {
	padding: 12px 20px;
	border-radius: 5px;
	border: 1px solid grey;
	font-size: 25px;
	margin: 2px;
}

button {
	background-color: green;
	border: none;
	border-radius: 6px; /* rounded corner look */
	color: white; /* button text color */
	text-align: center;
	font-size: 16px;
	margin: 1px;
}

ul {
	list-style-type: none;
  	margin: 0;
  	padding: 0;
}

li {
	background-color: grey;
	margin: 3px;
	font-size: 50px;
	border-radius: 5px;
	color: white;
	padding-left: 15px;
}
<!DOCTYPE html>
<html lang="en-US">
	<head>
		<meta charset="UTF-8">
		<meta name="viewport" content="width=device-width, initial-scale=1.0">
		<title>TO-DO</title>
		<script src="script.js"></script>
		<link rel="stylesheet" type="text/css" href="style.css">
	</head>
	<body onload="load_form()">
		<center>
			<form action="">
				<fieldset id="in">
					<!-- content goes below -->
				</fieldset>
			</form>
			<h1>TO-DO</h1>
		</center>
		<ul id="list">
			<!-- items will go below once added -->
		</ul>
	</body>
</html>

\$\endgroup\$
2
\$\begingroup\$

I recommend putting the script at the end of the page, just before </body> The reason is that you'll want to have scripts run when the DOM is ready (i.e. all the elements are present and ready for JS manipulation).

<center> is a deprecated stylistic element. To center an element, set left and right margins to auto. For text, use text-align: center:

.centered-element {
  display: block;
  width: 250px; /* Your desired width */
  margin-left: auto;
  margin-right: auto;
}

.element-with-centered-text {
  text-align: center;
}

This way, you have separation of concerns. HTML deals with describing the structure, CSS deals with styling your HTML.

* {
    font-family: "Times New Roman", Times, serif;
}

This is unnecessary. Applying font-family to body should be enough to apply it to all elements. Font style is inherited by all descendants until an element overrides it (and styles its own descendants).

fieldset {...}

input[type=text] {...}

button {...}

ul {..}

li {...}

I don't recommend styling elements directly because it breaks expectations. If I add a <ul> in your app for whatever purpose, I expect it to have bullets. But since your CSS removes them, my list won't have bullets. I'd have to add them back when, by default, they should have had bullets.

Instead, use classes to target app-specific styling and leave element defaults alone. The only time I would style an element directly is if it's part of a globally-applied theme or a normalizer.

As for your JavaScript, you could abstract away your element creation with a function like the following. This way, you don't have to repeat element creation scripts everywhere and you can easily describe dynamic HTML in a nested manner, like HTML.

const e = (name, properties = {}, children = []) => {
  // Create the element
  const element = document.createElement(name)

  // Apply properties
  Object.keys(properties).forEach(property => {
    element.setAttribute(property, propertyes[property])
  })

  // Append children
  children.forEach(c => {
    if(!c) return
    const node = (typeof c === 'string') ? document.createTextNode(c) : c
    element.appendChild(node)
  })

  return element
}

// Usage
const root = e('div', {}, [
  e('p', {}, [
    e('span', {}, [
      'Hello, World!',
      'Lorem Ipsum'
    ])
  ])
])

root.appendChild(e('span', {}, ['another piece of text']))

If the syntax looks familiar, it's because this is the basic premise of how most VDOM libraries work under the hood. They're just nested calls of functions that either return an actual element, or objects that represent elements (which are turned to elements later).

From there, you can describe and build your "components" like:

const LoadForm = () => {
  e('div', {}, [
    e('input', { type: 'text', name: 'user_input', placeholder: 'Enter here...' }),
    e('br'),
    e('button', { name: 'add_list' }, ['Add To List']),
    e('button', { name: 'remove_list' }, ['Remove From List']),
  ])

document.getElementById('in').appendChild(LoadForm())

You can even encapsulate this in a class if you want so that you can also add some helper methods.

class LoadForm {
  onAdd () {
    // Do something on add
  }
  onRemove () {
    // Do something on remove
  }
  render() {
    return (
      e('div', {}, [
        e('input', { type: 'text', name: 'user_input', placeholder: 'Enter here...' }),
        e('br'),
        e('button', { name: 'add_list', onclick: this.onAdd }, [
          'Add To List'
        ]),
        e('button', { name: 'remove_list', onclick: this.onRemove }, [
          'Remove From List'
        ]),
      ])
    )
  }
}

document.getElementById('in').appendChild((new LoadForm()).render())

Lastly, JavaScript is written in camelCase, not snake_case.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.