Donnerstag, 2. Juni 2011

 

Programmierrichtlinien haben doch Sinn

Eine Stunde Fehlersuche! Suche in einem fremden Code, der gewisse Anforderungen erfüllen muss. Der Unit-Test für eine Methode, die einen CSV-String liefern ist fehlgeschlagen. Das Feld an der Position 5 war immer leer. Der Test erwartete den String "42995";"47.01708,16.93225";"20.73708,29.96312";" 196";"201105132101";" 405";"9634090160";"O", die Methode lieferte jedoch 42995;"47.01708,16.93225";"20.73708,29.96312"; 196;201105132101;"";9634090160;"O"

OK, dieser "Fehler" war schnell gefunden. Die Methode hielt sich besser an das CSV-Format (nur Strings sind in Hochkomma eingeschlossen, Zahlen nicht) als mein Test. Das war schnell korrigiert und nun lieferte die Methode alles als String:
"42995";"47.01708,16.93225";"20.73708,29.96312";" 196";"201105132101";"";"9634090160";"O"

Fast richtig. Das ist ein wirklicher Fehler. Durchforsten und Debuggen hat ergeben, dass dieses Feld schon beim Lesen immer leer wird. Der Code dazu ist folgender (das Leerzeichen bei den Bedingungen "kleiner als" habe ich für den Blog hinzugefügt, damit da nicht ein ungültiges HTML-Tag ensteht, grundsätzlich erschwert das Fehlen solcher Leerzeichen das Lesen des Codes):

public Data(String fn) throws FileNotFoundException, IOException{
        fileName=fn;

        rf = new RandomAccessFile(fn, "rw");
            int mC=rf.readInt();
            if(mC==4223){
                int dataOffset=0;
                short count=0;

                dataOffset=rf.readInt();
                firstDataOffset=dataOffset;
                count=rf.readShort();
                information = new Header(count);
                for(int i=0;i< count;i++){
                    short fNameLength = rf.readShort();
                    byte[] roughData = new byte[fNameLength];
                    rf.read(roughData/*, (int)rf.getFilePointer(), fNameLength*/);

                    String fieldName = new String(roughData,"ISO-8859-1");
                    short fDescLength = rf.readShort();
                    roughData = new byte[fDescLength];
                    rf.read(roughData/*, (int)rf.getFilePointer(), fDescLength*/);
                    String fieldDescription = new String(roughData,"ISO-8859-1");

                    char type = (char)rf.readByte();

                    short fDataLength = rf.readShort();

                    Field f = new Field(fNameLength, fieldName, fDescLength, fieldDescription, type, fDataLength);

                    information.setHeaderField(i, f);
                }
                records = new ArrayList();
                while(dataOffset< rf.length()){
                    short flag = rf.readShort();
                    String[] data = new String[information.getLength()];
                    for(int i=0;i< data.length;i++){
                        int readLen = information.getField(i).length;
                        byte[] roughData = new byte[readLen];
                        rf.read(roughData/*, dataOffset, readLen*/);
                        switch(information.getField(i).type){
                            case'f':
                            case'F':
                                data[i] = new String(roughData,"ISO-8859-1");
                                break;
                            case'v':
                            case'V':
                                StringBuilder sb = new StringBuilder();
                                for(int j=0;i< readLen && (char)roughData[j]!='\0';j++){
                                    sb.append((char)roughData[j]);
                                }
                                data[i] = sb.toString();
                                break;
                            case'c':
                            case'C':
                                byte[] d = new byte[1];
                                d[0]=roughData[0];
                                data[i] = new String(d);
                                break;
                        }
                        dataOffset+=(2+readLen);
                    }
                    if(flag==0){
                        records.add(new RecordData(data, true, information));
                    }else{
                        records.add(new RecordData(data, false, information));
                    }

                }
                rf.close();
                information.recAnz = records.size();
            }else{
                throw new FileNotFoundException("Keine DB-Datei");
            } 
    }
