Javascript verbessern

[jsbetter]

Hey.

Ich habe diese Javaascript-Funktion hier gebastelt. Ich bin aber nicht gerade der beste in JS und wollte euch fragen, wie man diese besser gestalten könnte, schneller, cross-browserkompatibler, sicherer.

  
/* Update Auction Status*/  
function uAs(){  
		var r=getReq(),m=0,n,c,x,p,i,j,num;  
		r.open('GET','http://example.com/index.php?c=Auction&a=getStats&x='+aid,true);r.onreadystatechange=function(){  
			if(4===r.readyState&&200===r.status){  
				//alert(r.responseText);  
					var x=r.responseText.split('|');  
					$('#dC').countdown('change',{until: new Date(x[0])});  
					var m=x[1]*0.01*1;  
					var num=new Number(m);  
					var price=num.toFixed(2);  
					// Auctionsprice  
					document.getElementById('auctionprice').innerHTML=price;document.getElementById('auctionprice2').innerHTML=price;  
					// Saving Dollar  
					var p=document.getElementById('retailprice').innerHTML;  
					var i=p-price;  
					var pp=new Number(i);  
					document.getElementById('savingsdol').innerHTML=pp.toFixed(2);  
					// Saving Percent  
					var i=100-(price/(p/100));  
					var pp=new Number(i);  
					document.getElementById('savingsper').innerHTML=pp.toFixed(2);  
					// Bid Rebate  
					var i=0.01*x[4]*1;  
					var num=new Number(i);  
					document.getElementById('bidrebate').innerHTML=num.toFixed(2);  
					// Buy now  
					var j=1*(55.35-i);  
					var num=new Number(j);  
					var p=j.toFixed(2);  
					document.getElementById('newprice').value=p;  
					document.getElementById('purchaseprice').innerHTML=p;  
					// Your Bids  
					document.getElementById('yourbids').innerHTML=x[6];  
					// lastbidders  
					if(x[2]){  
						document.getElementById('currentWinner').innerHTML=x[3];  
						var c=document.getElementById('lastbidders');  
						c.innerHTML=x[2];  
					}  
                                        // End of Auction  
					if('0'!=x[5]&&false!=x[5]&&''!=x[5]){  
						if(x[5]==0){  
							setTimeout("location.href='http://example.com/Auction/Winner/'+aid",2000);  
						}else{  
							document.getElementById('dC').innerHTML=x[3];  
							window.clearInterval(aS);  
						}  
					}  
			}  
		}  
		r.send(null);  
}  
  

Liebe Grüße,
JSBETTERER

[Struppi]

Ich habe diese Javaascript-Funktion hier gebastelt. Ich bin aber nicht gerade der beste in JS und wollte euch fragen, wie man diese besser gestalten könnte, schneller, cross-browserkompatibler, sicherer.

»

Du erwartest doch nicht wirklich, dass jemand ein Skript das dermaßen schlecht dokumentiert ist und solche Bezeichner benutzt, für dich analysiert?

