Laden...

Code umgeschrieben. Ist das gut?

Erstellt von LittleTester vor einem Jahr Letzter Beitrag vor einem Jahr 1.381 Views
L
LittleTester Themenstarter:in
158 Beiträge seit 2019
vor einem Jahr
Code umgeschrieben. Ist das gut?

Ich habe meinen Code versucht zu optimieren und mir dabei die Nullreferenz-Warnung zunutze gemacht die ja ständig nervt.

Ich würde nun gerne wissen, ob dieser neue Code auch sauber ist oder man das so nicht machen sollte.
Alter Code:


try
{
    var findCitrix = RegistryKey.OpenBaseKey(RegistryHive.LocalMachine, RegistryView.Registry64).OpenSubKey(@"SOFTWARE\WOW6432Node\Microsoft\Windows\CurrentVersion\Uninstall\CitrixOnlinePluginPackWeb")?.GetValue("DisplayVersion") ??
        RegistryKey.OpenBaseKey(RegistryHive.CurrentUser, RegistryView.Registry64).OpenSubKey(@"SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\CitrixOnlinePluginPackWeb")?.GetValue("DisplayVersion");
    if (findCitrix == null)
    {
        softwareCitrixProgram = "Nicht installiert";
        softwareCitrixVersion = "";
    }
    else
    {
        softwareCitrixProgram = "";
        softwareCitrixVersion = findCitrix.ToString();
    }
}
catch (Exception)
{
    MessageBox.Show("Problem im Abschnitt Citrix Workspace.");
}

Neuer Code:


public static string GetCitrixWorkspace()
{
    string CitrixWorkspace = "";
    try
    {
        var findCitrix = RegistryKey.OpenBaseKey(RegistryHive.LocalMachine, RegistryView.Registry64).OpenSubKey(@"SOFTWARE\WOW6432Node\Microsoft\Windows\CurrentVersion\Uninstal\CitrixOnlinePluginPackWeb")?.GetValue("DisplayVersion") ??
            RegistryKey.OpenBaseKey(RegistryHive.CurrentUser, RegistryView.Registry64).OpenSubKey(@"SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\CitrixOnlinPluginPackWeb")?.GetValue("DisplayVersion");
            CitrixWorkspace = findCitrix?.ToString() ?? "Nicht installiert";
    }
    catch (Exception)
    {
        MessageBox.Show("Problem im Abschnitt Citrix Workspace.");
    }
    return CitrixWorkspace;
}

IDE: Visual Studio 2022
Sofern nicht anders genannt basieren meine Projekte auf C# und .net 6

16.806 Beiträge seit 2008
vor einem Jahr

Eher nicht.
Statischer Code hat gewisse Anwendungsfälle. Dieser Fall gehört nicht dazu.

a) weil Du hier Datenquelle mit UI vermischt (Registry -> Windows Forms), verletzt also [Artikel] Drei-Schichten-Architektur
b) weil dieser statische Code nicht mehr wirklich testbar ist => [Artikel] Unit-Tests: Einführung in das Unit-Testing mit VisualStudio

Daher würde ich das nicht als "gut" bezeichnen.

Wie ich das tun würde:


// Edit: das hier ist die zweite Variante, da ich zuvor übersehen hatte, dass RegistryKey auch immer disposed werden sollte
public class RegistryProvider
{
    private RegistryKey OpenLocal64 => RegistryKey.OpenBaseKey(RegistryHive.LocalMachine, RegistryView.Registry64);

    private bool TryGetLocal64DisplayValue(RegistryKey sourceKey, string keyName, out string? value)
    {
        if (sourceKey.OpenSubKey(keyName) is RegistryKey key)
        {
            value = key.GetValue("DisplayVersion")?.ToString();
            key.Dispose();

            return true;
        }

        value = null;
        return false;
    }

