JavaScript DOM Implementation

Question 1

I'm supposed to create a website that adds multiple forms to the page, is responsive and checks if the inputs are valid (validation is not important, just needs to show some attempt at regex). Below is what I've written so far. What I'm looking for is any advice on making it more efficient and compact. Any and all help is appreciated and considered. Thanks in advance!

index.html

<!DOCTYPE html>
<html lang="en-US">
 <head>
 <meta charset="UTF-8">
 <title>index.html</title>
 <script src="script.js"></script>
 <link rel="stylesheet" type="text/css" href="style.css">
 </head>
 <body onload="execute()" id="body">
 <h3>Content Below:</h3>
 <div id="buffer">
 <div id="content">
 <!-- Content will go here -->
 </div>
 </div>
 <div id="info">
 <!-- Info will go here -->
 </div>
 </body>
</html>

script.js

function ContentDisplayer() {
 this.count = 0;
 this.show = function(id) {
 var tag = document.getElementById(id);
 tag.style.display = 'block';
 }
 this.showText = function(id, content) {
 var tag = document.getElementById(id);
 tag.innerHTML = content;
 }
 this.constructForm = function(containing_id, question) {
 //Create div containing the form
 var div_tag = document.createElement('div');
 div_tag.id = 'div_' + this.count;
 document.getElementById('body').appendChild(div_tag);
 //Create the form tag
 var form_tag = document.createElement('form');
 form_tag.id = 'form_' + this.count;
 document.getElementById(div_tag.id).appendChild(form_tag);
 //Create the fieldset tag
 var fieldset_tag = document.createElement('fieldset');
 fieldset_tag.id = 'fieldset_' + this.count;
 document.getElementById(form_tag.id).appendChild(fieldset_tag);
 //Create question label
 var label_tag = document.createElement('label');
 var label_text = document.createTextNode(question);
 label_tag.appendChild(label_text);
 label_tag.id = 'label_' + this.count;
 document.getElementById(fieldset_tag.id).appendChild(label_tag);
 //insert line break
 var break_tag = document.createElement('br');
 document.getElementById(fieldset_tag.id).appendChild(break_tag);
 //Create answer label
 var input_tag = document.createElement('input');
 input_tag.id = 'input_' + this.count;
 input_tag.type = 'text';
 document.getElementById(fieldset_tag.id).appendChild(input_tag);
 //insert line break
 var break_tag = document.createElement('br');
 document.getElementById(fieldset_tag.id).appendChild(break_tag);
 //Create button
 var button_tag = document.createElement('button');
 var button_text = document.createTextNode('Submit');
 button_tag.appendChild(button_text);
 button_tag.type = 'button';
 button_tag.id = 'button_' + this.count;
 button_tag.onclick = function() {
 var x = document.getElementById(input_tag.id);
 if(input_tag.id == 'input_0') {
 if(/^[a-zA-Z]+$/.test(x.value)) {
 x.style.backgroundColor = "green";
 x.style.borderColor = "green";
 }
 }
 if(input_tag.id == 'input_1') {
 if((/^[0-9]+$/.test(x.value)) && x.value > 0 && x.value <= 100) {
 x.style.backgroundColor = "green";
 x.style.borderColor = "green";
 }
 }
 if(input_tag.id == 'input_2') {
 if(/^\w+@[a-zA-Z_]+?\.[a-zA-Z]{2,3}$/.test(x.value)) {
 x.style.backgroundColor = "green";
 x.style.borderColor = "green";
 }
 }
 if(input_tag.id == 'input_3') {
 if(/\d{1,5}\s\w{1,10}\s\w{1,10}/.test(x.value)) {
 x.style.backgroundColor = "green";
 x.style.borderColor = "green";
 }
 }
 if(input_tag.id == 'input_4') {
 if(/^\d{3}-\d{3}-\d{4}$/.test(x.value)) {
 x.style.backgroundColor = "green";
 x.style.borderColor = "green";
 }
 }
 }
 document.getElementById(fieldset_tag.id).appendChild(button_tag);
 this.count += 1;
 }
}
var c;
var questions = [
 'Enter your first name',
 'Enter your age',
 'Enter your email',
 'Enter your address',
 'Enter your phone number (must use dashes): ###-###-####'
];
var question_ids = [
 'name_content',
 'age_content',
 'email_content',
 'address_content',
 'phone_content'
];
function execute() {
 c = new ContentDisplayer();
 c.show('buffer');
 c.showText('content', '<h1>Hello!</h1>');
 c.showText('info', 'If the box turns green, the information is valid!');
 //create loop to add forms to page
 for (var i = 0; i < questions.length; i++) {
 c.constructForm(question_ids[i], questions[i]);
 }
}

style.css

body {
 font-family: "Times New Roman", Times, serif;
 background-color: pink;
}
.buffer {
 display: none;
}
input[type=text] {
 border: 2px solid red;
 border-radius: 4px;
 background-color: #f44242;
 margin: 1px;
}
input[type=text]:focus {
 border: 2px solid blue;
 border-radius: 4px;
 background-color: #41dcf4;
}
button {
 background-color: green;
 border: none;
 border-radius: 6px;
 color: white;
 text-align: center;
 font-size: 16px;
}

