HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Form generator for inputting URL, description and link

Submitted by: @import:stackexchange-codereview··
0
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.


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:

  • 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 setting document.getElementById('sliderFormGenerated').innerHTML is superior



  • Do not keep commented out code



-
"
\"Read More\" URL: ";
-> I would prefer

' "Read More" URL: ';


  • onClick="hideStep1(); clearForm(); toggleStep2(); toggleCodeOutput(); showGenCodeButton()" please use addEventListener instead



  • 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 like showGenCodeButton() 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.