function uAs(){
var r=getReq(),m=0,n,c,x,p,i,j,num;

Besser könntest du es machen, wenn du nicht einen Obfuscator über den Code laufen lassen würdest.

Struppi.

[jsbetter]

Besser könntest du es machen, wenn du nicht einen Obfuscator über den Code laufen lassen würdest.

Mache ich doch gar nicht oder? Also das da oben habe ich gemacht.. hab mal gelesen es sei schneller jede Variabele vorher anzumelden

[molily]

Besser könntest du es machen, wenn du nicht einen Obfuscator über den Code laufen lassen würdest.

Mache ich doch gar nicht oder? Also das da oben habe ich gemacht..

Struppi hat von den Variablennamen geredet - diese sind unverständlich, weil sehr kurz und nichtssagend (m, n, c, x, p ...).

hab mal gelesen es sei schneller jede Variabele vorher anzumelden

Es ist zwar guter Stil, Variablen am Funktionsanfang zu deklarieren. Auf die Geschwindigkeit der Codekompilierung und -ausführung hat das jedoch keine Auswirkung. Wenn der Interpreter in einen Funktionskontext springt, werden ohnehin alle Variablendeklarationen zuerst abgearbeitet, egal an welcher Stelle sie in der Funktion stehen. Das nennt sich »Hoisting« und betrifft auch Funktionsdeklarationen.

Mathias

[LX]

Hallo, JSBETTERER!

Zuallererst: als Datenformat verwendest Du hier Dash-separierten Text, was insbesondere dann zum Problem werden kann, wenn eine der Daten selbst ein | enthalten könnte. Weiterhin schreibst Du eben diesen Text teilweise ohne weitere Prüfung in das innerHTML Deiner Seite, was wiederum ein Einfallstor für Cross-Site-Scripting darstellen könnte.

Es wäre zur Steigerng der Sicherheit empfehlenswert, 1. JSON als Format zu wählen (korrekter Parser vorausgesetzt) und 2. die Werte, die eingefügt werden sollen, mittels einer RegExp zu validieren.

Als nächstes steht beim Befüllen von auctionprice2 ein leeres var ohne Variable oder Deklaration, was normalerweise zu einem Fehler führen sollte.

Dann hast Du hier einen ziemlichen Spaghetticode (ohne Carbonara), der ziemlich wiederkehrende Aktionen (das Befüllen eines Nodes mit funktional aus den GET-Daten errechneten Informationen) untereinander schreibt. Das kann man durch eine Schleife über ein Objekt eleganter (und auch flexibler) handhaben:

// gather data in object "data"
var fields = {
   'auctionprice': function() { return (data.price || '?').replace(/<.*?>/,''); },
   'auctionprice2': ...
}, field;
for (field in fields) {
   if (!fields.hasOwnProperty(field)) { continue; }
   (document.getElementById(field) || {}).innerHTML = fieldsfield;
}

Zuletzt fällt mir auf, dass Du offenbar sehr viele Dinge als gegeben annimmst, was die Robustheit Deines Codes deutlich reduziert. Was, wenn ein Feld gelöscht wurde? Was, wenn ein Interval nicht gesetzt ist? Was, wenn das DOM noch nicht komplett befüllt ist oder getReq() keinen XMLHttpRequest zurückliefert?

Gruß, LX

[jsbetterer]

Hallo, JSBETTERER!

Zuallererst: als Datenformat verwendest Du hier Dash-separierten Text, was insbesondere dann zum Problem werden kann, wenn eine der Daten selbst ein | enthalten könnte. Weiterhin schreibst Du eben diesen Text teilweise ohne weitere Prüfung in das innerHTML Deiner Seite, was wiederum ein Einfallstor für Cross-Site-Scripting darstellen könnte.

Hm okay.. Ich weiß aber 100%ig welche Daten kommen. Also ein | kann nicht kommen, jedenfalls nicht aus dem Ajax-Request.

Es wäre zur Steigerng der Sicherheit empfehlenswert, 1. JSON als Format zu wählen (korrekter Parser vorausgesetzt) und 2. die Werte, die eingefügt werden sollen, mittels einer RegExp zu validieren.

Ich soltle vllt. erwähnen das dieses Skript alle 300ms aufgerufen wird vom Clienten.

Als nächstes steht beim Befüllen von auctionprice2 ein leeres var ohne Variable oder Deklaration, was normalerweise zu einem Fehler führen sollte.

Wieso ein leeres var? Da steht doch "price" welches so ziemlich direkt davor deklariert wird.

Dann hast Du hier einen ziemlichen Spaghetticode (ohne Carbonara), der ziemlich wiederkehrende Aktionen (das Befüllen eines Nodes mit funktional aus den GET-Daten errechneten Informationen) untereinander schreibt. Das kann man durch eine Schleife über ein Objekt eleganter (und auch flexibler) handhaben:

Hmm.. den folgenden Conde verstehe ich leider nicht ganz.
Was wird gemacht?
Die Nodes sind alle an verschiedenen Stellen auf der Seite.

// gather data in object "data"
var fields = {
   'auctionprice': function() { return (data.price || '?').replace(/<.*?>/,''); },
   'auctionprice2': ...
}, field;
for (field in fields) {
   if (!fields.hasOwnProperty(field)) { continue; }
   (document.getElementById(field) || {}).innerHTML = fieldsfield;
}

Ist das schneller? Sicherer?

Zuletzt fällt mir auf, dass Du offenbar sehr viele Dinge als gegeben annimmst, was die Robustheit Deines Codes deutlich reduziert. Was, wenn ein Feld gelöscht wurde?

Meinst du wenn ein Node nicht mehr da Ist?

Was, wenn ein Interval nicht gesetzt ist? Was, wenn das DOM noch nicht komplett befüllt ist oder getReq() keinen XMLHttpRequest zurückliefert?

Stimmt das sollte ich vllt. mal angeben. Es soll einfach nichts passieren.
Danke, Lg,
jsbetterer

[LX]

Hallo, JSBETTERER!

Hm okay.. Ich weiß aber 100%ig welche Daten kommen. Also ein | kann nicht kommen, jedenfalls nicht aus dem Ajax-Request.

Dieses 100%ige Wissen basiert auf der unbewußten Unterstellung, dass 1. die Antwort immer von Deinem Server kommt, 2. dieser nicht gehackt werden kann, 3. auch ein Man-in-the-middle-Angriff nicht möglich ist und 4. dem Client auch nicht beispielsweise durch Cross-Site-Scripting andere Inhalte untergeschoben werden können.

Du musst natürlich nicht JSON verwenden, es hätte nur den Vorteil, dass es inzwischen bei allen modernen Browsern standardmäßig einen eingebauten Parser gibt, der enorm schnell ist - fast so schnell wie "split" - mit dem zusätzlichen Vorteil, dass Dein Code lesbarer wird.

Ich soltle vllt. erwähnen das dieses Skript alle 300ms aufgerufen wird vom Clienten.

Das bedeutet, dass Du ca. 3,25 Mal pro Sekunde das Script aufrufst. Da Dir offenbar nicht klar ist, dass ein asynchroner Request mehr als 3s dauern kann, kommt also noch das Problem hinzu, dass Du Timeout-Probleme nicht berücksichtigt hast: wenn auf diese Weise immer mehr Requests auflaufen, wird die Seite irgendwann nicht mehr funktionieren.

Einerseits solltest Du überlegen, ob Du wirklich diese Netzlast generieren und Deinen Server damit belasten möchtest. Andererseits solltest Du die möglichen Timeout-Probleme, die sich daraus ergeben, unbedingt abfangen, d.h. wenn ein Request offen ist, sollte kein weiterer geöffnet werden oder aber der vorherige Request abgebrochen werden.

// gather data in object "data"
var fields = {
   'auctionprice': function() { return (data.price || '?').replace(/<.*?>/,''); },
   'auctionprice2': ...
}, field;
for (field in fields) {
   if (!fields.hasOwnProperty(field)) { continue; }
   (document.getElementById(field) || {}).innerHTML = fieldsfield;
}

Ist das schneller? Sicherer?

Nicht schneller, aber übersichtlicher und schneller zu ergänzen, falls Du Deinen Code einmal erweitern möchtest. Was hier passiert, ist: die Funktionen, die den Wert zurückgeben, der in einem Node stehen sollen, werden in ein Objekt gepackt, dabei hat jede Funktion den Namen der ID des Felds. Dann wird über dieses Objekt iteriert (also einmal alle Eigenschaften des Objekts durchgegangen) und das jeweilige Feld entsprechend befüllt.

Was, wenn ein Feld gelöscht wurde?

Meinst du wenn ein Node nicht mehr da Ist?

Genau. Dann führt der Versuch, in document.getElementById('name').innerHTML zu schreiben, zwangsläufig zu einem Fehler - der sich durch eine Abfrage oder noch einfacher dem folgenden Konstrukt abfangen läßt:

(document.getElementById(feldname) || {}).innerHTML = ...

Wenn das Feld nicht gefunden werden kann, landen die Daten schadlos in einem anonymen Objekt und es wird kein Fehler geworfen. Alternativ kannst Du natürlich auch ein benanntes Objekt nehmen und nachträglich prüfen, ob die innerHTML-Eigenschaft befüllt wurde und dann den Fehler selbst behandeln.

Gruß, LX

[jsbetterer]

Dieses 100%ige Wissen basiert auf der unbewußten Unterstellung, dass 1. die Antwort immer von Deinem Server kommt, 2. dieser nicht gehackt werden kann, 3. auch ein Man-in-the-middle-Angriff nicht möglich ist und 4. dem Client auch nicht beispielsweise durch Cross-Site-Scripting andere Inhalte untergeschoben werden können.

Du hast Recht, da sollte ich mir Gedanken drüber machen.

Das bedeutet, dass Du ca. 3,25 Mal pro Sekunde das Script aufrufst. Da Dir offenbar nicht klar ist, dass ein asynchroner Request mehr als 3s dauern kann, kommt also noch das Problem hinzu, dass Du Timeout-Probleme nicht berücksichtigt hast: wenn auf diese Weise immer mehr Requests auflaufen, wird die Seite irgendwann nicht mehr funktionieren.

Habe es jetzt auf alle 500ms geändert. Das macht die Konkurrenz genauso.
Ne viel bessere Lösung wäre ein Comet-Server Long Polling. Allerdings kann ich das nicht "mal eben" umsetzen und ich muss vorerst das gegebene möglichst sicher gestalten um mir Zeit zu verschaffen das System zu wechseln.

Einerseits solltest Du überlegen, ob Du wirklich diese Netzlast generieren und Deinen Server damit belasten möchtest. Andererseits solltest Du die möglichen Timeout-Probleme, die sich daraus ergeben, unbedingt abfangen, d.h. wenn ein Request offen ist, sollte kein weiterer geöffnet werden oder aber der vorherige Request abgebrochen werden.

Wie kann ich ein Timeout abfangen?

Was, wenn ein Feld gelöscht wurde?

Meinst du wenn ein Node nicht mehr da Ist?

Genau.

Wie kann das bitte passieren?

Lg, Jsbetterer

[jsbetterer]

Das bedeutet, dass Du ca. 3,25 Mal pro Sekunde das Script aufrufst. Da Dir offenbar nicht klar ist, dass ein asynchroner Request mehr als 3s dauern kann, kommt also noch das Problem hinzu, dass Du Timeout-Probleme nicht berücksichtigt hast: wenn auf diese Weise immer mehr Requests auflaufen, wird die Seite irgendwann nicht mehr funktionieren.

Ist das hier eine gute Methode?
http://www.coder-wiki.de/HowTos/Ajax-Timeout-pruefen

Lg, JsB

[LX]

Du kannst mit setTimeout einen Timeout setzen. Wenn der Request erfolgreich war, wird der Timeout mit clearTimeout gestoppt, bevor er ausgeführt werden kann. Wenn der Timeout doch ausgeführt wurde, kann darin der Request gestoppt werden.

Gruß, LX

[ChrisB]

Hi,

Ist das hier eine gute Methode?
http://www.coder-wiki.de/HowTos/Ajax-Timeout-pruefen

Was das angeht, ist der IE anderen Browsern voraus - der kennt nämlich für das XMLHttpRequest-Objekt explizit eine timeout-Eigenschaft, und mit ontimeout auch einen Eventhandler, um darauf zu reagieren.

Für Browser, die das (noch) nicht kennen, muss man es halt emulieren.
Die Lösung im genannten Artikel sieht halbwegs tauglich aus - Schwächen hat sie in so fern, dass der timer einer globale Variable ist, und somit das ganze immer nur für einen Request funktionieren wird, nicht aber bei mehreren parallel gestarteten.

MfG ChrisB

[Encoder]

var m=x[1]*0.01*1;
var num=new Number(m);
var price=num.toFixed(2);

Warum "mal eins"?
Warum schreibst du statt dieser 3 Zeilen nicht einfach.
var num=new Number(x[1] * 0.01);
var price=num.toFixed(2);
oder gleich
var price=new Number(x[1] * 0.01).toFixed(2);
Das verwirrt alles und du kapierst in einer Woche selber nicht mehr was und warum du getan hast.
Und falls jemand den Code so sieht (was bei JS der Fall sein dürfte), wirst du dir damit weder Ruhm noch Ehre einholen ;-)

