jQuery-Code ist zu ausführlich, hätte gerne Hinweise, wie man ihn kürzer macht [closed]

Lesezeit: 11 Minuten

Benutzer-Avatar
Peiwen

Ich habe einen jQuery-Code, den ich gerne überprüfen und Hinweise dazu hätte, wie man die Anzahl der Zeilen verringert und verkürzt.

$('#p1').click(function() {
    $('#list').fadeOut(450);
    $('#q1').delay(600).fadeIn(450)
});
$('#p2').click(function() {
    $('#list').fadeOut(450);
    $('#q2').delay(600).fadeIn(450)
});
$('#p3').click(function() {
    $('#list').fadeOut(450);
    $('#q3').delay(600).fadeIn(450)
});
$('#p4').click(function() {
    $('#list').fadeOut(450);
    $('#q4').delay(600).fadeIn(450)
});

...

$('#p12').click(function() {
    $('#list').fadeOut(450);
    $('#q12').delay(600).fadeIn(450)
});
$('#p13').click(function() {
    $('#list').fadeOut(450);
    $('#q13').delay(600).fadeIn(450)
});

Kann dieser Code besser optimiert werden? Oder zumindest weniger ausführlich gemacht?

  • Oh Mann, das ist ein echtes Massaker mit falschen Antworten wegen Umfangsproblemen in einer Schleife. Hat jemand einen guten Link zu einer der vielen Fragen hier zu diesem Thema?

    – Thilo

    26. November 2011 um 11:17 Uhr


  • Das ist also eine Codeüberprüfung, ja?

    – zufällig

    26. November 2011 um 15:03 Uhr

  • @AnnaLear: Nein, bitte nicht. Dies ist eine absolut gute Frage für Stack Overflow. Im Code wurde auf ein sehr spezifisches Problem hingewiesen (zu repetitiv), und das OP fragte, wie das behoben werden könne. Und wenn man sich die ganze Diskussion darüber ansieht, war es sehr informativ für alle hier, es begann eine Diskussion über die Verwendung von IDs im Vergleich zu Klassen, über die Verwendung von Ereignisdelegierung und über den variablen Bereich in JS.

    – Thilo

    26. November 2011 um 23:30 Uhr


  • @Thilo Ich sage nicht, dass dies in irgendeiner Weise eine schlechte Frage ist. Aber wir haben eine SE-Site speziell für solche Dinge, also ist es zumindest teilweise meine Aufgabe, das bekannt zu machen.

    – Adam Lear

    26. November 2011 um 23:54 Uhr

  • Das OP wollte eine Codeüberprüfung. So einfach ist das. Das Refactoring und das Reduzieren von Zeilen ist Teil dessen, worum es bei Code Review geht. Dies ist ein Stück Code, das kleiner sein wollte. Zu sagen, dass niemand zur Code Review geht, weil wir alle hier sind, ist selbstzerstörerisch und engstirnig. @thi

    – zufällig

    27. November 2011 um 0:24 Uhr

Benutzer-Avatar
Xion

Sie können eine verwenden for Schleife, aber Sie sollten sicherstellen, dass der Wert des Schleifenzählers in den richtigen Bereich für gelangt click Ereignishandler:

var clickHandler = function(k) {
    return function() {
        $('#list').fadeOut(450);
        $('#q' + k).delay(600).fadeIn(450);
    };
};
for (var i = 1; i < 14; ++i) {
    $('#p' + i).click(clickHandler(i));
}

Ansonsten der delay und fadeIn würde sich bewerben #q13 Element ausschließlich, da der eigentliche Zähler (mit seinem Endwert von 13) in Schließung geraten würde.


BEARBEITEN: Da hier ziemlich viele Antworten falsch waren, werde ich versuchen, genauer zu erklären, was in diesem Code vor sich geht, da es ziemlich verwirrend zu sein scheint.

Die “natürliche” Lösung mit dem direkten Einfügen des Click-Handlers in die Schleife wäre die folgende:

for(var i = 1; i < 14; i++) {
    $('#p'+i).click(function() {
        $('#list').fadeOut(450);
        $('#q'+i).delay(600).fadeIn(450)
    });
}