Soviel konnte ich aufgrund der Rahmenbedingungen auch gleich herausfinden: das Feld mit Index 5 ist ein "V"-Feld, daher ist der Code ab Zeile 48 relevant. Der schaut eigentlich richtig aus. Erstaunlich ist jedoch, dass die "V"-Felder davor korrekt waren, das "F"-Feld auch. Aber das fehlerhafte "V"-Feld ist gleich nach dem ersten "F"-Feld. Daher war meine Hypothese: das "F"-Feld "erzeugt" den Fehler. Aber die Zeilen 44 und 45 sind korrekt.

Also nächste Hypothese: das Lesen der Daten ist fehlerhaft! Zeilen 38 bis 40. Aber die sind m.E. korrekt. Vielleicht wurde der Dateiheader falsch gelesen (dort sind die Feldlängen und Feldtypen spezifiziert).

Zeilen 5 bis 32. Aber auch dort ist nichts verdächtiges. Es ist schon zum Verzweifeln! Alles schaut richtig aus und trotzdem schlägt der Test fehl (zu Recht, denn das Ergebnis ist falsch).

Jetzt ist einmal Zurücklehnen angesagt. Das ganze mal von der Entfernung betrachten. Breakpoint auf Zeile 48 setzen und schauen, was sich tut.

Erst beim 5. Feld (0-basiert) tritt der Fehler auf. Die Schleife wird sofort verlassen, obwohl roughData tatsächlich mehr als 0 Bytes enthält, 5, um genau zu sein. readLen enthält sogar den richtigen Wert. Wie so zum Teufel ist dann die Bedingung j < readLen && roughData[j] != 0 nicht erfüllt (das Casten auf (char) hatte ich schon entfernt, weil '\0' tatsächlich den Wert 0 hat)?

Ich habe bei dem Ausdruck Leerzeichen eingebaut, die im Original nicht waren. Jetzt fällt es wie Schuppen aus den HaarenAugen! Im Original auf Zeile 49 steht i < roughData && roughData[j] != 0. i statt j - optisch fast nicht zu unterscheiden!

i zählt die Felder, j die einzelnen Bytes in einem Feld. Daher ist ab dem Feld 5, welches eine Länge von 5 hat, der erste Teil der Bedingung nicht mehr erfüllt und es wird ein leerer String erzeugt.

Ja, Programmieranfängern wird immer gepredigt: "sprechende" Namen verwenden!

Das hätte geholfen! Statt i könnte man z.B. fieldno verwenden und statt j wäre byteno angebracht. Damit wäre der Fehler nie passiert! Die Schleife hätte gleich falsch ausgesehen:

for(int byteno=0;fieldno< readLen && roughData[byteno]!=0;byteno++){
    sb.append((char)roughData[byteno]);
}
Wenn dann noch Leerzeichen dazwischen sind, springt der Fehler sofort ins Auge, denn warum wird in der Schleife einmal fieldno und einmal byteno verwendet?

Selbst wenn man in kurzen Schleifen i und j als Laufvariable erlaubt, dann müsste aber in der äußeren Schleife immer noch ein "sprechender" Name verwendet werden. Das stäche auch ins Auge (die Leerzeichen verbessern das auch noch!):

for (int i = 0; fieldno < readLen && roughData[i] != 0; i++){
    sb.append((char)roughData[byteno]);
}
Wenn schon kurze Laufvariable, dann besser nie i und j gemeinsam verwenden sondern "unterschiedlichere" Buchstaben wie i und k oder i und m. Die kann man nicht so leicht verwechseln!

Die Variante mit "sprechenden" Namen für Schleifen, die länger als ein paar Zeilen sind, und kurze Laufvariable für kurze Schleifen (Dreizeiler) ist die beste Möglichkeit, dann das Beispiel mit i und fieldno schaut gleich irgendwie falsch aus. Zumindest schaut man sich so etwas gleich näher an.

Leerzeichen und sprechende Namen bringen's!

Siehe auch Programmierrichtlinien allgemein. Dort steht leider nichts über die Verwendung von Leerzeichen.

Java Guidelines findet man z.B. hier: http://www.oracle.com/technetwork/java/codeconv-138413.html

Google findet auch etwas: www.google.com

Labels: , ,


Kommentare:

Kommentar veröffentlichen

Abonnieren Kommentare zum Post [Atom]





<< Startseite

This page is powered by Blogger. Isn't yours?

Abonnieren Posts [Atom]