Wie erhält man sich die Lesbarkeit?

[bokel]

Idealerweise wäre ein Stück Code ja so zu lesen wie ein Text. In der Realität schlüpfen aber an allen Ecken und Enden Implementationsdetails in den Code, die der Lesbarkeit alles andere als zuträglich sind.
Ich habe hier mal ein kleines Beispiel gebaut. Ein einfacher Diaprojektor, den man mit einem Magazin voller Bilder füttern kann, das er dann automatisch durchfährt.
Ok, gegen die Konvention mit public static main als Einstieg usw. kann man wenig machen, aber das ist schon mal die erste Sache, die davon ablenkt, was eigentlich gemeint ist. Wem fallen noch andere Sachen auf und wie könnte man das verbessern?

mfg. r

[André Michelle]

Erstmal das Banale:

Ich möchte die Klammern {} in einer Spalte haben. Es ist eine Fehlerquelle für Anfänger, die immer mal wieder eine Klammer vergessen und nicht auf einen Blick sehen, wo.

Das Kennzeichen "Impl" finde ich sehr fragwürdig. Wozu ein Interface, wenn ich schon in der Namensgebung der Klasse darauf hinweise, dass es nur eine Implementation gibt ? Darüber kann streiten, weil es den Code besser veränderlich macht. Im besten Fall weiss man halt gleich wo der Hammer hängt. Trotzdem würde ich mir das niemals antun.

Deine Graphics würde ich nicht mit statischen Methoden lösen. Graphics sollte instanzierbar sein und einen MovieClip als Member beherbergen.
In deinem Fall ist das eher ein GraphicsUtil, weil du nur ein paar Funktionen ohne reelen Bezug definierst.

'ui' als Referenzname für MovieClip finde ich seltsam. Ein MovieClip ist zunächst doch kein UserInterface (-element) ? Ich benutze immer: timeline, container oder auch nur g, wenn ich ihn zum Zeichnen mit der drawAPI brauche. Wenn du dem Clip noch UserControls verpassen würdest, dann könnte ich mich damit anfreunden.

Für erste. Das wird bestimmt spannend :)

[Janoscharlipp]

Also die hier gefällt mir garnicht:

private function startAutoSlideChanging( p:Projector){
        var id:Number;
        var onInterval:Function = function(){
            if( p.isSliding()) return;
            if( p.hasSlide()) p.nextSlide(); else clearInterval(id);
        }
        onInterval();
        id = setInterval( onInterval, 500);
    } 

Stattdessen würde ich sowas bevorzugen. Nur das Handling der intervalId gefällt mir nicht, bin mir nichtmal sicher, ob es so funktioniert.

private function startAutoSlideChanging( p:Projector){
        var intervalId:Number;
 
        changeSlide(p);
 
        intervalId = setInterval(changeSlide, 500, p, intervalId);
    }
 
    public function changeSlide(p:Projector, intervalId:Number) : Void
    {
        if (p.isSliding()) {
            return;
        }
 
        if (p.hasSlide()) {
            p.nextSlide();
        } else {
            clearInterval(intervalId);
        }
    } 

[bokel]

Da hast du Recht, Janoscharlipp. In der Richtung kann man noch mehr machen.

private var intervalId:Number = -1;
    
    private function startAutoSlideChanging(ms:Number){
        if( changeSlide()){
            startInterval(ms);
        }
    }
    
    private function stopAutoSlideChanging(){
        stopInterval();
    }
    
    private function startInterval(ms:Number){
        if( intervalId != -1) stopInterval();
        intervalId = setInterval( this, onInterval, ms);
    }
    
    private function stopInterval(){
        clearInterval( intervalId);
        intervalId = -1;
    }
    
    private function onInterval(){
        if( ! changeSlide()) stopInterval();
    }
    
    private function changeSlide():Boolean{
        if( p.isSliding()){ 
            return true;
        }
        if( p.hasSlide()){ 
            p.nextSlide(); 
            return true;
        } 
        return false;
    } 

Etwas abgewandelt von deiner Version gefällt mir das auch besser. Es ist allerdings jetzt schon fast soviel Code, dass ich drüber nachdenke eine eigene Klasse dafür zu schreiben. Eine Klasse, die ein Interval verwaltet, dass solange ausgeführt wird, bis ihr Intervalhandler false liefert.