Das entspricht aber keineswegs der erweiterten Form, die alle 13 Varianten nacheinander auflistet. Das Problem ist, dass hier zwar 13 Funktionen erstellt werden, diese aber alle über dieselbe Variable geschlossen werden i, dessen Wert sich ändert. Es kommt schließlich auf den Wert von an 13 und die Schleife endet.

Einige Zeit später die angehängten Funktionen #p1#p13 Elemente werden aufgerufen (wenn auf eines dieser Elemente geklickt wird) und sie verwenden diesen endgültigen Wert von i. Dies ergibt nur #q13 animiert werden.

Was hier getan werden muss, ist etwas namens zu tun Lambda-Lifting und eliminiere die freie Variable i, dessen Wert versehentlich geändert wird. Eine gängige Technik dafür besteht darin, eine “Factory-Funktion” bereitzustellen, die den Wert für unsere Variable akzeptiert und eine tatsächliche Funktion ausgibt, die wir als Event-Handler verwenden:

var clickHandler = function(k) {
    return function() {
        $('#list').fadeOut(450);
        $('#q' + k).delay(600).fadeIn(450);
    };
};

Da der Geltungsbereich von k Parameter ist lokal für clickHandlerjeder Anruf an clickHandler wird anders k Variable. Die Funktion, von der zurückgegeben wird clickHandler ist also abgeschlossen über verschiedene Variablen, die wiederum unterschiedliche Werte haben können. Genau das brauchen wir. Wir können dann anrufen clickHandler aus unserer Schleife und übergebe ihr den Wert des Zählers:

for (var i = 1; i < 14; ++i) {
    $('#p' + i).click(clickHandler(i));
}

Ich hoffe, das macht den Unterschied etwas deutlicher.


BEARBEITEN: Wie Esailija in den Kommentaren betonte, ist es auch möglich, es zu verwenden jQuery.each um einen ähnlichen Effekt zu erzielen:

$.each(new Array(13), function(idx) {
    $('#p' + (idx + 1)).click(function() {
        $('#list').fadeOut(450);
        $('#q' + idx).delay(600).fadeIn(450);
    });
});

Dies ist wahrscheinlich die Lösung der Wahl, wenn Sie sich bereits des Problems des Abschlusses/Scopings bewusst sind, das ich oben zu skizzieren versucht habe.

  • +1 einzige Antwort, die sich um den Umfang kümmert …

    – pimvdb

    26. November 2011 um 11:03 Uhr


  • Du könntest einfach verwenden jQuery.each der sich natürlich darum kümmert

    – Esailija

    26. November 2011 um 11:11 Uhr


  • jQuery.each ist alles schön und gut, aber ich denke, irgendwann muss jeder JavaScript-Programmierer verstehen, was hier genau vor sich geht. Und während es für dieses spezielle Beispiel ausreichen würde, hat es im Allgemeinen seine eigenen Fänge (z this innen und außen jQuery.each “Schleife”), die Sie bei der Verwendung beachten sollten.

    – Xion

    26. November 2011 um 11:27 Uhr


  • @Xion, nur die Verwendung der Factory-Funktion erklärt es auch kaum, es ist nur eine RY-Methode, dasselbe zu tun. Mein Kommentar wurde gemacht, bevor Sie Ihren Beitrag bearbeitet haben, um eine Erklärung zu haben. this wird im Onclick-Handler verwendet, was nichts mit der Art und Weise zu tun hat, wie Sie es schleifen – es wird so oder so das Element sein, sobald der Onclick aufgerufen wird.

    – Esailija

    26. November 2011 um 11:33 Uhr


  • Nein es wird nicht. Putten var local innerhalb der for Schleife ändert ihren Gültigkeitsbereich nicht. Es ist immer noch lokal innerhalb der Funktion, nicht der Schleifenblock. JavaScript hat überhaupt keinen Blockbereich. Probieren Sie das folgende einfache Beispiel in der Konsole Ihres Browsers aus, um es selbst zu sehen: a = []; for (var i = 0; i < 2; ++i) { var local = i; a.push(function() { alert(local); }); } Aufruf a[0]() und a[1]() beide führen zu einer Warnung mit 1statt 0 und 1beziehungsweise.

    – Xion

    26. November 2011 um 17:11 Uhr


