Wie zerlege ich diese 100-Zeilen JS Funktion in Clean Code?

[hmm]

Hallo,

ich habe folgende 100-Zeilen Funktion geschrieben:

function createTableFromJson(data) {
                data_global = data;
                
                var tr_header = document.createElement("tr");
                var tr_underline = document.createElement("tr");
                var th = document.createElement("th");
                tr_header.appendChild(th);
                tr_header.appendChild(document.createElement("th"));
                tr_header.appendChild(document.createElement("th"));
                tr_header.appendChild(document.createElement("th"));
                var tdA = document.createElement("td");
                tdA.innerHTML = "";
                tr_underline.appendChild(tdA);
                tr_underline.appendChild(document.createElement("td"));
                tr_underline.appendChild(document.createElement("td"));
                tr_underline.appendChild(document.createElement("td"));
                
                for(var i = 0; i < data[0].skills.length; i++) {
                    var th_ = document.createElement("th");
                    th_.innerHTML = "";
                    tr_header.appendChild(th_);
                    
                    if(i > 0) {
                        var th_1 = document.createElement("th");
                        th_1.innerHTML = "";
                        tr_header.appendChild(th_1);
                    }
                    
                    var th2 = document.createElement("th");
                    th2.innerHTML = data[0].skills[i].skill;
                    tr_header.appendChild(th2);
                    
                    var tdA2 = document.createElement("td");
                    tdA2.innerHTML = "Wissen";
                    tr_underline.appendChild(tdA2);
                    
                    var tdA3 = document.createElement("td");
                    tdA3.innerHTML = "Erfahrung";
                    tr_underline.appendChild(tdA3);
                    
                    var tdA4 = document.createElement("td");
                    tdA4.innerHTML = "Interesse";
                    tr_underline.appendChild(tdA4);
                }
                document.getElementsByTagName("table")[0].appendChild(tr_header);
                document.getElementsByTagName("table")[0].appendChild(tr_underline);
                
                for(var i = 0; i < data.length; i++) {
                    var tr = document.createElement("tr");
                    
                    var tdCh = document.createElement("input");
                    tdCh.type = "Checkbox";
                    tdCh.setAttribute("name", data[i].personenId);
                    tr.appendChild(tdCh);
                    
                    var tdNr = document.createElement("td");
                    tdNr.innerHTML = data[i].personenId;
                    tdNr.setAttribute("contenteditable", false);
                    tr.appendChild(tdNr);
                    
                    var td = document.createElement("td");
                    td.innerHTML = data[i].vorname;
                    td.setAttribute("contenteditable", true);
                    td.setAttribute("name", i + "," + "vorname");
                    tr.appendChild(td);
                    
                    var tdNA = document.createElement("td");
                    tdNA.innerHTML = data[i].nachname;
                    tdNA.setAttribute("contenteditable", true);
                    tdNA.setAttribute("name", i + "," + "nachname");
                    tr.appendChild(tdNA);
                    
                    for(var k = 0; k < data[i].skills.length; k++) {
                        var td_skill1 = document.createElement("td");
                        td_skill1.innerHTML = data[i].skills[k].wissen;
                        td_skill1.setAttribute("contenteditable", true);
                        td_skill1.setAttribute("name", i + "," + "skills," + k + ",wissen");
                        tr.appendChild(td_skill1);
                        
                        var td_skill2 = document.createElement("td");
                        td_skill2.innerHTML = data[i].skills[k].erfahrung;
                        td_skill2.setAttribute("contenteditable", true);
                        td_skill2.setAttribute("name", i + "," + "skills," + k + ",erfahrung");
                        tr.appendChild(td_skill2);
                        
                        var td_skill3 = document.createElement("td");
                        td_skill3.innerHTML = data[i].skills[k].interesse;
                        td_skill3.setAttribute("contenteditable", true);
                        td_skill3.setAttribute("name", i + "," + "skills," + k + ",interesse");
                        tr.appendChild(td_skill3);
                    }
                    
                    document.getElementsByTagName("table")[0].setAttribute("align", "center")
                    document.getElementsByTagName("table")[0].appendChild(tr);
                }
            };

Ich war müde und habe den Code einfach runtergehackt.

Der Code baut mir folgende Tabelle:

Testwebseite

Login geht mit: Christopher hallo123

In der Tabelle kann man die Zahlen ändern, was direkt gespeichert wird und die Namen, was auch direkt gespeichert wird.

