nicht angemeldet
Hallo,
Zur Struktur will ich jetzt mal nicht viel sagen, auf den ersten Bilck scheint sie mir gar nicht übel. Man könnte zunächst noch einiges am Code zu vereinfachen, anschliessend hat man dann einen noch besseren Überblick über die Stuktur, die dann man evtl. auch noch klarer bauen kann. Molily hat wohl schon das wichtigste zur Struktur gesagt.
Die Kapselung als anonyme Funktion ist i.O., muss dann aber auch konsequent sein, z.B. hast du die Variable i in Zeile 5 wohl vergessen zu deklarieren, so dass sie zur globalen Variablen wird, was die Kapselung untergräbt.
Was mir am meisten auffällt:
Vergleiche wie in if(etwas == false) sind meistens unnötig und manchmal führen sie auch zu unerwarteten Ergebnissen.
Mit true, false, 0, null, '' oder undefined sollte man entweder gar nicht vergleichen, d.h. einfach if(etwas) oder if(!etwas) notieren, oder aber – wenn man es denn sein muss – typgenau vergleichen mit den Operatoren === und !==.
Wenn du mehrfach auf ein Element zugreifst, so führe »document.getElementById(that.strip)« einmal aus und speichere den Rückgabewert zwischen.
Das würde ich noch allgemeiner formulieren: Eigentlich immer, wenn man auf ein Element, eine Eigenschaft oder einen berechneten Wert oder irgend etwas mehrfach zugreift, was nicht direkt im Scope liegt, sollte man es zwischenspeichern. Das erhöht die Performance und auch die Lesbarkeit bzw. Wartbarkeit des Codes. Z.B. hier:
if (that.dragObjekt != false) {
if ((posY - dragY) > that.min_top && (posY - dragY) < that.max_top) {
that.dragObjekt.style.top = (posY - dragY) + "px";
}
else if ((posY - dragY) <= that.min_top) {
that.dragObjekt.style.top = that.min_top + "px";
if (stripDownBusy == false) {
stripDown();
}
}
else if ((posY - dragY) >= that.max_top) {
that.dragObjekt.style.top = that.max_top + "px";
if (stripUpBusy == false) {
stripUp();
}
}
aktuellePos = posY - dragY;
aktualisiereBild();
return new Boolean(false);
}
wird unnötigerweise mehfach posY - dragY berechnet und auch mehrfach auf Objekte ausserhalb des aktuellen Scope zugegriffen.
Daher würde ich es lieber so notieren:
if (that.dragObjekt) {
var dragObjektStyle = that.dragObjekt.style,
minTop = that.min_top,
maxTop = that.max_top,
dy = posY - dragY;
if (dy <= minTop) {
dragObjektStyle.top = minTop + "px";
if (stripDownBusy) { stripDown(); }
}
else if (dy >= maxTop) {
dragObjektStyle.top = maxTop + "px";
if (stripUpBusy) { stripUp(); }
}
else {
c.top = dy + "px";
}
aktuellePos = dy;
aktualisiereBild();
return false;
}
Ist das nicht viel übersichtlicher und leichter verständlich?
Wenn man den Code so ein bisschen optimiert hat, sieht man oft auch strukturelle Ungereimtheiten viel besser. Ist es z.B. Absicht, dass die Funktion drag nur dann einen Rückgabewert liefert, wenn das dragObjekt nicht existiert?
Hier:
this.strip_map_top = new Array();
for (j=0; j<Presets.strip_map_top; j++) {
this.strip_map_top[i] = Presets.strip_map_top[i];
};
this.strip_map_bottom = new Array();
for (j=0; j<Presets.strip_map_bottom; j++) {
this.strip_map_bottom[i] = Presets.strip_map_bottom[i];
};
kopierst du offenbar Arrays. Das geht einfacher mit der slice-Methode:
this.strip_map_top = Presets.strip_map_top.slice(0);
this.strip_map_bottom = Presets.strip_map_bottom.slice(0);
Ob das Kopieren überhaupt nötig ist, könnte man noch untersuchen.
Gruß, Don P