views:

127

answers:

3

I have the following code snippet which uses'event' My fellow developers argue that the scope of 'var event' is restricted to 'if' condition. Is that true. How can I make this a better code

function prepForDrag(obj, event) {      
    if(event= "undefined"){  
        var event=obj || window.event;
    }
    if (event.altKey) {
        showShiftEditable(objCurrentEditRow, nCurrentEditableShift, lCurrentEditableBreak, true);    
        var thisForm = eval('document.${formName}');
        // ...
        enableDragState(obj);
        disableClickEditHandler(obj);  ## remove 'normal' line sched click handling in dd mode
    }
  }
+2  A: 

JavaScript does not have block scope (except the exception variable inside a catch block), so in your case, that event variable has function scope. The best you can do, is to reassign event with the new value, or maybe use another variable name.

Ionuț G. Stan
+10  A: 

That's not true. In JavaScript there is no block scope, only function scope*. All variables introduced in a function are hoisted up to the top of the function.

So this code:

function prepForDrag(obj, event) {
    if (event = "undefined") {
        var event = obj || window.event;
    }
    // ...
}

gets interpreted somewhat like this:

function prepForDrag(obj, event) {
    var event;
    if (event = "undefined") {
        event = obj || window.event;
    }
    // ...
}

As Marcel Korpel's pointed out, declaring variable event is unnecessary in this case because event is already a local variable since it's a function parameter.

function prepForDrag(obj, event) {
    if (event = "undefined") {
        event = obj || window.event;
    }
    // ...
}

For futher details, read Ben Cherry's article on JavaScript Scoping and Hoisting.

Nevertheless there are two additional problems in your code.

  1. In the condition you used the = assignment operator instead of the == comparision operator. So the condition always evaluates to true.

  2. If you want to check whether a function argument was given, use the typeof event == 'undefined' statement.

And I'm afraid there is even one more issue here. What is the purpose of the condition? Does argument obj anything to do with event? Modern browsers pass an event object to the event handler function as argument but some do not. To avoid the problem, the following pattern tends to be used:

function prepForDrag(e) {
    var event = e || window.event;
    // ...
}

*NB: there is a let statement introduced in JavaScript 1.7 that provides block scope inside functions. Currently it's only supported in Firefox.

Török Gábor
This is still not completely correct: as `event` is already a parameter to `prepForDrag`, it already has the local scope and declaring the variable using `var` not necessary. You last example of normalizing the Event interface is incorrect: the Event interface is passed as the *first* argument to an event handler, so in this case `obj` would hold the Event interface, `e` never does.
Marcel Korpel
@Marcel Korpel: you're absolutely right. Thanks for pointing them out.
Török Gábor
+6  A: 

As event is already a parameter to prepForDrag, its scope is local to the function.

But your if condition is wrong:

if(event= "undefined")

This assigns "undefined" to event and evaluates to true. You probably should use

if (typeof event == "undefined")

or (as I think what you want is)

function prepForDrag(event) {
    event = event || window.event;
    if (event.altKey) {
      showShiftEditable(objCurrentEditRow, nCurrentEditableShift, lCurrentEditableBreak, true);    
      var thisForm = eval('document.${formName}');
      ................................
      enableDragState(obj);
      disableClickEditHandler(obj);  // remove 'normal' line sched click handling in dd mode
    }
}

BTW, why are you evaling document.${formName}?

Marcel Korpel
@Korpel : This is very old (5yrs) code.never touched/remodelled unless faced issues. This one is FF compatability issue. So eval ing is basically reading form data which of course poor javsripting :-(
GustlyWind