    public string? GetCitrixWorkspace()
    {
        using (RegistryKey local64 = OpenLocal64)
        {
            if (TryGetLocal64DisplayValue(local64,
                @"SOFTWARE\WOW6432Node\Microsoft\Windows\CurrentVersion\Uninstal\CitrixOnlinePluginPackWeb", out string? wowKey))
            {
                // hier wenn gewünscht logging
                return wowKey;
            }
            else if (TryGetLocal64DisplayValue(local64,
                @"SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\CitrixOnlinPluginPackWeb", out string? msKey))
            {
                // hier wenn gewünscht logging
                return msKey;
            }
        }

        // hier wenn gewünscht logging
        return null;
    }
}


Begründung:

  • Saubere Abstrahierung und Testbar (durch zusätzliches Interface)
  • Direkte Möglichkeit Logging zu integrieren (wird leider in vielen Architekturen vergessen)
  • Keinerlei UI Logik
  • Exception Handling a) hier nicht notwendig oder eben b) zusätzlich abstrahieren

Und sowas wie


    catch (Exception)
    {
        MessageBox.Show("Problem im Abschnitt Citrix Workspace.");
    }

hilft niemanden.

Das ist im Endeffekt das typische Meme "Es ist ein Fehler aufgetreten" - und niemand wird je erfahren welcher.
Widerspricht absolut dem, wie man mit Exceptions umgehen soll => Exception Handling - C# Programming Guide

L
LittleTester Themenstarter:in
158 Beiträge seit 2019
vor einem Jahr

Danke Abt. Ich muss den Code jetzt erstmal nachvollziehen und verstehen. Geht noch über meien Wissensstand hinaus. Aber jetzt weiß ich in welche Richtung es gehen soll.

Ich habe mich beim coden an diesem Thread orientiert und mich für die Methodenlösung entschieden. War das falsch?

Ich habe den Code übrigens erstmal nicht in Code-Reviews gepostet, weil ich dachte, dass er gegen Regel 1 verstößt, also direkt kompilierbar sein muss. Weils aber nur eine Methode ist dachte ich, dass es hier falsch ist. Sorry.

IDE: Visual Studio 2022
Sofern nicht anders genannt basieren meine Projekte auf C# und .net 6

16.806 Beiträge seit 2008
vor einem Jahr

War das falsch?

Aus dem Thread ging halt nicht wirklich hervor, was Du tun wolltest.
Th69 schreibt ja auch:

Wenn du kein Klassenobjekt benötigst, dann kannst du auch die Methode (sowie die Klasse) statisch machen

Du mußt dir also designtechnisch überlegen, ob du die Werte nur einmalig ermitteln möchtest (und dann die Eigenschaften benutzt) oder aber ob jedesmal beim Aufruf die Werte abgefragt werden sollen. Es kommt dabei auch darauf an, ob du noch weitere Werte abfragen möchtest und du diese dann alle "im Rutsch" abfragst oder aber "lazy" nur die Methoden aufrufst, die in der konkreten Anwendung dann auch benötigt werden (die SystemClass wäre dann also eine allgemeine Utility/Tool-Klasse und könnte in eine eigene Assembly [bzw. NuGet Package] ausgelagert werden).

Sehe das aber in diesem Fall als nicht passend an, dass also statisch der falsche Weg für diesen Fall ist, wenn wir von "gutem Code" sprechen.
Du wirst aber wahrscheinlich nur ein Mini-Tool schreiben, ohne Aspekte wie Testen, Architektur und Co. Du wirst also vom "richtig gutem Code" im Sinne von Prinzipien nichts haben (ausser den Lerneffekt).

Ich habe den Code übrigens erstmal nicht in Code-Reviews gepostet, weil ich dachte, dass er gegen Regel 1 verstößt,

Glaub wir müssen mal die Regeln überarbeiten. Ist ne Quatschregel für sowas (meine pers. Meinung).

L
LittleTester Themenstarter:in
158 Beiträge seit 2019
vor einem Jahr

Also um das richtig zu verstehen: Für ein Mini-Tool ist der Code "OK" (Im Sinne von "Kann man so machen und ist nicht falsch"). Nur der Exception-Block ist Müll.