Ich wollte eine schöne Skillmatrix als Klausurvorbereitung schreiben und muss den Code oben dafür noch erweitern. Frage: Zerlege ich die Funktion oben in Unterfunktionen oder lieber in Objekte? Oder tausch ich den Code direkt gegen irgendeine Bibliothek aus?

[Tabellenkalk]

Hallo,

Zerlege ich die Funktion oben in Unterfunktionen oder lieber in Objekte?

Das sind ja keine sich gegenseitig ausschließende Konzepte. Hast du schon mal von DRY gehört? Du hast in deinem Script vieles mehrfach drin, das kann man prima in Funktionen auslagern.

Gruß
Kalk

[hmm]

Jo, den Redundanten Code würde ich im ersten Schritt auslagern. Ich überlege aber gerade, ob ich dieses ganze Tabellen Zeug nicht noch effizienter machen kann. Z.B. in Form eines Objekts, dass wiederum Funktionen hat. Würde ich die Tabelle direkt als Objekt schreiben, müsste ich mir auch überlegen ob dieses Objekt bereits die DOM Sachen erledigt oder ob ich das Objekt an eine Funktion übergebe, die diese DOM Dinge tut.

Hintergrund meiner Überlegungen ist auch, dass der Tabellencode noch wachsen wird.

[chorn]

bei

var td_skill1 …

weisst du ja wie viele da sind. Da kannst du ja ne Schleife und/oder Funktion draus machen und die X mal aufrufen.

[hmm]

Ich bin auch gerade dabei eine zweite Tabelle zu schreiben die an der linken Seite angezeigt werden soll. Die Mitarbeitertabelle soll weiterhin im Zentrum stehen.

<main>
            Hallo @username.
            <table id='tabelleSkills'></table>
            <table id='tabelleMain'></table>
            <input type="button" onclick="createNewMitarbeiter();">Neuen Mitarbeiter hinzufügen</input>
            <input type="button" onclick="removeMitarbeiter();">Ausgewählte Mitarbeiter löschen</input>
</main>

Das ist mein Main Bereich. Zieh ich die tabelleSkills per CSS in den linken Bereich?

Mein bisheriges CSS für die Main sieht so aus:

main {
	background-color: white;
	padding: 100px 0 400px;
	right: 0px;
	bottom: 0;
	left: 0;
	position: relative;
        text-align: center;
	color: black;
}

[hmm]

Sollte ich die CSS Anweisungen im JavaScript Code drin haben, so:

function createSkillTableFromJson(data) {
    	        var tr_header = document.createElement("tr");
    	        var th = document.createElement("th");
                th.innerHTML = "Skills";
                tr_header.appendChild(th);
                
                document.getElementsByTagName("table")[1].appendChild(tr_header);
    	        document.getElementsByTagName("table")[1].setAttribute("align", "left");
    	    };

Oder sollten CSS Dinge lieber weiterhin in einer CSS Datei liegen? Was ist die bessere Wahl für sauberen Code?

[dedlfix]

Tach!

Sollte ich die CSS Anweisungen im JavaScript Code drin haben, so:

Gibt es denn eine Notwendigkeit, sie in Javascript zu setzen?

dedlfix.

[beatovich]

hallo

Sollte ich die CSS Anweisungen im JavaScript Code drin haben, so:

Im Sinne der Konsistenz-Erhaltung sollte man sich darauf beschränken, mit Javascript bestenfalls Klassen zu setzen. Alles andere kann die Zahl der Baustellen vervielfachen.

[Gunnar Bittersmann]

@@beatovich

Im Sinne der Konsistenz-Erhaltung sollte man sich darauf beschränken, mit Javascript bestenfalls Klassen zu setzen

… oder andere Attribute.

In diesem responsiven Menü (Viewport so klein machen bis der Hamburger kommt) ist es das aria-expanded-Attribut. Da braucht man kein Klassenattribut.

Und dass die Sichtbarkeit übers hidden-Attribut geregelt werden kann/sollte, hatten wir auch schon desöfteren hier im Forum besprochen.

LLAP 🖖

[Julius]

Hallo hmm,

Sollte ich die CSS Anweisungen im JavaScript Code drin haben, so:

// […]
document.getElementsByTagName("table")[1].setAttribute("align", "left");
// […]

das ist noch nicht einmal CSS, sondern obsoletes HTML. Mach es mit CSS, das du in einer separaten Datei unterbringst.

Gruß
Julius

[dedlfix]

Tach!

Oder tausch ich den Code direkt gegen irgendeine Bibliothek aus?