Die ideale Lösung, IMO, im Gegensatz zur akzeptierten Antwort, besteht darin, Ihren Code nicht auf Beziehungen zwischen zu basieren id Werte, sondern auf Beziehungen im DOM basieren. jQuery bietet Ihnen eine index Methode, mit der Sie sehen können, wo sich Ihr Element im Verhältnis zu seinen Geschwistern befindet. Sie können diesen Wert dann verwenden, um das entsprechende andere anzuzeigende Element auszuwählen.

Dies setzt natürlich voraus, dass Sie Ihr HTML semantisch strukturieren. Zum Beispiel:

<div id="list">
    <a href="#">Link 1</a>
    <a href="#">Link 2</a>
    <a href="#">Link 3</a>
    <a href="#">Link 4</a>
    <a href="#">Link 5</a>
    <a href="#">Link 6</a>
    <a href="#">Link 7</a>
    <a href="#">Link 8</a>
</div>
<div id="questions"> <!-- perhaps this is what q stands for... -->
    <div>1 ...</div>
    <div>2 ...</div>
    <div>3 ...</div>
    <div>4 ...</div>
    <div>5 ...</div>
    <div>6 ...</div>
    <div>7 ...</div>
    <div>8 ...</div>
</div>

Der erste Link gilt für die erste Verschachtelung divvon Sekunde zu Sekunde usw. Das Markup ist einfach, gut lesbar und klar semantisch.

Ihr Code kann dann sehr einfach sein, ohne dass Sie sich Gedanken über Fabriken machen müssen. In der Tat ist der beste Weg, dies zu tun, das Bubbling und Delegieren von Ereignissen, das von jQuery bewundernswert gehandhabt wird on Methode (in 1.7; vorher verwenden delegate).

$('#list').on('click', 'a', function() {
    $('#list').fadeOut(450);
    $('#questions div').eq($(this).index()).delay(600).fadeIn(450);
});

Arbeiten mit jsFiddle (Ich weiß, dass die Animation etwas klobig aussieht, sorry.)

