Es geht um folgenden Eintrag aus Dariusz Blog:
http://blogs.msdn.com/dparys/archive/2009/01/20/clean-code-mein-versuch-daran-zu-arbeiten.aspx
Leider kann man keine neuen Kommentare an seinen Artikel hängen, also schreibe ich meinen Senf in mein Blog.
Es gibt an deiner Lösung mehrere Dinge die ich noch für verbesserungswürdig halte.
1. Eine Methode die nichts weiteres macht als eine andere Methode aufzurufen, ergibt keinen Sinn.
Statt
private bool IsProxyValid()
{
return IsProxyOpen();
}
private bool IsProxyOpen()
{
return IsProxyInstance()
? proxy.State == CommunicationState.Opened : false;
}
private bool IsProxyInstance()
{
return proxy != null;
}
könntest du eine Methode einsparen und es gleich so machen:
private bool IsProxyValid()
{
return IsProxyInstance()
? proxy.State == CommunicationState.Opened : false;
}
private bool IsProxyInstance()
{
return proxy != null;
}
Aber Moment, ich bin noch nicht fertig.
2. Deine Proxy-Klasse ist doch sowieso instanziiert, oder? Das klein geschriebene Wort proxy ist jedenfalls ein Indiz dafür. Ein weiteres ist der Zugriff auf das State-Property. Das ist doch wohl nicht statisch, oder? IsProxyInstance dürfte demnach also immer true zurück liefern. Die Methode kannst du dir also sparen.
3. Ich stolpere immer wieder über diese Zeile:
return IsProxyInstance()
? proxy.State == CommunicationState.Opened : false;
Ich muss gut überlegen was hier passiert - und das bedeutet schon daß hier etwas gegen das CCD-Prinzip verstößt.
Mal sehen, Da wird gefragt, IsProxyInstance. Dann wird eine weitere Prüfung ausgeführt, die ermittelt ob der Kommunikationskanal offen ist. Was passiert, wenn er nicht offen ist? Dann ergibt diese zweite Prüfung false.
In diesem Falle würde sich dieses Bild ergeben:
return true ? false : false
Es wird zwar korrekt zurück gegeben daß der Proxy in einem invaliden Zustand ist, aber trotzdem sieht das Konstrukt doch reichlich seltsam und verwirrend aus.
Wenn du jetzt meinen Punkt 2 hier einfließen lässt, ergibt sich als neue (zwischenzeitliche) Lösung das:
private bool IsProxyOpen()
{
return proxy.State == CommunicationState.Opened;
}
4. Robert C. Martin hat in seinem Buch auch empfohlen daß Code lesbar sein sollte wie ein Buch. Darauf bist du ja auch eingegangen - nur nicht konsequent. Dein Code liest sich so:
if is proxy valid
Nach der Revision aus Punkt 2 und 3 so:
if is proxy open
Du musst zugeben, das ist kein korrektes Englisch. Müsste es nicht so aussehen?
if proxy is open
Daraus mache ich eine Anforderung. Der Code sollte demnach so aussehen:
if(proxy.IsOpen)
Dieses Ergebnis erreichen wir, indem wir die Prüfung in die Proxy-Klasse auslagern. Das ist nicht nur legitim, sondern sogar besser. Auf diese Weise erreichen wir eine höhere Flexibilität und weniger Redundanz. Die Prüfung muss nur einmal geschrieben werden, egal wie viele Komponenten und Applikationen du hast!
Ein weiterer Pluspunkt: Damit ist das Separation of Concerns Prinzip erfüllt. Es obliegt dem Proxy, seinen Status anzugeben.
Ich schreibe in die Proxy-Klasse ein öffentliches, nicht statisches readonly boolean Property "IsOpen" und verlagere den gesamten Prüfcode in den Getter.
public bool IsOpen
{
get { return this.State == CommunicationState.Opened; }
}
An dieses Property gelangt man nur über eine Instanz (Prüfung darauf entfällt also) und der Code liest sich wie korrektes Englisch.
Übrigens könnte this. entfallen. Ich habe es eingebaut um darauf hinzuweisen daß das State-Property aus der selben Klasse stammt.
if(proxy.IsOpen)