Wenn du etwas schreibst, das im Grunde genommen eine SPA ist, dann kannst du dir ja mal anschauen, wie sich Frameworks für SPAs bedienen lassen und inwieweit sie dir das Leben erleichtern oder vielleicht auch nicht.

Es ist zum Beispiel wesentlich einfacher (auch beim späteren Lesen und Erweitern), HTML als HTML zu schreiben und darin Platzhalter zu haben, als das DOM mit vielen einzelnen Funktionsaufrufen zu erstellen.

dedlfix.

[beatovich]

hallo

Wenn du etwas schreibst, das im Grunde genommen eine SPA ist, dann kannst du dir ja mal anschauen, wie sich Frameworks für SPAs bedienen lassen und inwieweit sie dir das Leben erleichtern oder vielleicht auch nicht.

Bist du sicher, dass dein Vorgänger das Acronym SPA versteht? http://acronyms.thefreedictionary.com/spa

[dedlfix]

Tach!

Bist du sicher, dass dein Vorgänger das Acronym SPA versteht? http://acronyms.thefreedictionary.com/spa

Beeindruckende Liste ... die aber schnell schrumpft, wenn man Kontext hinzunimmt. Die en.Wikipedia hat im Bereich Computing und technology nur noch 5 Einträge. Und wenn er da nicht den richtigen findet, dann kann er immer noch nachfragen.

dedlfix.

[hmm]

Singel Page Application hatte ich in meiner Vorlesung. Wir haben das mit DOM und der Template Funktion von HTML gemacht.

Ich versuche gerade eine "belastbare" Architektur für meine Tabellen zu finden.

Ich bräuchte Beispiele für Architekturen. Welche Kernfunktionen und Kernobjekte könnten welche Aufgaben übernehmen. Die DOM Geschichte sollte ich wahrscheinlich vom Bau des Tabellenobjekts trennen, das Senden und holen der Daten via Post/Get sollte ich besser auch in ein Kernfunktion packen anstatt mehrere Funktionen zu haben die nahezu das gleiche machen.

Versteht Ihr mein Konzeptionelles Problem?

[Rolf b]

Du verwendest doch eh schon Angular. Ich selbst habe es noch nicht verwendet, aber ich weiß, dass es Zweiwege-Datenbindung beherrscht. Schau doch mal, ob Du mit Angular nicht Funktionen bekommst, die deine JS Tabelle 1:1 auf ein Template für die HTML Table mappen können. Alles von Hand per JS rauszurendern ist sowas von öde... das schreit nach Templating.

Du solltest von Angular dann auch Features bekommen, die Dir Modelländerungen anzeigen, so dass Du das Modell mit dem Server synchronisieren kannst.

Ob editable

Rolf

[hmm]

Danke, dass klingt interessant.

Anstelle von NodeJs/Express nutze ich das Skala Play Framework für die Datenkommunikation zwischen Client und Server.

Woran erkennst du, dass ich Angular verwende? Das mach ich nicht bewust.

[Rolf b]

Sieht man im Netzwerktrace der Seite. Sie lädt jQuery und Angular.

Rolf

[hmm]

Hallo Leute,

CSS auslagern würde heißen, dass ich das setAttribute aligne aus dem JS Code rausnehme und in die CSS Datei schreibe?

