Mittwoch, 7. Dezember 2011

BIFF - eine Übung im Refaktorisieren

Laut Statistik sind die Beiträge meines Blogs, die von Excel und XML handeln, besonders beliebt - neben dem Dauerbrenner Refactoring in ABAP. Das gibt mir die Rechtfertigung, mich hier erneut über ein Excel-Format auszulassen, und obendrein noch über das mittlerweile überholte Binary Interchange File Format (BIFF), das den Excel-Versionen bis einschliesslich Excel 2007 zugrundelag.

Dieses auf binären Records basierende Format, das von der Apache POI Gemeinde als Horrible Spreadsheet Format (HSSF) geführt wird, hat Microsoft selbst mit dem neuen Format Open XML (das den Dateien mit der Endung .xlsx zugrundeliegt) hinter sich gelassen.[1]

Dennoch gibt es immer noch User, die mit dem alten xls-Format arbeiten. Wenn die Daten aus solchen Excel-Sheets im SAP-System weiterverarbeitet werden sollen, verlangt man üblicherweise, dass der Benutzer sie als csv-Datei abspeichert, um sie danach als Textdatei mit einem CSV-Parser einlesen zu können - wie ich es im Blog Parser für komma-separierte Daten beschrieben habe.

Wenn dies nicht möglich ist, könnte man eine binäre Excel-Datei in ABAP mit den Objekten der OLE Automation einlesen und bearbeiten. In diesem Fall wäre das Format sogar egal: Ein OLE-Objekt spricht die Daten des darunterliegenden Spreadsheets über Zeilen-, Spalten- und Arbeitsblattnummer an, ohne dass der Aufrufer sich um die zugrundeliegende Datenstruktur kümmern muss.

Einziges Problem: Die OLE Automation erfordert einen SAP GUI. Damit ist sie für Hintergrundjobs, RFC-Bausteine oder Webanwendungen nicht geeignet.

Um zu sehen, ob es prinzipiell möglich ist, das native Excel-Format einer xls-Datei in ABAP direkt einzulesen, hatte ich im Jahre 2002 einen Baustein Z_EXCEL_TO_INTERNAL_TABLE geschrieben, der aber seitdem nie das Licht eines produktiven Einsatzes gesehen hat. Das Experiment war erfolgreich ausgegangen: Der Baustein war tatsächlich in der Lage gewesen, die wesentlichen Zellinhalte eines Excel-Files - Zahlen und Strings - in eine ABAP-Datenstruktur einzulesen.

Manche Dinge brauchen einfach ihre Zeit: Nach fast zehn Jahren wird dieser Baustein nun in einem produktiven Kontext benötigt: für die Eingangsbearbeitung von Dokumenten in einem Szenario, in dem den Benutzern keine Vorgaben über das verwendete Format gemacht werden können und Datenfiles sowohl im neuen (xlsx) als auch im alten (xls) Excel-Format eingehen.

Für das neue Format verwenden wir die von Ivan Femia und Gregor Wolf entwickelte Addon-Komponente abap2xlsx, die keine Wünsche offen lässt. Da auch das alte Format in der im Hintergrund laufenden Eingangsverarbeitung benötigt wird, war dies die Möglichkeit, den Baustein, der nun eine Dekade im Repository gelagert hatte, in kleinen Schritten zu verbessern: Ihn ein bisschen in Schuss zu bringen und zu entwanzen.

Programmiergewohnheiten ändern sich im Laufe der Jahre, und wenn es auch von Zeit zu Zeit nur wenige Änderungen zu sein scheinen, zeigen sich doch beim Blick auf zehn Jahre alten Code beträchtliche Unterschiede im Stil.

