JSLint: Consider Closures

[Kim]

Moin,

meine Frage richtet sich an die, die JSLint kennen bzw. gern streng sauberes Javascript schreiben :)

Ich schreibe gerade an einer Webanwendung, bei der Fenster über eine Javascript quasi instanziiert werden können. An einem dieser Fenstertypen kann eine Liste angezeigt werden, die ich mittels der Mootools Sortables per Drag and Drop sortierbar gemacht habe.

Nun will ich das Drag and Drop optisch aufpeppen und will dazu jedem Listenelement ein verhalten bei onmousedown usw. verpassen. Dazu gibt es die folgende, "private Methode" in dieser Klasse:

  
  var initColPanelUl = function()  
  {  
   for (var i = 0; i < colPanelUl.childNodes.length; i++)  
   {  
     // Recognize the following if-case. We have to distinguish  
     // between possible white-space and the li-elements of the list.  
     var li = colPanelUl.childNodes[i];  
     if (li.nodeName.toLowerCase() == "li")  
     {  
       event.on(li.id, "mousemove", function(ev) { interactFilterPanelEv(this.id, "mousemove"); });  
       event.on(li.id, "mouseout", function(ev) { interactFilterPanelEv(this.id, "mouseout"); });  
       event.on(li.id, "mousedown", function(ev) { interactFilterPanelEv(this.id, "mousedown"); });  
       event.on(li.id, "mouseup", function(ev) { interactFilterPanelEv(this.id, "mouseup"); });  
     }  
   }  
  };  

Hinweis: event ist ein Attribut der Klasse, das YUI.util.Event entspricht.

Nun meckert JSLint viermal wie folgt, was ich ja auch nachvollziehen kann, aber mir fehlt gerade die Idee zu einer einfacheren, sauberen Lösung:

Be careful when making functions within a loop. Consider putting the function in a closure.

Die Menge an möglichen Listeneinträgen ist leider keine feste Menge, sonst könnte man ja einfach auflisten.

Vorschläge? :)

[molily]

Hallo,

Was Crockfords Tipp bedeutet, verstehe ich auch nicht ganz. Deine Funktionen sind bereits Closures. Closures in Schleifen werden oft missverstanden, aber du nutzt gar nicht den möglichen Zugriff auf die eingeschlossenen Variablen.

Deine Lösung ist nicht schlecht, höchstens unperformant und kann verbessert werden. Du legst in der Schleife immer wieder neue, aber gleiche Funktionsobjekte an, die den Speicher vollballern. Alle schließen den Scope von »initColPanelUl« ein und der Funktion, in der diese steckt (deiner Beschreibung nach vermutlich eine Konstruktorfunktion). Das sorgt auch dafür, dass der Speicher schwer vom Garbage-Collector aufgeräumt werden kann.

Das ließe sich zumindest so verbessern:

var handler = function (ev) {  
   interactFilterPanelEv(this.id, ev.type);  
};  
for (...)  
{  
   event.on(li.id, "mousemove", handler);  
   event.on(li.id, "mouseout", handler);  
   event.on(li.id, "mousedown", handler);  
   event.on(li.id, "mouseup", handler);  
}

Handler einmal außerhalb der Schleife anlegen und für alle Event-Typen anpassen. Der fragt dann einfach den Event-Typ über Eventobjekt.type ab.

Mathias

[Kim]

Hallo Mathias,

herzlichen Dank, mit der Verzweigung meckert JSLint nicht mehr. Aber im Grunde veräppeln wir ja nur den JSLint Validator damit :-) Aber soll mir jetzt gerade Recht sein.

Die Liste beinhaltet übrigens so rund 70 +/- 10 Einträge. Sollte also nicht so gefährlich sein.

Besten Dank für die professionelle Antwort!

[KurtZ]

Kurtz gegrüßt

herzlichen Dank, mit der Verzweigung meckert JSLint nicht mehr. Aber im Grunde veräppeln wir ja nur den JSLint Validator damit :-) Aber soll mir jetzt gerade Recht sein.

Wenn du stärker in Crockfords Gedankenwelt einsteigen möchtest, er hat gerade bei O'Reilly ein JS-Buch rausgebracht, inklusive JS-Lint Kapitel.

Ich finds sehr dünn und komprimiert, aber für ne Abteilung lohnen sich 30 € schon.

Er bietet auch diverse Muster an, um Closurefunktionen einzusetzen.

Grüße
 Kurt

[KurtZ]

Kurtz gegrüßt

JavaScript: The Good Parts

Grüße
 Kurt

[Struppi]