Für größere Projekte ist der Ansatz aber nicht gut und ich sollte auf die von dir vorgeschlagene Variante umschwenken und noch mehr lernen.

IDE: Visual Studio 2022
Sofern nicht anders genannt basieren meine Projekte auf C# und .net 6

16.806 Beiträge seit 2008
vor einem Jahr

Das sind ja zwei verschiedene paar Stiefel.
Statische Klasse mit Abhängigkeiten (hier Registry) vs statische Methode ohne Abhängigkeiten.

Klar, hier würde auch eine statische Methode funktionieren; aber halt auch nur, wenn man die Abhängigkeiten der Methode mit gibt, aber eben nicht in der Methode erzeugt werden.


public static class RegistryExtensions
{
    public static bool TryDisplayValue(RegistryKey sourceKey, string keyName, out string? value)
    {
        if (sourceKey.OpenSubKey(keyName) is RegistryKey key)
        {
            value = key.GetValue("DisplayVersion")?.ToString();
            key.Dispose();

            return true;
        }

        value = null;
        return false;
    }

    public static string? GetCitrixWorkspace(RegistryKey local64, ILogger logger)
    {
        if (TryDisplayValue(local64,
            @"SOFTWARE\WOW6432Node\Microsoft\Windows\CurrentVersion\Uninstal\CitrixOnlinePluginPackWeb", out string? wowKey))
        {
            logger.Log(LogLevel.Information, "Log");
            return wowKey;
        }
        else if (TryDisplayValue(local64,
            @"SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\CitrixOnlinPluginPackWeb", out string? msKey))
        {
            // hier wenn gewünscht logging
            return msKey;
        }

        // hier wenn gewünscht logging
        return null;
    }
}

Der Fakt aber bleibt, dass Du dies nicht (einfach so) für Testing mocken kannst.

L
LittleTester Themenstarter:in
158 Beiträge seit 2019
vor einem Jahr

Ich habe mir das durchgelesen. Bei dem Thema seid ihr ja echt total unterschiedlicher Meinungen.

Für mich als jemand der ohnehin schon stark zu kämpfen hat und ein Fall von "Unbedingt Wollen, aber eigentlich nicht Können" ist, ist das schwer, wenn die beiden aktivsten und hilfsbereitesten Mitglieder jeweils unterschiedliche Meinungen vertreten und man nun nicht so recht weis, was OK ist und was nicht.

Ich nehme jetzt mal mit, dass man die Methodenvarianten in klein gehaltenen Projekten verwenden kann und nicht grundlegend falsch ist, es aber durchaus sinnvoll ist bei größeren Projekten auf die Klassenvariante zu wechseln, bzw. zumindest dann, wenn man sich von einer Drittkomponente (Hier Registry) abhängig macht. Mit hoffentlich fortschreitendem Wissen sollte man dann beide Varianten beherrschen.

Zu definieren wäre dann noch, was als "Drittkomponente" zu definieren ist, weil im Grunde greift man bei WMI-Abfragen ja auch auf eine Drittkomponente (WMI-Klasse) zurück und ist von dieser abhängig.

@Th69: Abt schreibt noch, dass die Methodenvariante (in meiner Vorgehensweise?) das Drei-Schichten-Modell verletzt. Gehst du da mit? Wie handhabst du diese Thematik?

IDE: Visual Studio 2022
Sofern nicht anders genannt basieren meine Projekte auf C# und .net 6

16.806 Beiträge seit 2008
vor einem Jahr

Die Schichtenverletzung hat nichts mit der Variante (ob static oder nicht) zutun.
Ist in beiden Fällen eine Verletzung.