Die Namenskonventionen des CRM mit ihrem zweibuchstabigen Präfix verwendete ich allerdings schon damals. Und doch stimme ich mit Horst Keller [2] wie auch Robert C. Martin [3] überein, dass es sich bei diesen Präfix-Namenskonventionen nur um Krücken handelt und dass Variablennamen noch besser lesbar sind, wenn sie durch überhaupt kein Präfix verschmutzt werden. Aber präfixfreie Variablennamen können wir uns in ABAP erst leisten, wenn es in der ABAP-Programmierergemeinde zur Gewohnheit geworden ist, wirklich kurze Methoden zu schreiben, d.h.: Nicht mehr hundert-, sondern zehnzeilige Methoden. Davon sind wir de facto noch entfernt. Solange das so ist, haben die "ungarische" und verwandte Konventionen noch ihre Existenzberechtigung: In einer Methode, die man nicht ganz auf dem Bildschirm zu sehen bekommt (also inclusive ihrer lokalen data-Definitionen), ist es nützlich, wenn man jeder Variablen sofort ihren Gültigkeitsbereich ansieht.

Natürlich würde ich heute für eine solche Aufgabe eine Klasse schreiben, keine Funktionsgruppe. Auch waren meine Funktionen damals viel länger als ich es mir heute erlauben würde. Insgesamt enthält diese Funktionsgruppe für das, was sie leistet, viel zu wenig Modularisierungseinheiten, d.h. vor allem zu wenig Unterprogramme.

In diesem speziellen Fall hatte ich mich aus Performancegründen obendrein zu einer intensiven Verwendung von Macros entschlossen: Der Stream eines einzelnen Worksheets ist im BIFF-Format ein langer binärer String, auf den ich immer wieder mit Offset und Länge zugreifen musste, um einzelne Bytes, Doppelbytes oder Vier-Byte-Wörter zu lesen. Um nicht für jedes einzelne Byte, das ich auf diese Weise lese, einen Unterprogrammaufruf machen zu müssen, hatte ich ein Macro _get_data mit zwei Parametern entworfen: Der Anzahl der zu lesenden Bytes und der Zielvariablen (ein Feld vom Typ x oder ein xstring), in die die Bytes hineingestellt werden sollen.

Nun ist es so, dass die Folge der zu lesenden Bytes im BIFF-Format an beliebiger Stelle durch das Ende des aktuellen Records unterbrochen werden kann. Es folgt dann meistens ein zweibytiger CONTINUE-Code und die Länge dieses CONTINUE-Records, und erst dann geht es weiter mit der Bytefolge. Um diesen Fall abzuhandeln, hatte ich in das Macro schrecklich viel Logik hineingepackt. Ich wundere mich nachträglich, wie ich diesen Code, ohne ihn debuggen zu können, verfasst haben kann [4]:

*---------------------------------------------------------------------*
* Macro zum Lesen von Daten aus dem Stream
*---------------------------------------------------------------------*
define _get_data.
* Berechnen des Offsets nach erfolgtem Lesen
lv_pos = gv_pos + &1.
* Zu gross? Fehler
if lv_pos > gv_length.
message 'Fehler: Speicherüberschreitung bei get data (1)' type 'X'.
endif.

