TL;DR
Ja, passt soweit.
Einführung
Heute (30. Mai 2020) hat das Konsortium rund um die Corona Warn App den Sourcecode für selbige veröffentlicht. Es wurde sowohl der für Android als auch der für iOS veröffentlicht. Ich werde mich in diesem Code Review ausschließlich mit dem iOS Sourcecode beschäftigen. Zu Android kann ich nichts sagen und werde es folgerichtig nicht tun.
Grundsätzliches
Ich betrachte den Commit b76053d814115bf99bb79905b67106570ec0f6cc in diesem Review. Dinge, die ich hier anmerke können in der aktuellen Version bereits verändert / gefixt sein. Ich bitte das zu berücksichtigen.
Ich begrüße es sehr, dass die App Open Source ist. SAP macht sich damit angreifbar, aber es existiert natürlich auch die Chance, das eigene Können zu demonstrieren. Abgesehen davon schafft diese Offenheit natürlich auch die Vertrauensbasis für einen Einsatz in einer breiten Öffentlichkeit. Mir verschafft es die Möglichkeit, diesen Code Review zu schreiben. Danke dafür.
Die App ist in Swift geschrieben auf Basis von Apples UIKit. Sie enthält kein Flutter oder React native. Das ist eine sehr grundsätzliche Entscheidung der Verantwortlichen und ich halte sie für richtig. (Ohne Begründung, das wäre ein eigener Blogpost)
Interessant auch die Feststellung, dass kein Reactive Framework verwendet wurde (Kein Reactive Cocoa, RxSwift oder Combine). Ob das die Komplexität erhöht oder klein hält wird gerne hitzig diskutiert. Zu dem Thema ließe sich eine ganze Blogpostserie zu schreiben, deshalb lasse ich das mal als reine Feststellung so stehen.
Architektur
„One of the biggest traps for an engineer is to build a thing that shouldn’t even be there“
Elon Musk
Ich will an dieser Stelle die Architekturmodelle nicht erklären, die hinter den Abkürzungsmonstern wie MVC, MVP, MVVM, VIPER, VIP, aber auch Bezeichnungen wie Clean Swift, Flux, Redux, Unidirectional Flow, etc stehen. Das übersteigt definitiv den Umfang dieses Code Reviews (eigene Blogpostserien, wissen schon). Ich werde diese Begriffe teilweise trotzdem in diesem Abschnitt verwenden. Dem/der interessierten Leser/in empfehle ich diese Begriffe mal zu googlen.
Das erste was bei der Architektur auffällt, ist das verschiedene Screens in verschieden komplexen Architekturmodellen implementiert sind. Das könnte man natürlich als Chaos abtun, aber tatsächlich ist eine gewisse Methode erkennbar: Je komplexer der Screen desto komplexer die Architektur. Auch wenn im Detail einige Entscheidungen diskutierbar sind, ist das grundsätzlich positiv zu werten. Es hat keinen Sinn eine komplexe Architektur wie VIPER oder Clean Swift auf einen Screen los zu lassen, der im Grunde nicht viel tut.
Ein Beispiel für einen sehr einfachen View wäre der „FriendsInviteController“. Der ist in klassischen MVC geschrieben und könnte so direkt aus Apple Beispielcode kommen. Mit allen seinen Problemen zum Thema Testability. (dazu später mehr)
Andere Views sind in einem merkwürdigen Zwischending zwischen MVP und MVVM geschrieben. Da merkt man sehr deutlich, dass versucht wird ein View Model zu bauen, dass den View State hält, gleichzeitig gibt es dann aber keinen Bindingmechanismus, der MVVM erst so elegant macht. Man arbeitet da mit Protokollen, was für mich eher ein typisches Element von MVP ist.
Beim „HomeViewController“ sind wir schon fast bei VIPER.
Einige Screens erben vom „DynamicTableViewController“. Der wirkt massiv overengineert und ich werde das Gefühl nicht los, dass dieser im Zweifelsfall recht schwierig zu debuggen ist. Wahrscheinlich hatte den SAP noch irgendwo herumliegen und hat ihn hier verwendet, obwohl man da Spatzen bombardiert.
Auffällig ist aber das grundsätzliche Fehlen eines Routers. ViewController werden immer in ViewControllern erzeugt. Dadurch ist die Navigation innerhalb der App nicht oder wenn, dann nur umständlich per UITests automatisch testbar.
Das Konzept „Unidirectional Flow“ wird nicht verwendet.
Network Layer
Der Network Layer verwendet ausschließlich Protocol Buffers als Transportdatenformat. Ich gehe davon aus, dass das daran liegt, dass Apple und Google sich bei ihrem Exposure Notification Standard eben auf diese von Google entwickelten Technologie geeinigt haben.
Der Network Layer hat als Interface ein Protocol (Client) und lässt sich somit gut mocken, wenn man Teile der App automatisiert testen möchte. Das ist an sich Standard aber ich wollte es nicht unerwähnt lassen.
Typischerweise besteht so ein Netzwerklayer aus zwei Teilen. Der eine Teil spricht mit dem Server der andere Teil macht aus den empfangenen Rohdaten die Objekte die der Rest der App dann benutzt.
Seit Apple die Klassen rund um URLSession eingeführt hat, gibt es meistens bei dem Code der sich mit der reinen Serverkommunikation beschäftigt wenig zu sehen. So auch hier. Einzig bei der Fehlerbehandlung ist noch Luft nach oben. Bei einigen Calls werden Fehlerzustände großzügig verschluckt. Auch lässt sich anmerken, dass die Anzahl der verwendeten Completion Blocks nicht weiter steigen sollte, sonst wird es unleserlich.
Der andere Aufgabenbereich wird komplett von dem generierten Protocol Buffers Code in Zusammenarbeit mit der SwiftProtoBuf Library erledigt.
Data Layer
Die vom Server heruntergeladenen Exposure Daten werden lokal in einer sqlite Datenbank gespeichert. Es gibt noch eine In-Memory Datenbank, aber die wird augenscheinlich nur zum automatisierten Testen verwendet.
Dependency Injection
Dependency Injection findet weitestgehend nicht statt. Das ist eins der größten Probleme, die ich mit dieser Codebase habe. Siehe auch Testing.
Testing
Der Fairness halber muss ich an dieser Stelle darauf hinweisen, dass die Codebasis zum Zeitpunkt des Verfassens 5 Wochen alt ist und dass alles was ich in diesem Absatz kritisiere unter dem Zeitaspekt absolut tolerabel ist. In einem halben Jahr mögen diese Kritikpunkt aber bitte Geschichte sein.
Los geht’s: Das Thema Testing ist in der App sehr schwach ausgeprägt. Die Achitektur einer App (siehe oben) hat ganz massiv Einfluss darauf wie gut oder schlecht eine App testbar ist. Zwei Dinge spielen da eine wesentliche Rolle. Als erstes muss die gesamte Entscheidungslogik aus Klassen heraussepariert werden, die eine Abhängigkeit zu UIKit haben. Andernfalls tritt man sich den ganzen Rattenschwanz an Apple-Klassen ein, die man für einen Test miterzeugen muss. Zweitens muss man es ermöglichen alle Abhängigkeiten per Dependency Injection der zu testenden Klassen zu übergeben um Mockobjekte einsetzen zu können.
Für den Network Layer und den Data Layer ist das sehr gut gelungen. Für den Rest der App sieht es aber bitter aus.
Zwei kleinere Niggeligkeiten habe ich außerdem noch: Erstens, diese „__tests__“ Ordner sind weird! Das habe ich noch nie vorher gesehen. Gibt es eine Programmierumgebung wo das üblich ist? Zweitens, die vorhandenen Tests sind schwer zu lesen. Mit der inzwischen allseits üblichen Gliederung in „given – when – then“ könnte man da mit wenig Aufwand viel erreichen.
Syntax und Naming
Hier gibt’s nicht viel zu sagen. Die Codebase hält sich im Großen und Ganzen an Apples API Design Guidelines (https://swift.org/documentation/api-design-guidelines/) und das ist auch gut so.
Projekt und Build System
Auch hier gibt es nicht viel zu sagen, weil auch hier die üblichen Standards benutzt werden. Hervorzuheben ist die Verwendung von Swiftlint. Swiftlint prüft grundlegende Syntax und Formatierungsdinge und sollte in keinem Projekt fehlen. Und die derzeit bestehenden 42 Warnung werden bestimmt noch entfernt.
Dinge die mir sonst so auffielen
An einigen Stellen wird das Klassen-Prefix „ENA“ verwendet. Auch in der BundleID findet sich das wieder. Ich vermute mal, das steht für „Exposure Notification App“ quasi der ursprüngliche (Code)Name.
In der Info.plist Datei wurden die App Transport Security Settings auf „Allow arbitrary Loads“ gesetzt. Das sollte vor dem Release dringend geändert werden.
Die Assets wirken noch recht temporär.
Zusammenfassung
Bei aller Kritik, für 5 Wochen ist diese Codebasis beeindruckend. Es sind keine groben Schnitzer gemacht worden und der Allgemeinzustand lässt sich sehen. Ich rechne auch nicht mit Sicherheitsproblemen, obwohl ich natürlich dafür die Hand nicht ins Feuer lege. Es gibt keine Hinweise auf eine Backdoor oder Datenausleitung. Will heißen: Ich werde die App sofort nach Veröffentlichung installieren.
