Schreibt der Entwickler von Pulse Audio und systemd wirklich so einen Kotzcode und das im Jahr 2018?



  • Zugegeben, ich habe mir nie den Code von systemd und Pulse Audio angesehen, aber bin darauf in folgende Diskussion gestoßen und was soll ich sagen, der Mann der hier seine Meinung zum Code sagt, dem gebe ich (mit Ausnahme des Punktes wann Variablen deklariert gehören) recht.

    Aber guckt einfach selbst:

    https://www.heise.de/forum/c-t/Kommentare-zu-c-t-Artikeln/Ubuntu-18-04-LTS-Ubuntu-Desktop-statt-Unity-Kernel-Livepatching/Re-und-jetzt-bitte-noch-init-statt-systemD/posting-32275856/show/

    Um dieses Prachtstück geht es:
    https://github.com/systemd/casync/blob/master/src/cautil.c

    Wenn so der Code von systemd und pulse Audio aussieht, dann bin ich echt geschockt.



  • Naja, andere kochen auch nur mit Wasser.
    Insbesondere open source Devs.
    Und systemd stammt noch aus einer Zeit und einer Szene wo möglichst kryptische C-Hackerkultur noch 1337 war.
    Ich würde heute sowas auch nicht mehr bei ner Peer Review durchgehen lassen.
    Aber jetzt deswegen alles umschreiben obwohl es funktioniert? Wohl nicht.



  • Also ich finde den Code weder besonders gut noch besonders schlecht. Und zustimmen kann ich der Kritik nur teilweise, einiges sehe ich da ganz anders.

    Zeilen 1 bis 3:
    Gratulation zu diesen aussagekraeftigen Variablennamen, und zu gleich mehreren davon auf einer Zeile.
    Dass sie schon deklariert sind, wo sie weder einen sinnvollen Wert haben noch bereits gebraucht werden, ist eine weitere Schwaeche (war in C89 mehr oder weniger notwendig, aber es ist 2018).

    Sehe ich genau so.

    Zeilen 14 und 19:
    "goto unescape", genau. Weil ihm gerade nicht eingefallen ist, dass man "if" verschachteln kann und wofuer "else" dient

    Ja, goto ist hier unnötig. Nur bitte keine if-Kaskade als Alternative vorschlagen. => return unescape(e);

    Zeile 34:
    f[2] kann out-of-bounds sein

    Muss er mir erstmal erklären wie das gehen soll. f[2] wird nur ausgewertet wenn beide Bedingungen davor erfüllt waren. Ich gehe weiters davon aus dass unhexchar(0) einen Wert < 0 zurückgeben wird. D.h. im einzigen Fall wo es verboten wäre f[2] anzugucken, nämlich wenn f[1] die abschliessende Null ist, wird f[2] nicht angeguckt. Also wo soll das Problem sein?

    Zeile 36:
    Noch mehr Operationen auf einmal haben links und rechts vom "=" nicht hingepasst, nichtwahr?

    Für low-level String-gefummel ganz normal. Klar, kann man vermutlich schöner schreiben, aber furchtbar ist ganz was anderes.

    Zeile 37:
    Die Variable zu veraendern, die in der expression des "for" statements steht, wir potentiell den naechsten Maintainer freuen, der das uebersieht und sich wundert, warum manche der Werte nicht daherkommen

    In diesem speziellen Fall würde ich sagen: das ist genau die Art und Weise wie man es einfach schreibt. Wenn der nächste Maintainer mit sowas zu überraschen ist, dann ... hat er einfach noch viel dazuzulernen.

    Zeile 38:
    Wer braucht schon geschickt strukturierten control flow, wenn man auch einfach "goto" und "continue" nehmen kann...

    Was goto angeht, ja, auf goto sollte man unbedingt verzichten.
    Was continue angeht: in übersichtlichen Schleifen ist continue ... OK. Genau so wie "early return" kann "early continue" die Übersichtlichkeit von Code erhöhen. Trifft auf diesen Fall IMO nicht unbedingt zu, hier wäre wohl eher einfach ein else Zweig angebracht. Kann man aber so oder so sehen.

    Dazu kommen noch querbeet verteile "return"-Statements, mehrere Operationen pro Zeile, auch mitten in Conditions, generell schlecht strukturierter control flow, ein schlechter/schlampiger coding style...

    Oh jeh, das klint nach wiedermal jemand den sie an der Uni/HF/HTL versaut haben. Single-Entry/Single-Exit und so Blödsinn.
    Nein, mehrere return Statements sind überhaupt kein schlechter Stil.
    Was schlechter Stil ist, ist Code künstlich unklar/unleserlich zu machen indem man zwanghaft darauf verzichtet.


Anmelden zum Antworten