var initColPanelUl = function()
  {
   for (var i = 0; i < colPanelUl.childNodes.length; i++)
   {
     // Recognize the following if-case. We have to distinguish
     // between possible white-space and the li-elements of the list.
     var li = colPanelUl.childNodes[i];
     if (li.nodeName.toLowerCase() == "li")

Wieso nutzt du hier nicht colPanelUl.getElementsByTagName('li')? Dann sparst du dir diese abfrage.

{
       event.on(li.id, "mousemove", function(ev) { interactFilterPanelEv(this.id, "mousemove"); });

Bist du sicher, dass du hier die id übergeben musst? Warum nicht direkt das Objekt? Die id ist in den seltesten Fällen interessant, das Objekt aber fast immer (zumal du in der Funktion den umgekehrten weg gehen kannst).

Nun meckert JSLint viermal wie folgt, was ich ja auch nachvollziehen kann, aber mir fehlt gerade die Idee zu einer einfacheren, sauberen Lösung:

Be careful when making functions within a loop. Consider putting the function in a closure.

Meckern halte ich hier für das falsche Wort, es ist lediglich eine Warnung, die du in deinem Fall nicht beachten braucht.

Struppi.

[Kim]

JSLint gibt das als "Error" aus. Damit würde unser Build-Prozeß fehlschlagen. :-) Das mit der ID ist ein guter Hinweis. Frag mich gerade selbst, warum ich nicht gleich das Objekt übergeben habe. Muss ein Artefakt sein :-)

[Christian Seiler]

Hallo!

JSLint gibt das als "Error" aus. Damit würde unser Build-Prozeß fehlschlagen. :-)

Dann ist euer Build-Prozess aber blödsinnig. ;-) JSLint ist ein nützliches Werkzeug, um Probleme in JS zu finden. Allerdings findet der bei weitem nicht alle und sehr oft meckert er auch Dinge an, die vollkommen in Ordnung sind (zudem hat Crockford seinen eigenen Programmierstil stark einfließen lassen, dem man ja nicht unbedingt folgen muss). JSLint ist daher ein nützliches Tool, um als Entwickler nach Problemen zu suchen (wenn man weiß, was man tut), aber es als Metrik zu verwenden, ist nicht sinnvoll. Im Gegensatz zu einem Compilerfehler sind Fehler, die JSLint liefert, nicht immer ein Problem.

Viele Grüße,
Christian

[Kai345]

[latex]Mae  govannen![/latex]

Dann ist euer Build-Prozess aber blödsinnig. ;-) JSLint ist ein nützliches Werkzeug, um Probleme in JS zu finden. Allerdings findet der bei weitem nicht alle und sehr oft meckert er auch Dinge an, die vollkommen in Ordnung sind (zudem hat Crockford seinen eigenen Programmierstil stark einfließen lassen, dem man ja nicht unbedingt folgen muss). JSLint ist daher ein nützliches Tool, um als Entwickler nach Problemen zu suchen (wenn man weiß, was man tut), aber es als Metrik zu verwenden, ist nicht sinnvoll. Im Gegensatz zu einem Compilerfehler sind Fehler, die JSLint liefert, nicht immer ein Problem.

Stimmt schon. Leider bricht JSLint bei einer bestimmten Anzahl "Fehlern" den Überprüfungsvorgang komplett ab, wodurch es ggf. bei einem etwas eigenen Programmierstil vollkommen unbrauchbar wird, obwohl man z.B. nur durchgängig eine bestimmte Sache anders schreibt als JSLint dies wünscht. Solange JSLint diese Stil-Eigenheiten als Fehler betrachtet, ist es leider als Überprüfungstool nur sehr eingeschränkt verwendbar, wenn man es sich nicht gegebenenfalls selber entsprechend anpass(t|enkann). Glücklicherweise wurden inzwischen einige Dinge einstellbar gemacht, aber immer noch nicht ausreichend. Meiner Meinung nach müßte JSLint einen Schalter haben, der alle Stilüberprüfungen abschaltbar macht. Na ja, mit diesen Einschränkungen ist es natürlich ein ganz hervorragendes Tool, um z.B. Flüchtigkeitsfehler zu finden und auch -je nach gesetzten Optionen- einen halbwegs sauberen Codestil einzuhalten.

Cü,

Kai

[Kai345]

[latex]Mae  govannen![/latex]

Nun meckert JSLint viermal wie folgt, was ich ja auch nachvollziehen kann, aber mir fehlt gerade die Idee zu einer einfacheren, sauberen Lösung:

Be careful when making functions within a loop. Consider putting the function in a closure.

Die Menge an möglichen Listeneinträgen ist leider keine feste Menge, sonst könnte man ja einfach auflisten.

Vorschläge? :)

JSLint erwartet es wie folgt:
http://tech.groups.yahoo.com/group/jslint_com/message/35

Ob es wirklich sinnvoll ist weiß ich nicht (bin kein JS-Guru), seinen Grund für diese Änderung hat DC aber auch im übernächsten Beitrag des Threads genannt.

Cü,

Kai

[Struppi]

JSLint erwartet es wie folgt:
http://tech.groups.yahoo.com/group/jslint_com/message/35

Ob es wirklich sinnvoll ist weiß ich nicht (bin kein JS-Guru), seinen Grund für diese Änderung hat DC aber auch im übernächsten Beitrag des Threads genannt.

Das ist sinnvoll, wenn in der Funktion die Schleifenvariabel verwendet wird, was hier aber nicht der Fall war, aber Mathias hat ja einen Vorschlag gemacht, der das Tool zufrieden stellt.

Struppi.