Question 2

The question title is a little misleading; my first thought was something like jsdom.

Question 3

Duplicate If-Statements

You have 5 validations that look all the same. You could write a function to get ride of the duplication.

The function could look like:

function makeGreenIfValidationIsValid(tagId, regex) {
 if(input_tag.id == tagId) {
 if(regex.test(x.value)) {
 x.style.backgroundColor = "green";
 x.style.borderColor = "green";
 }
 }
}

After that, the onClick-calback would look like

button_tag.onclick = function() {
 var x = document.getElementById(input_tag.id);
 makeGreenIfValidationIsValid('input_0', /^[a-zA-Z]+$/)
 makeGreenIfValidationIsValid('input_1', /^[0-9]+$/)
 makeGreenIfValidationIsValid('input_2', /^\w+@[a-zA-Z_]+?\.[a-zA-Z]{2,3}$/)
 makeGreenIfValidationIsValid('input_3', /\d{1,5}\s\w{1,10}\s\w{1,10}/)
 makeGreenIfValidationIsValid('input_4', /^\d{3}-\d{3}-\d{4}$/)
 document.getElementById(fieldset_tag.id).appendChild(button_tag);
 this.count += 1;
}

Extract Class

The method constructForm in ContentDisplayer should be a own class. An indicator for that is that it is huge (more than 80 lines) and you have many tag-comments in it.

Comments are add all not bad but when you group your code in a method you all ready see semi-independent logic. In Robert C. Martins words: "Don’t Use a Comment When You Can Use a Function or a Variable"

For example, the class might be named "Form" and could contain several methods. Based on your comments I could look like

function Form() {
 //Create div containing the form
 this.createDivTag() {}
 //Create the form tag
 this.createFormTag() {}
 //Create the fieldset tag
 this.createFieldsetTag() {}
}

The logic in create[tag-name]Tag for creating a div, form and fieldset looks very similar. We should extract the common logic into a function.

Prototyping

Currently ContentDisplayer and Form (the class from above) don't use it.

A disadvantage is that on each creation of an object all methods like show will be recreated each time. The result is that it costs performance.

With prototyping it would look like

function ContentDisplayer() {
 this.count = 0;
}
ContentDisplayer.prototype.show = function(id) {/**/}
ContentDisplayer.prototype.showText = function(id, content) {/**/}
// ...

Question 4

Compiled functions are cached. There is no performance gain by creating an external prototype. External prototypes do not allow the use of closure to create private members. Even way back when compilation was not cached external prototypes provided no benefit for single instance objects which is what is being used in this case. In modern JS external prototypes only provide a benefit when there are many instances (100+ to 1000+) with short lifetimes that are longer than a single GC cycle, with most of the small benifit in reduced GC overhead

Question 5

makeGreenIfValidationIsValid(tagId, regex) - I like the name! It's exactly what I'd come up with because I suck at being concise, so I just go with "long, boring but descriptive". Still, I'd personally inverse the order of the parameters passed in. That way you can very easily partially apply the function and derive more descriptive functions like validateNumeric = makeGreenIfValidationIsValid.bind.(this, /^[0-9]+$/) and then call validateNumeric("input_1").

Question 6

I'd agree with extracting the form creation into its own class. But I'd go a step further and make it a Builder. So you'd probably use it like formBuilder.addDiv(this.count).addForm(this.count).addLabel().addBreak().build(). Or something similar there is probably a better way to handle this, the point is how the interface is then exposed.

Question 7

Finally, for Prototyping, I wouldn't say it's a disadvantage. The code works perfectly fine as it is. Using a prototype might help but the claim that "it costs performance" is very likely a premature optimisation. Even if it is slower, it might not matter, depending on the use case. And for a homework assignment I'd definitely not care about this aspect. I'd leave it as a preference/option but not something that's really "wrong" with the code as it is.

Question 8

I realize the question already has an accepter answer, but I couldn’t help but notice you used the HTML5 doctype, which means you could use its new input types (such as email, tel and number) and their validation attributes (such as min, max and pattern).

That in combination with CSS’ :valid and :invalid pseudo-classes allows for real-time validation with 0 lines of javascript.

A simple quick incomplete example to illustrate above points:

<!DOCTYPE html>
<html lang="en-US">
 <head>
 <meta charset="UTF-8">
 <style>
 body { font-family: sans-serif }
 label { display: block }
 label span {
 display: inline-block;
 width: 8em;
 text-align: right;
 margin-right: .25em;
 }
 input {
 border-width: 2px;
 border-radius: 1ex;
 border-style: solid;
 }
 input:focus {
 background-color: #41dcf4;
 border-color: blue;
 }
 :valid { /* note that without a selector it applies also to whole form */
 background-color: green;
 border-color: green;
 }
 :invalid {
 background-color: #f44242;
 border-color: red;
 }
 </style>
 </head>
 <body>
 <form>
 <h1>Hello!</h1>
 <p>If the box turns green, the information is valid!</p>
 <label><span>first name</span>
 <input
 name="name"
 required
 pattern="[A-Z][a-z-]+"
 >
 </label>
 <label><span>age</span>
 <input
 type="number"
 name="age"
 required
 min="0"
 max="100"
 >
 </label>
 <label><span>email</span>
 <input
 type="email"
 name="email"
 required
 pattern="[\w-.]+@[\w-]{1,62}(\.[\w-]{1,62})*"
 >
 </label>
 <label><span>address</span>
 <input
 name="address"
 required
 >
 </label>
 <label><span>phone number</span>
 <input
 type="tel"
 name="phone"
 required
 pattern="\d{3}-\d{3}-\d{4}"
 >
 </label>
 <input type="submit">
 </form>
 </body>
</html>

Roman RomanRoman 2,90311 silver badges23 bronze badges · Accepted Answer · 2019-02-12 07:09:48Z

Duplicate If-Statements

You have 5 validations that look all the same. You could write a function to get ride of the duplication.

The function could look like:

function makeGreenIfValidationIsValid(tagId, regex) {
 if(input_tag.id == tagId) {
 if(regex.test(x.value)) {
 x.style.backgroundColor = "green";
 x.style.borderColor = "green";
 }
 }
}

After that, the onClick-calback would look like

button_tag.onclick = function() {
 var x = document.getElementById(input_tag.id);
 makeGreenIfValidationIsValid('input_0', /^[a-zA-Z]+$/)
 makeGreenIfValidationIsValid('input_1', /^[0-9]+$/)
 makeGreenIfValidationIsValid('input_2', /^\w+@[a-zA-Z_]+?\.[a-zA-Z]{2,3}$/)
 makeGreenIfValidationIsValid('input_3', /\d{1,5}\s\w{1,10}\s\w{1,10}/)
 makeGreenIfValidationIsValid('input_4', /^\d{3}-\d{3}-\d{4}$/)
 document.getElementById(fieldset_tag.id).appendChild(button_tag);
 this.count += 1;
}

Extract Class

The method constructForm in ContentDisplayer should be a own class. An indicator for that is that it is huge (more than 80 lines) and you have many tag-comments in it.

Comments are add all not bad but when you group your code in a method you all ready see semi-independent logic. In Robert C. Martins words: "Don’t Use a Comment When You Can Use a Function or a Variable"

For example, the class might be named "Form" and could contain several methods. Based on your comments I could look like

function Form() {
 //Create div containing the form
 this.createDivTag() {}
 //Create the form tag
 this.createFormTag() {}
 //Create the fieldset tag
 this.createFieldsetTag() {}
}

The logic in create[tag-name]Tag for creating a div, form and fieldset looks very similar. We should extract the common logic into a function.

Prototyping

Currently ContentDisplayer and Form (the class from above) don't use it.

A disadvantage is that on each creation of an object all methods like show will be recreated each time. The result is that it costs performance.

With prototyping it would look like

function ContentDisplayer() {
 this.count = 0;
}
ContentDisplayer.prototype.show = function(id) {/**/}
ContentDisplayer.prototype.showText = function(id, content) {/**/}
// ...

Compiled functions are cached. There is no performance gain by creating an external prototype. External prototypes do not allow the use of closure to create private members. Even way back when compilation was not cached external prototypes provided no benefit for single instance objects which is what is being used in this case. In modern JS external prototypes only provide a benefit when there are many instances (100+ to 1000+) with short lifetimes that are longer than a single GC cycle, with most of the small benifit in reduced GC overhead
makeGreenIfValidationIsValid(tagId, regex) - I like the name! It's exactly what I'd come up with because I suck at being concise, so I just go with "long, boring but descriptive". Still, I'd personally inverse the order of the parameters passed in. That way you can very easily partially apply the function and derive more descriptive functions like validateNumeric = makeGreenIfValidationIsValid.bind.(this, /^[0-9]+$/) and then call validateNumeric("input_1").
I'd agree with extracting the form creation into its own class. But I'd go a step further and make it a Builder. So you'd probably use it like formBuilder.addDiv(this.count).addForm(this.count).addLabel().addBreak().build(). Or something similar there is probably a better way to handle this, the point is how the interface is then exposed.
Finally, for Prototyping, I wouldn't say it's a disadvantage. The code works perfectly fine as it is. Using a prototype might help but the claim that "it costs performance" is very likely a premature optimisation. Even if it is slower, it might not matter, depending on the use case. And for a homework assignment I'd definitely not care about this aspect. I'd leave it as a preference/option but not something that's really "wrong" with the code as it is.

Stack Exchange Network

JavaScript DOM Implementation

2 Answers 2

Duplicate If-Statements

Extract Class

Prototyping

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

2 Answers 2

Duplicate If-Statements

Extract Class

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related