Noch besser:
var num=new Number(x[1] * 0.01);
document.getElementById('auctionprice').innerHTML=num.toFixed(2);document.getElementById('auctionprice2').innerHTML=num.toFixed(2);
Damit hättest du dann um einiges lesbarer was du ausdrücken willst.

[jsbetter]

Warum "mal eins"?

Soltle ich dokumentieren.. das ist die Wechselrate. Da von Dollar ausgegangen wird un ich den Quelltext kopiert habe aus der Einstellung "US", ist *1.

Warum schreibst du statt dieser 3 Zeilen nicht einfach.
var num=new Number(x[1] * 0.01);
var price=num.toFixed(2);
oder gleich
var price=new Number(x[1] * 0.01).toFixed(2);
Das verwirrt alles und du kapierst in einer Woche selber nicht mehr was und warum du getan hast.

Ist das schneller?

Danke.

[Encoder]

Ist das schneller?

"das" ist nicht (bzw. nicht bedeutend) schneller. Aber "du" bist schneller, denn

• du schreibst in Zukunft weniger Code
• du schreibst deswegen weniger Fehler
• du kapierst deinen Code schneller wieder, wenn du ihn irgendwann mal anschauen musst.
  Guter Code ist nicht nur auf maximale Schnelligkeit ausgelegt, sondern auch auf Wartbarkeit. Vor allem wenn es nur um wenige CPU-Ticks geht und es keine zeitkritische Anwendung ist, wie es bei hoher Benutzeraktion der Fall ist.