if lv_next_key ne gc_record-continue or
lv_pos < lv_next_pos.
*---------------------------------------------------------------------*
* Fall 1: Zu lesendes Datum liegt vollständig im aktuellen Record
*---------------------------------------------------------------------*
* Feld einlesen
&2 = <gv_data>+gv_pos(&1).
* Globalen Positionszeiger vorsetzen
add &1 to gv_pos.
else.
* Abstand bis zum Offset des CONTINUE Key:
lv_m_dist = lv_next_pos - gv_pos.
*---------------------------------------------------------------------*
* Fall 2: Zu lesendes Datum muss einen Continue-Record überspringen
*---------------------------------------------------------------------*
if &1 > lv_m_dist.
* Erster Teil des Feldes kann eingelesen werden:
&2 = <gv_data>+gv_pos(lv_m_dist).
* Jetzt Länge des CONTINUE Records ermitteln (little Endian Format!)
lv_pos = lv_next_pos + 2.
lv_m_xlength+1 = <gv_data>+lv_pos(1).
add 1 to lv_pos.
lv_m_xlength(1) = <gv_data>+lv_pos(1).
* lv_pos auf erstes Byte des CONTINUE Records setzen:
add 1 to lv_pos.
* Neue lv_next_pos = Länge des CONTINUE Records + 4 + Offset von CONTINUE
* = Länge des CONTINUE Records + lv_pos
lv_next_pos = lv_m_xlength + lv_pos.
* Der auf CONTINUE folgende nächste Key muss ermittelt werden:
lv_length = lv_next_pos + 2.
if lv_length <= gv_length.
lv_next_key = <gv_data>+lv_next_pos(2).
concatenate lv_next_key+1(1) lv_next_key(1) into lv_next_key in byte mode.
else.
* Keine Daten mehr -> Schleife ist beendet
exit.
endif.
* Verbleibende, zu lesende Zeichenzahl ermitteln:
lv_m_length = &1 - lv_m_dist.
* Reststring lesen und an &2 anhängen
if lv_m_dist > 0.
concatenate &2 <gv_data>+lv_pos(lv_m_length) into &2 in byte mode.
else.
&2 = <gv_data>+lv_pos(lv_m_length).
endif.
* Globalen Positionszeiger vorsetzen
gv_pos = lv_pos + lv_m_length.
else.
* Jetzt Länge des CONTINUE Records ermitteln (little Endian Format!)
lv_pos = lv_next_pos + 2.
lv_m_xlength+1 = <gv_data>+lv_pos(1).
add 1 to lv_pos.
lv_m_xlength(1) = <gv_data>+lv_pos(1).
* lv_pos auf erstes Byte des CONTINUE Records setzen:
add 1 to lv_pos.
* Neue lv_next_pos = Länge des CONTINUE Records + 4 + Offset von CONTINUE
* = Länge des CONTINUE Records + lv_pos
lv_next_pos = lv_m_xlength + lv_pos.
* Der auf CONTINUE folgende nächste Key muss ermittelt werden:
lv_length = lv_next_pos + 2.
if lv_length <= gv_length.
lv_next_key = <gv_data>+lv_next_pos(2).
concatenate lv_next_key+1(1) lv_next_key(1) into lv_next_key in byte mode.
else.
* Keine Daten mehr -> Schleife ist beendet
exit.
endif.
* String lesen und an &2 anhängen
&2 = <gv_data>+lv_pos(&1).
* Globalen Positionszeiger vorsetzen
gv_pos = lv_pos + &1.
endif.
endif.
end-of-definition.


Der Code ist nicht falsch, aber trotzdem scheusslich und für das, was er tun soll, zu lang. Ein Beispiel: Wenn man sich die Bedingung des inneren if genau anschaut, so folgt sie bereits aus der Bedingung des umgebenden else-Zweigs. Das bedeutet, der innere else-Zweig kann nie durchlaufen werden und kann somit gelöscht werden.

Ich hatte nun die Möglichkeit, das Macro komplett durch ein Unterprogramm zu ersetzen. Um die ursprüngliche Intention (die Performance-Bedenken) zu berücksichtigen, habe ich das Macro aber so stehengelassen, dass wenigstens der Hauptfall weiterhin ohne Unterprogramm ausgeführt werden kann. Nur den eher weniger häufigen Fall, dass beim Einlesen von Bytes ein CONTINUE-Record übersprungen werden muss, habe ich in ein Unterprogramm ausgelagert. Das Macro sieht neu wie folgt aus:

  define _get_data.

_compute_offset_by &1 lv_pos.

if lv_next_key ne gc_record-continue or
lv_pos <= lv_next_pos.

*---------------------------------------------------------------------*
* Fall 1: Zu lesendes Datum liegt vollständig im aktuellen Record
*---------------------------------------------------------------------*
* Feld einlesen
&2 = <gv_data>+gv_pos(&1).
* Globalen Positionszeiger vorsetzen
add &1 to gv_pos.

else.