@Andre: Das mit den geschweiften Klammern in einer Spalte ist Geschmackssache, finde ich. Mir ist das dann zuviel Whitespace.

Die Impls rühren daher, dass ich mit den Interfaces angefangen habe. Es stimmt, wenn sich abzeichnet, dass ich wirklich nur eine Implementation habe, kann ich die Interfaces löschen und die Impl umbenennnen.

Die statischen Graphics finde ich genau richtig so. Es ist für mich nur eine Methode um ein Rechteck zu zeichnen. Ich mache ja nichts weiter damit. Was du vorschlägst würde auf einen Decorator hinauslaufen. Den brauche ich aber nicht wirklich, weil ich das Rechteck nicht mehr anfasse.

ui ist für mich alles, was mit Anzeige zu tun hat. g oder graphics gefällt mir aber auch.

mfg. r

[Janoscharlipp]

Also ganz allgemein mag ich das erstellen von Funktionen zur Laufzeit nicht (das hatten wir doch schonmal ) wenn die dann auch noch so durchgereicht werden (onComplete) und überhaupt nicht mehr klar ist, wer was und welcher scope, dann würde ich versuchen es anders zu machen.
Also Funktionen am richtigen Ort definieren, und dann zum Beispiel mit Delegate.create. Das das das () Problem nur verschiebt ist mir klar, aber dein Code wird dann schöner.
Alternativ könntest du eigentlich auch Listener verwenden, warum sollte sich beispielsweise der Projektor nicht beim sNew als Listener anmelden?

[flory]

Ich finde die Bezeichnungen nicht so toll:

Interfaces würde ich immer mit 'I' am Anfang bennen, das macht mittlerwile Adobe auch so.

Aus dem Namen 'startAutoSlideChanging' wird nicht klar, was die Methode eigentlich macht.
Das hat sicher auch was zu tun, das wir Deutschen uns mit englischen Bezeichnungen immer etwas
schwer tun.
Wie wärs mit 'startAutomaticSlideshow'?

'public static void main' stört finde ich gar nicht, das kennt eigentlich jeder und es ist
klar, dass es dort losgeht.

Gruz
Florian

[André Michelle]

Es ist allerdings jetzt schon fast soviel Code, dass ich drüber nachdenke eine eigene Klasse dafür zu schreiben.

Definitiv. Das ist ein klassischer Timer.

Das mit den geschweiften Klammern in einer Spalte ist Geschmackssache, finde ich. Mir ist das dann zuviel Whitespace.

Kann ich verstehen. Das Argument bezog sich auf Anfänger. Ich gebe gerade einen Kurs, wo genau diese Fehler immer wieder vorkommen. Seitdem ich darauf bestehe, gibt es weniger Fehler.

Die Impls rühren daher, dass ich mit den Interfaces angefangen habe. Es stimmt, wenn sich abzeichnet, dass ich wirklich nur eine Implementation habe, kann ich die Interfaces löschen und die Impl umbenennnen.

Das mache ich auch oft, aber benenne die Implementation gleich anders. In der Regel gehe ich davon aus, dass mehrere Klassen das interface implementieren. Generell ist das auch bei mir eine Baustelle. Ich mag das 'I'-Prefix genauso wenig wie das 'Impl' Postfix.

Die statischen Graphics finde ich genau richtig so. Es ist für mich nur eine Methode um ein Rechteck zu zeichnen. Ich mache ja nichts weiter damit. Was du vorschlägst würde auf einen Decorator hinauslaufen. Den brauche ich aber nicht wirklich, weil ich das Rechteck nicht mehr anfasse.

Ja, aber wäre es dann nicht völlig ausreichend alle zeichenbaren Formen in die Graphics Klasse einzubauen ? Ich finde die Rect Klasse dann überflüssig. Diese sollte wirklich ein Rechteck beschreiben und nicht zeichnen.

ui ist für mich alles, was mit Anzeige zu tun hat. g oder graphics gefällt mir aber auch.