[jsbetterer]

Guter Code ist nicht nur auf maximale Schnelligkeit ausgelegt, sondern auch auf Wartbarkeit. Vor allem wenn es nur um wenige CPU-Ticks geht und es keine zeitkritische Anwendung ist, wie es bei hoher Benutzeraktion der Fall ist.

Das Skript wird alle 300ms vom User ausgeführt...

[molily]

Noch besser:
var num=new Number(x[1] * 0.01);

Das reicht völlig:
var num = x[1] * 0.01;

Der *-Operator wandelt alle Operanden in Number um und das Ergebnis dieses Ausdrucks ist immer ein Number-Wert (dazu gehört auch NaN). Von »new Number« sollte man sich immer fern halten, weil das ein Number-Objekt anstelle eines Primitives erzeugt.

Mathias

[jsbetterer]

Der *-Operator wandelt alle Operanden in Number um und das Ergebnis dieses Ausdrucks ist immer ein Number-Wert (dazu gehört auch NaN). Von »new Number« sollte man sich immer fern halten, weil das ein Number-Objekt anstelle eines Primitives erzeugt.

Das Number-Object gibt mir aber die Möglichkeit es auf 2 stellen gerundet auszugeben. Mit Math.round(FOO*100)/100 gibt es manchmal leider Probleme wie ich feststellen musste.