Hier mein überarbeiteter Tabellencode:

            function createTableFromJson(data) {
                data_global = data;
                
                var skillHeader = document.createElement("tr");
                var attributeHeader = document.createElement("tr");
                
                setPlatzhalterInRow(4, skillHeader, "td");
                setPlatzhalterInRow(4, attributeHeader, "td");
                
                for(var i = 0; i < data[0].skills.length; i++) {
                    setPlatzhalterInRow(1, skillHeader, "td");
                    
                    if(i > 0) {
                        setPlatzhalterInRow(1, skillHeader, "td");
                    }
                    
                    setElementInRow(skillHeader, "th", data[0].skills[i].skill);
                    setElementInRow(attributeHeader, "td", "Wissen");
                    setElementInRow(attributeHeader, "td", "Erfahrung");
                    setElementInRow(attributeHeader, "td", "Interesse");
                }
                document.getElementsByTagName("table")[0].appendChild(skillHeader);
                document.getElementsByTagName("table")[0].appendChild(attributeHeader);
                document.getElementsByTagName("table")[0].setAttribute("align", "center");
                
                for(var i = 0; i < data.length; i++) {
                    var line = document.createElement("tr");
                    
                    setElementWithAttributeInRow(line, "input", "Checkbox", "", "Checkbox", ["name"], [data[i].personenId]);
                    setElementWithAttributeInRow(line, "td", "", "", data[i].personenId, ["contenteditable"], [false]);
                    setElementWithAttributeInRow(line, "td", "", "", data[i].vorname, ["contenteditable", "name"], [true, i + "," + "vorname"]);
                    setElementWithAttributeInRow(line, "td", "", "", data[i].nachname, ["contenteditable", "name"], [true, i + "," + "nachname"]);
                    
                    for(var k = 0; k < data[i].skills.length; k++) {
                        setElementWithAttributeInRow(line, "td", "", "", data[i].nachname, ["contenteditable", "name"], [true, i + "," + "skills," + k + ",wissen"]);
                        setElementWithAttributeInRow(line, "td", "", "", data[i].nachname, ["contenteditable", "name"], [true, i + "," + "skills," + k + ",erfahrung"]);
                        setElementWithAttributeInRow(line, "td", "", "", data[i].nachname, ["contenteditable", "name"], [true, i + "," + "skills," + k + ",interesse"]);
                    }
                    
                    document.getElementsByTagName("table")[0].appendChild(line);
                }
            };
            
            function setElementWithAttributeInRow(line, elementType, type, innerHtml, className, attributeNameArray, attributeValueArray) {
            	var element = document.createElement(elementType);
            	element.type = type;
            	element.className = className;
            	element.innerHTML = innerHtml;
            	
            	for(var i = 0; i < attributeNameArray.length; i++) {
            		element.setAttribute(attributeNameArray[i], attributeValueArray[i]);
            	}
            	
            	line.appendChild(element);
            	return line;
            };
            
            function setElementInRow(line, elementType, innerHtml) {
            	var element = document.createElement(elementType);
            	element.innerHTML = innerHtml;
            	line.appendChild(element);
            	return line;
            };
            
            function setPlatzhalterInRow(anzahlPlatzhalter, line, platzhalterType) {
            	for(var i = 0; i < anzahlPlatzhalter; i++) {
            		line.appendChild(document.createElement(platzhalterType));
            	}	
            	return line;
            };

was wären die nächsten Schritte zur Verbesserung des Codes?

[dedlfix]

Tach!

was wären die nächsten Schritte zur Verbesserung des Codes?

Wenn man alles zu Fuß macht, wird es nur geringfügig besser, indem man die Wiederholungen in Schleifen und Funktionen packt. Und viel mehr Potential sehe ich bei diesem Prinzip nicht. Da hilft auch nicht, wenn man von prozedural auf OOP umsteigt, oder ähnliche Maßnahmen. Der Code bleibt am Ende schwer überschaubar, weil er ziemlich geschwätzig ist, und jeden kleinen Schritt mit Einzelanweisung ausführen muss. Man hat sich nicht umsonst Muster wie MVC oder MVVM und darauf basierende Frameworks ausgedacht. Dadurch wird die Sachlage ingesamt nicht weniger komplex, aber die Komplexität wird im Framework versteckt, so dass es für den Programmierer und Leser eine Ebene weiter oben wieder übersichtlicher ausschaut, und man sich auf wesentlichere Dinge konzentrieren kann, als einzelne HTML-Elemente ins DOM zu pflanzen.

dedlfix.

[hmm]

meinst du sowas wie d3?

Ich baue das gerade auf einer Ubuntu VM in Cloud 9 io und hab noch ca. 200 MB Physischen Speicher. Kannst du mir ein Framework nennen, dass sich leicht benutzen und installieren lässt?

[dedlfix]

Tach!

meinst du sowas wie d3?

Nein, das ist im Grunde dasselbe Prinzip nur mit anderer Syntax. Das bringt nur unwesentliche Verbesserungen.

Ich baue das gerade auf einer Ubuntu VM in Cloud 9 io und hab noch ca. 200 MB Physischen Speicher. Kannst du mir ein Framework nennen, dass sich leicht benutzen und installieren lässt?

Leicht wird eine Sache immer erst, nachdem man sich eingearbeitet hat. Und können kann ich dir nur Angular nennen. Konkret: Angular 2. Das alte Angular 1, oder auch AngularJS genannt, ist sozusagen ein ganz anderes Produkt mit größtenteils eigener Philosophie.

Versprechen kann ich dir auch, dass du nach der grundlegenden Einarbeitung in Angular, deine Anwendung neu schreiben musst, wenn du dich für Angular entscheidest.

dedlfix.

[hmm]

Ok, danke. Ich werd mir Angualar anschauen, ich weiß aber noch nicht ob ich dass in diesem Projekt verwenden werde.