Ich unterscheide halt, welche Funktion der MovieClip haben wird. MovieClip ist an sich bis AS2 eine Monsterklasse mit viel zu viel Overhead. Daher ist eine feine Unterscheidung schon nicht verkehrt.

[bokel]

@Andre: Alles in eine Klasse einbauen skaliert aber nicht so schön, Wenn ich andere Formen hinzufügen möchte, müßte ich immer die Graphics-Klasse anfassen. Das möchte ich vermeiden.

@Flory: Das nenne ich mal einen schönen Namen. Das passt.
Was das I angeht - das kann man in einem Framework machen, in dem feststeht, was ein Interface ist. In einer Applikation möchte ich da lieber flexibel bleiben. Ohne I kann ich ein Interface jederzeit gegen eine Klasse austauschen, zumindest, wenn ich eine Factory anstatt new benutzt hätte .

@Janosch: Ich benutze haeufig anonyme Funktionen als Glue. Echte Methoden finde ich dafür zu statisch. Meine Funktionen sind aber immer Delegates, also der Scope ist mit drin, darum soll sich der Empfänger nicht kümmern müssen. Events sind u.U. auch gut dafür, allerdings hat man dann wieder Sorgen mit dem Speicher usw. Für Sachen, die bleiben, sind Events gut. Für so schnelle Oneshots eher nicht.

mfg. r

[elias]

Ich finde deinen Umgang mit Whitespace relativ unleserlich und inkonsistent.

Nimm dir einfach mal eine Tageszeitung oder ein Buch und schau wie dort die
Elemente genutzt werden die es auch in der Programmiersprache deiner Wahl
gibt. In Sachen lesbarkeit hat die Typographie den Programmierern viel voraus.

Binäre Operatoren:
Sollte vorn und hinten von WS umgeben sein.

Runde Klammern:
1. Kein WS nach der öffnenden und kein WS vor der schließenden Klammer.
2. Bei if, else und co. sollte ein WS vor die öffnende Klammer, bei Funktionen
mache ichs nicht aber fänd ich auch ok.

Kommas:
Ein WS nach dem Komma.

Doppelpunkt:
Relativ egal. Aber wenn WS dann nach dem Doppelpunkt.

Newlines:
1. Man kann Code durch 'Absätze' strukturieren. Es ist aber die Frage ob man den Teil dann nicht gleich in eine Methode auslagern sollte.
2. Vor und nach einer Klassendefinition gehört WS.
3. Manchmal mache ich vor dem 'return' einen Absatz, so als abschließendes
Fazit.