*---------------------------------------------------------------------*
* Fall 2: Zu lesendes Datum muss einen Continue-Record überspringen
*---------------------------------------------------------------------*
perform read_over_continue using &1 space
changing &2
lv_m_options
lv_next_pos
lv_next_key.
endif.
end-of-definition.

Das ist vor allem viel weniger Code als früher: So wenig, dass man sich trauen kann, das _get_data im Debugger mit einem Schritt auszuführen und das Ergebnis gerade noch mit blossem Auge nachvollziehen kann - zumindest wenn es nicht in den else-Zweig geht, aber in den könnte man mittels Einzelschritt (F5) hineindebuggen, da er ja in einem Unterprogramm ausgeführt wird.

Die anderen Macros wie _get_number zum Einlesen von einzelnen Records habe ich vollständig durch Unterprogramme ersetzt.

Mithilfe von Unit Tests konnte ich darüberhinaus einige Bugs aufdecken, die im Code noch schlummerten. Der erste einfache Test funktionierte noch so, dass ich ein komplettes File im Binärcode einlas:
  method test_simple.

data: lv_xls type xstring,
ls_xls type zexcel,
ls_exp type zexcel.

* Einfaches XLS-Worksheet im Binärformat aufbauen
* | abc | 123 | xyz |
* | 456 | pqr | 789 |
perform get_simple_xls in program zutil_excel_testdata changing lv_xls.

* ... und dem Funktionsbaustein vorlegen
call function 'Z_EXCEL_TO_INTERNAL_TABLE'
exporting
iv_file_as_stream = lv_xls
importing
es_excel = ls_xls.


* Testerwartung
_insert_row_n_fields ls_exp-cells 'row;col;typ;ref' :
'0001;0001;S;00000001',
'0001;0002;F;00000001',
'0001;0003;S;00000002',
'0002;0001;F;00000002',
'0002;0002;S;00000003',
'0002;0003;F;00000003'.

define _append.
append &2 to &1.
end-of-definition.

_append ls_exp-strings:
'abc',
'xyz',
'pqr'.

_append ls_exp-float:
123,
456,
789.

cl_aunit_assert=>assert_equals( act = ls_xls
exp = ls_exp ).

endmethod. "test_simple

Um die Load der Funktionsgruppe nicht unnötig zu vergrössen, habe ich dabei die Herstellung des xls-Dokuments in ein externes Unterprogramm ausgelagert:

program zutil_excel_testdata.

form get_simple_xls changing cv_xls type xstring.

data: lv_line type xstring.

define _add_line.
lv_line = &1.
concatenate cv_xls lv_line into cv_xls in byte mode.
end-of-definition.

* Binäres Bild einer Excel-Datei mit folgendem Inhalt:
* | abc | 123 | xyz |
* | 456 | pqr | 789 |

_add_line:
'D0CF11E0A1B11AE100000...0FEFFFFFF00000000FEFF',
'FFFF000000001800000FF...FFFFFFFFFFFFFFFFFFFFF',
...
endform.

Würde ich diesen totalen Weg über den Funktionsbaustein für alle Testfälle wählen, die mir einfallen, so wäre die Gesamt-Ausführungszeit der Unit Tests zu hoch. Ich entschied mich daher, für die Teile des Codes, die die einzelnen Records auswerten, isolierte Tests zu schreiben.

Hier zum Beispiel ist eine Methode, die das Einlesen einer im Format RK_NUMBER codierten Ganzzahl testet:
  method test_get_rk_i.

data: ls_excel type zexcel,
lv_int type i.

get_rk_number( exporting iv_data = '3A85F102'
changing cs_excel = ls_excel ).

read table ls_excel-int into lv_int index 1.
assert_subrc( act = sy-subrc
msg = 'Die Ganzzahl wurde nicht erkannt' ).

assert_equals( act = lv_int
exp = 12345678
msg = 'Ganzzahl wurde nicht korrekt gelesen' ).

endmethod. "test_get_rk_i

