patternjavascriptMinor
Form generator for inputting URL, description and link
Viewed 0 times
descriptiongeneratorinputtingforandformurllink
Problem
This is a form generator I created so users can input a URL, description and link. The output will eventually be a JavaScript news slider. The HTML the form generated is irrelevant for now but I'd like to see how I can improve the JavaScript code handing the processing.
This is my first version/draft.
```
How many slides do you want to generate?
function generateForm(){
//get num of slides required by user, limit to 8
var numOfSlides = document.getElementById('numOfSlides').value;
//make sure num of slides is a valid integer 1-8
if(isNaN(numOfSlides)){
alert("Well that doesn't make much sense. Please enter a positive integer 1-8");
//document.getElementById('sliderFormGenerated').innerHTML="Please enter a positive integer 1-8.";
}
else if (numOfSlides >= 9) {
alert("The limit is 8 slides, please enter 8 or less.");
//document.getElementById('sliderFormGenerated').innerHTML="Please enter a positive integer 1-8.";
}
else if(numOfSlides Please enter a positive integer 1-8.";
}
else {
//document.getElementById('sliderFormGenerated').innerHTML="Great! For simplicity, let's make one at a time";
//document.getElementById('sliderFormGenerated').innerHTML=sliderFormNumber;
//for loop to add unique id to each generated element
for(var i=0; i \"Read More\" URL: ";
$("#sliderFormGenerated").append("Slide " +(i+1) + "Image URL: " + i + "Description: " + i +"\"Read More\" URL: " +i + "");
$('#sliderFormGenera
This is my first version/draft.
body{
background-color:green;
}
.hideElement{
display:none;
}
.container{
display: inline-block;
margin:25px;
border: 2px solid white;
padding:10px;
}
p.slidenum{
text-align: center;
font-size: 22px;
font-weight: bolder;
}
p{
font-size: 20px;
}
input[type="text"]{
background-color:white;
color:black;
font-size:16px;
padding:10px;
}
input[type="button"]{
background-color:#A17F3F;
color:white;
font-size:16px;
border:2px solid;
border-radius:5px;
padding:10px;
}
```
How many slides do you want to generate?
function generateForm(){
//get num of slides required by user, limit to 8
var numOfSlides = document.getElementById('numOfSlides').value;
//make sure num of slides is a valid integer 1-8
if(isNaN(numOfSlides)){
alert("Well that doesn't make much sense. Please enter a positive integer 1-8");
//document.getElementById('sliderFormGenerated').innerHTML="Please enter a positive integer 1-8.";
}
else if (numOfSlides >= 9) {
alert("The limit is 8 slides, please enter 8 or less.");
//document.getElementById('sliderFormGenerated').innerHTML="Please enter a positive integer 1-8.";
}
else if(numOfSlides Please enter a positive integer 1-8.";
}
else {
//document.getElementById('sliderFormGenerated').innerHTML="Great! For simplicity, let's make one at a time";
//document.getElementById('sliderFormGenerated').innerHTML=sliderFormNumber;
//for loop to add unique id to each generated element
for(var i=0; i \"Read More\" URL: ";
$("#sliderFormGenerated").append("Slide " +(i+1) + "Image URL: " + i + "Description: " + i +"\"Read More\" URL: " +i + "");
$('#sliderFormGenera
Solution
I will start with a snooty remark; please don't submit your first draft, but submit the best code you think you can write.
From a once over:
-
-
This:
I am sure you could think up more creative names for those variable names
I have some more items, but at this point I think you need to polish your script and if you wish resubmit it, the quality of this code is very low.
From a once over:
- You should consider having your JavaScript in a separate file, JavaScript mixed in with your HTML is old skool
- Indentation in
function generateForm(){is off
- Do not use
alert, it annoys users, the commented out approach of settingdocument.getElementById('sliderFormGenerated').innerHTMLis superior
- Do not keep commented out code
-
"
\"Read More\" URL: "; -> I would prefer ' "Read More" URL: ';onClick="hideStep1(); clearForm(); toggleStep2(); toggleCodeOutput(); showGenCodeButton()"please useaddEventListenerinstead
- If you had to use
onclick, then you should at least have grouped these functions into 1 well named function, this way you would not have to create functions likeshowGenCodeButton()which is a short one liner that you only use once.
-
This:
var temp = document.getElementById('imgsrc' + i).value;
var temp1 = document.getElementById('description' + i).value;
var temp2 = document.getElementById('link' + i).value;I am sure you could think up more creative names for those variable names
I have some more items, but at this point I think you need to polish your script and if you wish resubmit it, the quality of this code is very low.
Code Snippets
' <p>"Read More" URL: </p><input type="text" id="link">';var temp = document.getElementById('imgsrc' + i).value;
var temp1 = document.getElementById('description' + i).value;
var temp2 = document.getElementById('link' + i).value;Context
StackExchange Code Review Q#46071, answer score: 3
Revisions (0)
No revisions yet.