Auch hat die Variante nichts mit der Größe eines Projekts zutun, sondern mit Anforderungen an den Code - klassischer Fall von Software Architektur. Daher sind beide Fälle „okay“ - je nachdem was man tun will.
Deine Anforderung an testbaren Code wird nicht hoch sein. Für dich funktionieren beide Varianten wahrscheinlich.
In „meiner Welt“ hätte ich nur wenige Fälle, in denen die statische Variante „okay“ wäre - aufgrund der Möglichkeiten, die hier geringer sind, zu testen.
Daher ist deine Frage „ist das guter Code“ auch nicht pauschal zu beantworten - es gibt einfach sehr viel unterschiedliche Anforderungen an Code - und viele Wege mach Rom.

Und als letztes: hab nirgends von Komponenten was gesagt, sondern von Abhängigkeiten.
Deine Abhängigkeit ist hier die Datenquelle: die registry.

PS: das Forum hier besteht aus viel funktionalen Code, also statische Handler-Varianten; aber halt nur für die Fälle, wo das funktioniert und alle Abhängigkeiten über Parameter mitgegeben.

L
LittleTester Themenstarter:in
158 Beiträge seit 2019
vor einem Jahr

Ist WMI auch als Datenquelle zu sehen oder ist das was anderes? Für mich irgendwie schon, weil ich darüber ja die Informationen beziehe, die ich dann anzeigen lasse.

Ja, das mit der Abhängigkeit habe ich so gemeint, also Abhängigkeit von der Registry, die ja eine Systemkomponente ist. Ist für mich das gleiche. Ist es aber in der Sprache der Programmierung gar nicht und ich vermische etwas? Von WMI als Komponente bin ich ja auch abhängig. Wenn Microsoft da was ändert könnte mein Tool plötzlich andere Informationen ausgeben oder auch gar nicht mehr funktionieren. Ich frage das nur, um mich zukünftig hoffentlich verständlich aus zu drücken.

Die Frage nach "gutem" Code, war so gemeint, ob das Code ist mit dem man durchaus was anfangen kann. Bei dem alten "Spaghetti-Code" den ich an den Tag gelegt habe sind wir uns ja einig, dass man es drehen und wenden kann wie man will: Er funktionierte einfach nur, machte aber schon mein kleines Mini-Tool wahnsinnig unübersichtlich, weil alles in eine einzige Datei geklatscht wurde (Form1.cs) und am Ende über 1.000 Zeilen Code stand. Das war nicht gut.

IDE: Visual Studio 2022
Sofern nicht anders genannt basieren meine Projekte auf C# und .net 6

M
368 Beiträge seit 2006
vor einem Jahr

Abhängigkeit... "gutem" Code Man sollte -auch auf Vorrat- davon ausgehen, dass sich die externen wie internen Laufzeitbedingungen eines Programms (öfter(s)) ändern können (z.B. könnte man das Vorhandensein einer Registry oder bestimmter Äste dort im Vorab abfragen). Ansonsten besteht eine weitere Gefahr von "Pain Driven Development" (Anspielung auf das neuste YT-Video von D. Tielke)

Goalkicker.com // DNC Magazine for .NET Developers // .NET Blogs zum Folgen
Software is like cathedrals: first we build them, then we pray 😉

4.931 Beiträge seit 2008
vor einem Jahr

Das mit abhängigen Komponenten ist ein generelles Problem der Software-Architektur (bei längerfristigen Projekten weiß man ja nie, wie lange externe Komponenten noch verwendet werden können oder ob man sie dann nicht doch eines Tages durch etwas anderes ersetzen möchte).
Sauber läßt sich das nur lösen, wenn man diese Abhängigkeit so lokal wie möglich hält (also nur ein paar [kleine] Klassen hat, welche diese externe Komponente verwendet).
Idealerweise erstellt man sich eine eigene Schnittstelle (interface) und erzeugt dann dazu eine Implementationsklasse, welche alle Funktionalitäten auf die externe Komponente weiterleitet (auch Wrapper-Klasse genannt). Und im eigenen Code wird dann nur diese Schnittstelle verwendet, so daß dieser Code dann unabhängig von der externen Komponente [per Mocking] getestet werden kann.