Die RK_NUMBER-Tests laufen immer über die Hilfsmethode get_rk_number der Testklasse. Für die anderen Recordtypen gibt es ähnliche Hilfsmethoden. Sie stellen die benötigten globalen Daten - das Feldsymbol <gv_data> mit dem Stream, sowie gv_length mit dessen Länge, bereit.
  method get_rk_number.

data: lv_next_pos type i,
lv_next_key type recordkey,
ls_cell type zexcelcell.

assign iv_data to <gv_data>.
gv_length = xstrlen( <gv_data> ).
gv_pos = 0.

perform get_rk_number using ls_cell
changing lv_next_pos lv_next_key cs_excel.

endmethod. "get_rk_number

Mit solchen kleinen Testmethoden konnte ich Probleme isolieren, die beim Einlesen von grösseren Excel-Dateien auftraten. Dann konnte ich den Code so lange ändern, bis auch der neue Test auf Grün ging. Wenn ich die Suite aktuell im Unit Test Browser mit Abdeckungsmesung laufen lasse, komme ich auf eine Prozedurenabdeckung von 85%. Für einzelne Routinen ist es klar, dass sie nicht durch Modultests abgedeckt werden können, z.B. read_file: Hier wird eine Datei aus dem Filesystem im Binärformat eingelesen und in einen xstring gestellt. Mehr als den Aufruf der Methode gui_upload() von cl_gui_frontend_services sowie dem Fehlerhandling enthält diese Routine aber auch nicht. Das ist also eine Lücke, die man lassen kann. Andere Stellen, die nicht abgedeckt sind, betreffen Grenzfälle: Nicht alle Parametrisierungen, mit denen der Baustein theoretisch aufgerufen würde, auch wenn sie zu einem Fehler führten, werden in einem Test durchgespielt. Wichtiger erschienen mir bei diesen nachträglich hinzugefügten Unit Tests die Kernfunktionen: Das Einlesen der verschiedenen Recordformate für Zahlen und Strings.

Natürlich bin ich noch lange nicht zufrieden mit dem Ergebnis der Refaktorisierungen. Man könnte die Sache noch weitertreiben. Da ich nur begrenzte Kapazitäten dafür aufwenden kann, muss aber irgendwo ein Punkt gemacht werden. Ich habe den Campingplatz sicher sauberer hinterlassen als ich ihn diesmal vorgefunden habe. Vielleicht gibt es ja ein nächstes Mal, um weitere Verbesserungen einzubringen.

Sehr hilfreich bei der Analyse des Formats und bei der Erstellung von Beispielen war mir übrigens ein altes, einfaches Programm namens biffview.exe, das den Stream eines Worksheets analysiert und die einzelnen Bestandteile hexadezimal ausgibt - mit Verweis auf die Spezifikation des jeweiligen Recordtyps.

[1] Was das neuere OpenXML-Format angeht, so ist die Konvertierung von und nach ABAP mit der sehr komfortablen Softwarekomponente abap2xlsx gelöst, die eine bemerkenswert grosse Teilmenge des Excel-Feature-Sets unterstützt.
[2] Horst Keller, Wolf Hagen Thümmel: ABAP-Programmierichtlinien, Galileo Press, 2009
[3] Robert C. Martin: Clean Code: A Handbook of Agile Software Craftsmanship, Prentice Hall International, 2008.
[4] Es ist allerdings ein Mythos zu glauben, Macros liessen sich grundsätzlich nicht debuggen. Das Problem für den Debugger besteht ja nur darin, dass er den ausgeführten Anweisungen keine laufende Quelltextzeile zuordnen kann. Wenn man den Macro-Quelltext in eine Include-Datei auslagert - etwa so:
define _langes_macro.
include z_langes_macro_corpus.
end-of-definition.

dann werden die einzelnen Anweisungen im Debugger ganz normal erreichbar. Das funktioniert allerdings nur für parameterlose Macros, denn die Symbole &1 etc. können vom Compiler nur im Gültigkeitsbereich einer Datei aufgelöst werden.

Keine Kommentare :