$(this).index() findet die Beziehung des aktuellen Elements zu seinen Geschwistern. .eq() filtert die Auswahl (von #questions div Elemente), um das Element an der entsprechenden Position zu finden.

Der Code kann nicht sein ziemlich so schnell (wenn Sie verwenden id Werte, es wird mit ziemlicher Sicherheit etwas schneller sein), aber Ihr Markup ist einfacher und daher robuster.

  • Ich stimme zwar zu, dass DOM-Daten ein hässlicher Hack sind, aber sie skalieren viel besser mit einer größeren Menge von Elementen, insbesondere wenn wir über Mouseovers oder Ereignisse sprechen, bei denen die Leistung kritischer ist. Für eine akzeptable Leistung in diesen Situationen ist die sauberste Lösung, die ich bisher gefunden habe, eindeutige Hashes, sodass die Indexsuche schnell bleibt, aber immer noch ziemlich hässlich ist. Immer noch +1, da wir über eine kleine Menge von Elementen und einen einfachen Klick hier sprechen 🙂

    – Esailija

    26. November 2011 um 23:54 Uhr

Wenn Sie darauf bestehen müssen, keine Ereignisdelegierung und Klassen zu verwenden, dann:

$.each( new Array(13), function(index){
    $('#p'+(index+1) ).click(function() {
        $('#list').fadeOut(450);
        $('#q'+(index+1)).delay(600).fadeIn(450)
    });
});

Verwenden von Klassen und Delegierung:

$( document ).delegate( ".my-class", "click", function(){
var idIndex = this.id.match( /\d+$/, )[0];

        $('#list').fadeOut(450);
            $('#q'+idIndex).delay(600).fadeIn(450)

});

  • Bevor jemand die Verwendung einer Klasse ablehnt: Es gibt einen Effizienzvorteil bei der Verwendung der Klassennamendelegierung anstelle eines übermäßig komplexen Trickselektors, da er jedes Mal ausgewertet werden muss, wenn auf das Dokument geklickt wird.

    – Esailija

    26. November 2011 um 11:27 Uhr


  • Fast +1 für Klassen, aber Sie verwenden immer noch IDs, um die beiden entsprechenden abzugleichen p und q. Vermutlich geht das auch mit Klassen und relativen Positionen im Dokument.

    – Thilo

    26. November 2011 um 11:36 Uhr

  • @Thilo, es ist wahrscheinlich effizienter, die ID direkt abzurufen, anstatt jedes Mal die Dokumentpositionen zu vergleichen. Und da die IDs sowieso da sind, warum nicht verwenden 🙂 Mindestens +1 für den neuen Array-Trick, wenn überhaupt: D

    – Esailija

    26. November 2011 um 11:39 Uhr


  • Die ID ist wahrscheinlich sowieso nicht vorhanden, sondern musste für genau diese Animation hinzugefügt werden (natürlich nur meine Vermutung).

    – Thilo

    26. November 2011 um 11:41 Uhr

  • +1 für die Verwendung von Klassen und Ereignisdelegierung! Für Leute, die keine Regex wollen, können sie dies tun, wenn sie es vorziehen $('#'+this.id.replace('p','q')).

    – RightSaidFred

    26. November 2011 um 14:09 Uhr


Benutzer-Avatar
Abdul Munim

Wie andere können Sie a verwenden for-loop dies zu tun, aber Sie verpflichten sich tatsächlich dazu 14 ps und wann immer Sie eine neue hinzufügen p Sie müssen Ihre erhöhen for-loop Testbedingung.

Das würde ich tun:

$("[id^=p]").bind("click", function() {
    $('#list').fadeOut(450);
    $('#q' + $(this).attr("id").match(/^#p(\d+)$/i)[1]).delay(600).fadeIn(450)
})

Könnten Sie nicht einfach eine Klasse anstelle all dieser verschiedenen IDs verwenden? Und dann eine Navigation, um das passende andere Element zu finden?

$('.myCoolEffect').click(function(event) {
    $('#list').fadeOut(450);
    // find the matching element based on this on somehow
    $(event.currentTarget).up(1).delay(600).fadeIn(450);
});

  • woher hast du .up aus?

    – pimvdb

    26. November 2011 um 11:39 Uhr

  • .up ist nur ein Beispiel. Vermutlich gibt es eine Art festen Weg, von dem aus man navigieren kann p5 zu q5 im Dokument.

    – Thilo

    26. November 2011 um 11:42 Uhr


Benutzer-Avatar
Gemeinschaft

Versuche dies,

$('[id^="p"]').live('click', function(e){
    var temp = this.id.match(/^p(\d+)$/);
    if(temp){
       $('#list').fadeOut(450);
       $('#q' + temp[1]).delay(600).fadeIn(450);
    }

});

Ich habe benutzt jqueryStartsWith-Selektor & Live-Funktion.

Aktualisieren:

Xion kommentierte: Wenn Sie diesen Code verwenden, wirkt sich dies auf alle anderen Elemente von Dessen aus id's beginnen mit p.

Wenn Sie mehr Zeichen in die ID-Mittel einfügen könnten, ist dies kein Problem.

Aktualisierung 2:

Wie BrunoLM im Kommentar erwähnt hat, habe ich den Code leicht geändert, um das Ereignis nicht für andere Elemente auf dem Dom auszulösen, mit denen die ID beginnt p.

Das var temp = this.id.match(/^p(\d+)$/); und der folgende if-Block kümmert sich um andere Elemente von deren id beginnt mit p.

match(/^p(\d+)$/) kehrt zurück null für die id wie pblabla, p2sdad, pdasds2323 und es gibt ein Array für die IDs wie zurück p1, p2, ... p100000....

  • woher hast du .up aus?

    – pimvdb

    26. November 2011 um 11:39 Uhr

  • .up ist nur ein Beispiel. Vermutlich gibt es eine Art festen Weg, von dem aus man navigieren kann p5 zu q5 im Dokument.

    – Thilo

    26. November 2011 um 11:42 Uhr


Benutzer-Avatar
kba

$('div').filter(function () {
  return this.id.match(/^p([0-9]+)$/);
}).click(function () {
  $('#list').fadeOut(450);
  $('#' +$(this).attr('id').replace('p','q')).delay(600).fadeIn(450)
});

1229250cookie-checkjQuery-Code ist zu ausführlich, hätte gerne Hinweise, wie man ihn kürzer macht [closed]

This website is using cookies to improve the user-friendliness. You agree by using the website further.

Privacy policy