Full Stack Universal Windows Platform

Don't just dream it. Hack it.

Clean Code – Mein Versuch daran zu arbeiten

Ich finde die Idee vom Clean Code Developer Schick. Wer möchte schliesslich nicht in seinem tun besser werden? Ich habe angefangen mich mit dem Buch Clean Code zu beschäftigen. Heute beim weiter arbeiten an meinem Code habe ich folgende Funktion entdeckt:

   1: private bool ProxyIsValid()
   2: {
   3:     return (proxy != null && proxy.State == CommunicationState.Opened) ? true : false;
   4: }

Eine kurze Funktion die Aussagekräftig genug ist, oder? Doch irgendwie stört mich etwas daran.

Ich fange mal mit dem Namen an. ProxyIsValid(). Vielleicht sollte ich es lieber IsProxyValid() nennen? Schliesslich prüfe ich bevor ich auf den Proxy zugreife ob ich es tun kann. Doch was macht die Funktion.

Zum einen prüft sie ob die Instanz nicht null ist und dann prüft sie noch ob die Instanz einen Kommunikationskanal geöffnet hat. Sind eigentlich zwei Prüfungen, der Name IsProxyValid() ist zwar nich generell falsch doch viel zu generisch. Ich benenne die Funktion um in IsProxyValidAndOpen(). Anbei der Code der momentanen Funktion und ein Aufruf der Funktion aus einer anderen heraus:

   1: private void NotifyTicketAvailable(Ticket ticket)
   2: {
   3:     if (IsProxyValidAndOpen())
   4:     {
   5:         proxy.NewTicketAvailable(
   6:             new NewTicketAvailableRequest { Ticket = ticket });
   7:     }
   8: }
   9:  
  10: private bool IsProxyValidAndOpen()
  11: {
  12:     return (proxy != null && proxy.State == CommunicationState.Opened) 
  13:         ? true : false;
  14: }

Doch irgendwie stören mich noch die direkten Abfragen auf die Instanz und die Eigenschaft. Sollte man statt proxy != null eine Funktion implementieren? Sollte man proxy.State == CommunicationState.Opened kapseln? Ich mache es einfach mal und schaue mir dann das Ergebnis im Code an:

   1: private void NotifyTicketAvailable(Ticket ticket)
   2: {
   3:     if (IsProxyValidAndOpen())
   4:     {
   5:         proxy.NewTicketAvailable(
   6:             new NewTicketAvailableRequest { Ticket = ticket });
   7:     }
   8: }
   9:  
  10: private bool IsProxyValidAndOpen()
  11: {
  12:     return (IsProxyValid() && IsProxyOpen()) 
  13:         ? true : false;
  14: }
  15:  
  16: private bool IsProxyOpen()
  17: {
  18:     return proxy.State == CommunicationState.Opened;
  19: }
  20:  
  21: private bool IsProxyValid()
  22: {
  23:     return proxy != null;
  24: }

Liest sich zumindest gut. Doch ist es richtig? Irgendwie hat sich ein neuer Fehler eingeschlichen. Ich habe, muss ich gestehen, keine Unit Tests für meinen Code. Ich kann die Änderungen jetzt nicht validieren. Da ist noch ein großes TODO das an mich selbst geht. Schreibe Unit Tests, schreibe Unit Tests, schreibe Unit Tests. Doch den Fehler sieht man bestimmt. Jemand könnte die Methode IsProxyOpen() aufrufen ohne das es eine Instanz Proxy gibt. Also wieder refactoring, ich schreibe die Methode IsProxyOpen() um:

   1: private bool IsProxyOpen()
   2: {
   3:     return IsProxyValid() 
   4:         ? proxy.State == CommunicationState.Opened : false;
   5: }

Hmmm… Ich drehe mich im Kreis scheint mir. Ist nun die Namensgebung IsProxyOpen() noch korrekt? Ich sage jetzt einfach mal ja. Also passe ich noch die Methode IsProxyValidAndOpen() an:

   1: private bool IsProxyValidAndOpen()
   2: {
   3:     return IsProxyOpen();
   4: }

Haleluja. Ne, doch nicht. Jetzt scheint es irgendwie auszuarten. Ich habe eine Funktion die heißt IsProxyValidAndOpen() und die gibt einfach den Return-Wert von IsProxyOpen() zurück. Das ist so nicht im Sinne des Erfinders, noch ist es lesbar, noch wartbar.

Wo liegt denn das Problem. Ich habe es. Die Namensgebung ist doch das Problem. Mein Denken darüber vor dem Umschreiben der Methode war bereits im Bilde, ich habe mich gewehrt, doch nun sehe ich es bildlich vorm Auge. Die Namen sind Crap!

Welchen Namen wähle ich nun, ich denke ich schaue mir nochmal alle momentanen Funktionen auf einmal an:

   1: private void NotifyTicketAvailable(Ticket ticket)
   2: {
   3:     if (IsProxyValidAndOpen())
   4:     {
   5:         proxy.NewTicketAvailable(
   6:             new NewTicketAvailableRequest { Ticket = ticket });
   7:     }
   8: }
   9:  
  10: private bool IsProxyValidAndOpen()
  11: {
  12:     return IsProxyOpen();
  13: }
  14:  
  15: private bool IsProxyOpen()
  16: {
  17:     return IsProxyValid() 
  18:         ? proxy.State == CommunicationState.Opened : false;
  19: }
  20:  
  21: private bool IsProxyValid()
  22: {
  23:     return proxy != null;
  24: }

Da, Zeile 21! Sollte nicht IsProxyValid() in IsProxyValidInstance() oder so umbenannt werden? Und IsProxyValidAndOpen() in IsProxyReadyForCommunication()? Bin mir nicht sicher. Ich sollte konsistent bleiben. Als nicht Ready und Valid mit ein und der selben Bedeutung ausstatten. Ich würde gerne meine ursprüngliche Bezeichnung IsProxyValid() für einen gültigen, voll initialisierten Proxy benutzen, der sofort zum kommunizieren genutzt werden kann. Dann würde ich IsProxyValid() einfach in IsProxyInstance() umbenennen.

   1: private void NotifyTicketAvailable(Ticket ticket)
   2: {
   3:     if (IsProxyValid())
   4:     {
   5:         proxy.NewTicketAvailable(
   6:             new NewTicketAvailableRequest { Ticket = ticket });
   7:     }
   8: }
   9:  
  10: private bool IsProxyValid()
  11: {
  12:     return IsProxyOpen();
  13: }
  14:  
  15: private bool IsProxyOpen()
  16: {
  17:     return IsProxyInstance() 
  18:         ? proxy.State == CommunicationState.Opened : false;
  19: }
  20:  
  21: private bool IsProxyInstance()
  22: {
  23:     return proxy != null;
  24: }

Bin ich jetzt glücklich. Nein, jetzt erkenne ich das die Prüfung des Proxies gar nicht die Aufgabe dieser Klasse ist, sondern mir der Proxy einfach eine Funktion zur Verfügung stellen sollte. In etwa so:

   1: private void NotifyTicketAvailable(Ticket ticket)
   2: {
   3:     if (proxy.IsValid())
   4:     {
   5:         proxy.NewTicketAvailable(
   6:             new NewTicketAvailableRequest { Ticket = ticket });
   7:     }
   8: }

Was meint Ihr? Hat sich der Aufwand gelohnt?