Hast du ein paar Ideen für das Aussehen der Tabelle? Aktuell weiß ich nichtmal, welche farben zur Seite passen würden.

[dedlfix]

Tach!

Hast du ein paar Ideen für das Aussehen der Tabelle? Aktuell weiß ich nichtmal, welche farben zur Seite passen würden.

Schwarz und Weiß und Grautöne passen immer. Ansonsten musst du mal bei Designern vorbeischauen. Üblicherweise legen die sich eine Palette an miteinander harmonierenden Farben zurecht und bauen darauf die Farbgebung auf. Solche Palettenvorschläge gibt es auch zuhauf im Web.

dedlfix.

[Rolf b]

Wenn Du Angular nur "irrtümlich" drin hast, kann ich Dir auch noch knockout vorschlagen. Das ist eine JS Library die sich ausschließlich um das MVVM Thema kümmert und vor allem ein sehr gutes Online-Tutorial hat.

Es wird nur kritisch, wenn die Tabellen groß werden, dann dauert das Auflösen der Templates zu lange. Dann ist aber auch dein handgemachter Ansatz langsam. Grund dafür ist, dass der Browser nach jeder Änderung am DOM das Layout neu berechnet. Wenn's eine Methode gäbe, mit der sich dieses ständige Re-Rendering unterbinden ließe, würde Knockout sie garantiert benutzen, von daher vermute ich, dass es sie nicht gibt. Bei mir war es so, dass ich ca 1000 Zeilen hatte und er mehrere Sekunden brauchte, um das aufzubauen. Ich bin dann auf Kendo umgestiegen (Payware), weil das eh im Unternehmen war und es auch gleich eine Funktion zur seitenweisen Anzeige mitbrachte. Seitdem kämpfe ich nicht mehr mit Kendo gegen meine Probleme, sondern gegen die Probleme, die ich mit Kendo habe...

Falls es bei Dir zu langsam wird, kannst Du den brute force Ansatz, den die Kendo Widgets verwenden. zu adaptieren versuchen. Sie bauen nicht Stück für Stück das DOM mit DOM-Methoden auf, sondern erzeugen die Tabelle als einen langen String mit dem passenden HTML darin und setzen den als innerHTML in einen Container. Dann wird nur einmal layoutet, es ist nur schwieriger, alle Change-Events richtig zuzuordnen und ich denke, dass Knockout das deshalb nicht macht. Wenn Du HTML zu Fuß als String aufbaust, kannst Du an einigen Stellen auch auf Funktionsaufrufe verzichten und einfach Stringkonstanten verwenden (z.B. für die Überschriften).

Rolf

[Danyal]

Hallo!

deine Aufräum-Arbeit in allen Ehren, aber ich finde diesen Code nicht besser, sondern sogar unübersichtlicher. Man kann ihn schwerer lesen. Er ist weniger sprechend.

Du hast dir zwei Helfer-Funktionen gebaut, um Wiederholungen zu vermeiden. Erst einmal was gutes. Aber mit zum Teil 7 Parametern, darunter 2 komplexe, korrespondierende Arrays. Da weiß man gar nich mehr was passiert. Das ist nicht Clean Code, das ist nur Verkürzung und Verschleierung. In JS würde man eher ein Object-Hash übergeben mit sprechenden Keys.

DOM-Elemente zu erzeugen ist immer ekelig. Es gibt DOM-Bibliotheken, die das vereinfachen. Die bekannteste ist wohl jQuery. Oder etwas wie redom.

Alternativ gibt es Template-Bibliotheken (ältere Übersicht). Da kannst du ganz normales HTML schreiben und Strings hinein bauen.

Wenn es etwas mehr sein soll, helfen dir größere Frameworks wie Angular, React, Vue oder Ember. Die bringen gute Template- bzw. DOM-Lösungen und Data Binding mit sich.

Ich würde erst einmal den einfachen Template-Weg ausprobieren und schauen ob der Code dann einfacher und besser lesbar ist.

Später würde ich Richtung React oder Angular gehen, wenn du eine Menge solchen Codes und zu dem eine dynamische Seitenoberfläche hast, also das DOM immer wieder neu zusammengebaut oder aktualisiert werden muss.

Viel Erfolg!

Danyal

[pl]

Mit einer einfachen Templating Engine die mit Array's umgehen kann bist Du mit sowas besser beraten. Erstens isses viel weniger Aufwand, 2. wartungsfreundlicher und 3. ist das Layout vom Code getrennt.

Dein Code hingegen sieht so aus, also wolltest Du den nie wieder anfassen 😉

MfG