Geschweifte Klammern:
Wenn du schon Befehl und die { schon in eine Zeile quetscht dann solltest
du ein WS davor machen.

if:
If's sollten nie nur aus eine Zeile bestehen. Bestenfalls immer mit Klammern ausschreiben.

interfaces:
Das hin und her mit den Namen ist imho das größte Manko an Interfaces.
I Präfix und Impl Postfix finde ich auch sehr unschön. Das I hat den Vorteil
das man sofort sieht wo ein Interface genutzt oder definiert ist. Das Impl
kommt dagegen seltener vor und man kann 'schöne' Interface Namen wählen.

funktionssignatur:
Brecht ihr lange Zeilen um? Ich machs selten, da es immer recht hässlich aussieht.

[atothek]

Cool nen Refactoring Thread .
Bisher sind ja schon einige anregungen gefallen,

- Methode extrahieren
- Methode umbenennen
- Methode verschieben

vielleicht
- Klasse extrahieren

den hinweis von patrick finde ich gut, typografie in programmierung,
wobei ich einige sachen eher anders sehe. Ich finde WS in ( ) klammern sehr leserlich.

if(variable1 || variable2) { //... }

oder

if( variable1 || variable2 ) { // ... }

Brecht ihr lange Zeilen um? Ich machs selten, da es immer recht hässlich aussieht.

in manchen Sitiationen erzeuge ich mir eine eigen methode dafür mit einigen mehr conditions, allerdings deuten zuviel conditions auf einen Designfehler hin

private function check( Condition1 ) : Boolean {
return Condition1 || ( Condition2 == MyClass.constant && !Condition1 );
}

mfg
alex

[bokel]

@elias: Ich bin schon relativ konsistent mit WS. Ich finde z.B. "if( test)" besser als "if (test)" weil mich die Klammer beim Lesen am Anfang eines Wort eher stört als am Ende. Edit: Allerdings sieht es hier im Browser genau andersherum besser aus, als in meinem Editor.

Aber Whitespace ist sowieso meine geringste Sorge, das ist auch viel Geschmackssache. Mein Hauptinteresse liegt darin, das der Sinn des Programms klar zutage tritt. Also im vorliegenden Fall die drei Schritte

1. Magazin mit Bildern laden
2. Magazin in Projektor schieben
3. Automatische Projektion starten

Deswegen meinte ich auch, dass mich das ganze drumherum mit public static main usw. schon stört. Hier in dieser Klasse geht es ja sogar noch, aber in den unteren Klassen gibt es noch viel mehr uninteressante Details zu sehen. Es wäre doch schön, wenn man alles was nicht direkt mit diesen drei Schritten zu tun hat, ausblenden könnte. Sozusagen eine Art semantisches Codefolding, oder eine Möglichkeit ein Programm auf Knopfdruck aus 10000 Meter Höhe zu sehen. Es ist die Frage, ob man das überhaupt mit Code erreichen kann, oder ob es nur mit einem Tool ginge, wenn überhaupt.

Jetzt aber nochmal ganz konkret zum vorliegenden Beispiel. Diese ganze Geschichte mit dem automatischen Weiterschalten des Projektors. Seht ihr das eigentlich als eigenes Objekt, das den Knopf des Projektors drückt, oder ist das eher eine Eigenschaft eines "erweiterten" Projektors?

mfg. r

[atothek]

Ich hab mir die Sachen nun nochmal genauer betrachtet und finde die ProjectorImpl als das paradebeispiel zum refactorisieren. Die bezeichnung sind schlecht und weichen stark vom rest der klassen ab, und ist zudem ziemlich zerfahren bei der übergabe der function an die display methode welche ja auch eher unschön geschrieben ist.

Die namensgebung bei Repository finde ich persönlich eher mißglückt da fände ich SlideList oder SlideCollection eindeutiger, und warum erweitert Rect die Graphics klasse?? es sind beides statische implemtierungen da macht vererbung keinen Sinn

mfg
alex

[bokel]

@atothek:
Repository habe ich von dict.leo.org - das ist die Übersetzung von Magazin, so nennt man das doch bei einem Projektor.
Was genau stört dich an der Projektor Klasse? Oder anders gefragt, wie würdest du sie refaktorisieren?

mfg. r

[elias]

Also bei Kodak heißt es Tray:
http://slideprojector.kodak.com/accessories/trays.shtml



Edit:
Nochmal zu den Interface Namen. Für "SlideImpl" eher:
- SimpleSlide
- SlideObject
- DefaultSlide

[atothek]

also mal auf die schnelle würde ich es so machen
ich finde die bezeichnungen klarer und konsistenter in bezug auf die anderen klassen, und die scopes sind klar und deutlich innerhalb der klasse geblieben

ich verwende mal die Delegate von MM da die jeder hat

PHP-Code:

import mx.utils.Delegate;
class ProjectorImpl implements Projector {
    
    private var repository : Repository = null;
    private var sliding : Boolean = false;
    private var newSlide : Slide;
    private var currentSlide : Slide;
    
    public function loadRepository( repository : Repository ):Void{
        this.repository = repository;
    }
    
    public function isSliding():Boolean{
        return sliding;
    }
    
    public function hasSlide():Boolean{
        return repository.hasSlide();
    }
    
    public function nextSlide():Void{
        sliding = true;
        newSlide = repository.nextSlide();
        
        if( newSlide == null ){
            sliding = false;
        }
        
        display();
    }
    
    private function display():Void{
        currentSlide.prepareHide();
        newSlide.display( Delegate.create( this, onComplete ) );
    }
    
    public function onComplete() : Void {
        currentSlide.hide();
        currentSlide = newSlide;
        sliding = false;
    }
} 


btw. Tray wäre nen klasse klassenname (Schubladeneinsatz)

mfg
alex