I am looking for some feedback on my code, it is not done but most of the functionality is already there however the code seems to be long and wanted to see how I could improve and shorten it.

I did mention the oc function before, but until I find out what you intend to use a for, which currently seems to be to empty properties of the object.

Ahh, looking further in to the rest of the code, that oc function is used only to work out if an array item already exists in an array, for which there is a built-in array.indexOf() method that should be used instead.

You do need to cater for web browsers such as Internet Explorer that haven't yet reached support for JavaScript 1.6, so the page also gives you compatibility code to help those browsers out.

So instead of the following code:

if (type in (oc(input_a))){

You can use this instead:

if (input_a.indexOf(type) > -1) {

Next though, about the variables. Are they global variables? They are fundamentally at risk of being clobbered by any other code on the page that also sets global variables (not just by your code, but by other code that you cannot trust). We can try and resolve that problem by putting them and the functions within a single formBuilder object, but that can occur later on once the functions are much more encapsulated.

Counters

The sel_check_opt_counter variable is not needed and can be easily removed. Instead of using the following while loop:

Similar work can be performed to remove other counters, such as the last_created one. That whole mess can be replaced with pushing and popping values from the array.

We can even get your special message in for if no changes have been made, by tracking whether it has been used or not.

last_created.push(form_preview);
...
if (last_created.length === 0) {
if (!last_created.used) {
alert('Sorry can´t undo, you have not done any changes.');
} else {
alert('Sorry can´t undo, use the red x on the field that you like to delete.');
} else {
document.getElementById(form_id).innerHTML = last_created.pop();
last_created.used = true;
}

There's a lot more to be done, but I'll hold off until there is some kind of testing harness, or even a test page where the script can be exercised to ensure that through the code changes that the behaviour remains consistent and as expected.

tlacaelelrl
—
2011-07-26T04:53:49Z —
#4

I am checking your post right now, but I read your first message and did some changes and this is how the code looks right now, and I will go through your new post and make more changes to it, thank you.

The first if works in IE the second one does not, both work in FF, you can view the error here it says the object does not accept the property or method

Paul_Wilkins
—
2011-07-26T11:31:28Z —
#8

tlacaelelrl said:

The first if works in IE the second one does not, both work in FF, you can view the error here it says the object does not accept the property or method

Did you also read about how it works on modern web browsers, but others like Internet Explorer require compatibility code to support that version 1.6 feature?

tlacaelelrl
—
2011-07-26T11:41:37Z —
#9

paul_wilkins said:

Did you also read about how it works on modern web browsers, but others like Internet Explorer require compatibility code to support that version 1.6 feature?

Oh, I guess I missed that part, it is working now and I removed the oc() function, thank you

Paul_Wilkins
—
2011-07-26T14:20:43Z —
#10

tlacaelelrl said:

Oh, I guess I missed that part, it is working now and I removed the oc() function, thank you

I passed the script code through jslint.com to help deal with most of the obvious issues.

After that, I found that the only indexOf that you were doing was in that one area. Since it's a list of input types that won't be varying, a more appropriate way to achieve the desired end result here is with a switch statement, which also allows you to get rid of the compatibility code.

Then I updated the form_create_input() function by moving the new field part out to a separate function, where a switch statement is used to determine what to do based on the type:

I was also wondering what the unshift() part was for, then when seeing where you splice an item out, realised that it's where you're adding and removing undo states from stack. That part has been tidied up too so that it uses push() and pop() which more people find to be a more understandable way to manage such stacks.

There's no need to retrieve the parent from the global array, since the parent is available from the parentNode property. What's worse though is that if the nodeToRemove is not a direct descendant of the parent from your array, that will result in a script-killing error occurring.

If the parent is obtained directly from the node that you want to remove, no such problems occur.

The returnValue part of the script has also been removed, because that's only for Internet Explorer and you already have a return false, which achieves the same result.

Next up is to deal to the horrible form_values[0] notation. Arrays are supposed to contain the same semantic items, but this breaks that idea. By making it an object with properties instead, the code becomes much easier to follow:

it does work, however assuming I have 3 fields, when I click the move up link in the top field twice it sends it at the bottom, and when I click the move down link on the bottom field it gives me a nextSibling null error, I tried catching the exemption with an if but I was not able to do it, how could I check if the nextSibling is null as this:

if(nextSibling != null)

did not work for me?

Paul_Wilkins
—
2011-07-27T09:14:25Z —
#17

tlacaelelrl said:

and when I click the move down link on the bottom field it gives me a nextSibling null error

That's odd, because the insertBefore() method is designed to allow a null value as the second parameter, in which case it moves the element to the end of the list.