Liebe Grüße

[ChrisB]

Hi,

Von »new Number« sollte man sich immer fern halten, weil das ein Number-Objekt anstelle eines Primitives erzeugt.

Das Number-Object gibt mir aber die Möglichkeit es auf 2 stellen gerundet auszugeben.

Die hast du ohne auch - auch ein Primitive, der eine Zahl enthält, hat die Methoden des Number-Objektes zur Verfügung.

alert((4710 * .01).toFixed(2))

MfG ChrisB

[molily]

Die hast du ohne auch - auch ein Primitive, der eine Zahl enthält, hat die Methoden des Number-Objektes zur Verfügung.

Jein, ein Primitive wird automatisch zu einem Number-Objekt gecastet, wenn man ihn mit einem Property Accessor verwendet:
http://ecma262-5.com/ELS5_HTML.htm#Section_11.2.1
http://ecma262-5.com/ELS5_HTML.htm#Section_8.7.1 GetValue
http://ecma262-5.com/ELS5_HTML.htm#Section_9.9 ToObject
http://ecma262-5.com/ELS5_HTML.htm#Section_15.7.2.1 new Number

new Number wird also implizit aufgerufen - wahrscheinlich nicht wirklich, sondern von der Logik her. Intern ist es wahrscheinlich simpler umgesetzt, sodass 1.234.toFixed(2) vermutlich schneller ist als das explizite new Number(1.234).toFixed().

Letztlich kommt's natürlich auf dasselbe heraus.

Mathias