What’s Wrong With This Code (#16)

Tuesday, July 3, 2007

It’s been some time since the last WWTC, but Estelle Hertz is still cranking out software. This time Estelle was asked to write some JavaScript code for a web page with hundreds of textbox input controls (the page collects details for a life insurance policy). When a user double clicks on any textbox on the page, the textbox should reset itself to its initial value (the value in the control when the page first loaded).

Estelle has yet to experience all the slippery pitfalls in JavaScript development, and writes the following code:

<%@ Page Language="C#" %>

<html xmlns="http://www.w3.org/1999/xhtml">
<
body>
    <form id="form1" runat="server">
        <div>
            <input id="Text1" value="default" type="text" />
            <%-- and so on for a 100 textbox controls --%>
        
</div>
    </form>
</
body>
</
html>

<
script type="text/javascript">

function
InputManager(input)
{
   
this._input = input;
   
this._initialValue = input.value;
   
this._input.ondblclick = this.resetToInitialValue;
}

InputManager.prototype.resetToInitialValue =
function()
{
    _input.value = _initialValue;
}

var inputs = document.getElementsByTagName("input");
for(var i = 0; i < inputs.length; i++)
{
    
if(inputs[i].type != "text")
        
continue;
        
    
var manager = new InputManager(inputs[i]);
}

</script>

Anyone spot a problem? Two problems? Three problems? Could there be possibly be more than three problems in the 20 line hunk of script?


Comments
laimis Tuesday, July 3, 2007
In the input prototype resetToInitialValue function, she should use 'this' to access the variables.

It also would be a good idea to register the javascript to run after either the window or the dom tree is loaded. However I think all the current browsers will run the script at the time when the controls will be in the DOM tree and available.
Jonas Tuesday, July 3, 2007
Hi,

The first problem is the memory leak that you are going to experience by saving a reference to the input DOM element in the JavaScript "member" this._input.

If you are going to keep references like this you need to tear them down on unload.

The rest is so complicated that I can't even understand it ;) What scope are being used ?

I would take advantage of event bubbling and expando properties to manage this instead.


Thanks
/Jonas
Rasmus Kromann-Larsen Tuesday, July 3, 2007
Atleast the

_input.value = _initialValue;

should be:

this._input.value = this._initialValue;
Filini Tuesday, July 3, 2007
I'm guessing, without testing the code in a real browser - that would be cheating :-p

1. I'm not so sure, since I haven't been experimenting with JS prototype, but I think that adding the eventhandler (resetToInitialValue) to the input before actually defining the eventhandler will not work, it will just add a null handler to the event.

2. Variable scope in JavaScript can be a bit "blurry" sometimes, but I think that since "manager" is declared inside the loop, then the object with _initialValue will be lost after the loop. How this will actually result at double-click time, only god knows...

3. Who on earth decided that double-clicking on a textbox, an action universally used to select a word inside the textbox, should actually RESET the value?!?!? ^^
Skup Tuesday, July 3, 2007
Hi,

there is at least the problem called 'this' is not what you expect...
when resetToInitialValue will be called, 'this' will be the input instance and not the InputManager instance...
You have to use a delegate (have a look at the Function.createDelegate method in AJAX for ASP.Net)

then, in javascript, this is never implied, so
'_input.value = _initialValue' will fail since thoses values are not defined. use 'this._input.value = this._initialValue' instead.

then, I'm not sure, but I would not let the manager instances flow in memory space without keeping a reference on it. When there is no reference left, the instance will be deleted.
But it shouldn't be the case when using a delegate.

Dejan Vesić Tuesday, July 3, 2007
It seems to me that it should pack InputManager instances to an array, not to single variable - above code will preserve only last instance of IM.
Brig Lamoreaux Tuesday, July 3, 2007
Why not just do this?

var inputs = document.getElementsByTagName("input");
for(var i = 0; i < inputs.length; i++)
{
if(inputs[i].type != "text")
continue;

inputs[i].initialValue = inputs[i].value;
inputs[i].ondblclick = function(){
this.value = this.initialValue;
};
}
Jeff Mayeur Tuesday, July 3, 2007
There seems to be several problems.

1) The var manager = new InputManager() isn't really necessary, as using a prototype is pretty, but not really what's needed, an anonymous function would work fine.
2) The prototype off of the Input manager has no reference to the IM itself, so the ondblclick event will have the input itself as "this" and therefore will not have the _initialValue or _input attributes/properties.
3) Being that the dblclick event has no reference to the IM, the _initialValue is not stored in an accessible location, and due to the reuse of the var manager, it is actual tossed immediately.
4) even if the var manager was an Object() array, the scope is wrong, it would need to be declared outside of the for loop

Using the prototype style you could:
function InputManager (input)
{
input._initialValue = input.value;
input.ondblclick = this.resetToInitialValue;
}

InputManager.prototype.resetToInitialValue = function(e)
{
this.value = this._initialValue ;
}

but that's probably too complicated for what's needed
Jonas Wednesday, July 11, 2007
Scott,

I enjoy your explanation of closures and scope.
It's powerful "stuff" ;)

I hope you will cover a "much easier" approach than "hooking up the 100 textbox controls" using closures and delegates.

You have to excuse my x-browser experience.

But adding ONE event handler on the container element &lt;div&gt; and use an expando property initialValue="xyz" is much easier and doesn't consume that much memory.

&lt;div ondblclick="ResetStuff( evt )"&gt;
&lt;input id="Text1" value="default" type="text" initialValue="xyz"/&gt;
&lt;%-- and so on for a 100 textbox controls --%&gt;
&lt;/div&gt;

&lt;script language="JavaScript"&gt;
function ResetStuff( evt )
{
var target == evt ? evt.target : event.srcElement; // Or some valid X-browser

var initialValue = target.getAttribute("initialValue");
if( initialValue == null )
{
return;
}
target.setAttribute("value, initialValue);
}
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!