Now we want to tackle the coupling between WizbangWidget and MonolithLogger. This should not only get us even closer to the
Single-Responsibility Principle, but should now make it easier to test that WizbangWidget logs as expected.
The first thing we notice is that logging is sort of just another event broadcast: we're just telling the world "here is something that happened, and we want this to be logged." So we handle the logging in much the same way that we handled events: by broadcasting to any 'log listeners' added to WizbangWidget. We've also been asked to start distinguishing between different types of logged events. For WizbangWidget.DoItAll(), we've identified four types of logged events:
- INFO: This is just an informational logged event, such as 'Entering DoItAll'
- WARNING: This is intended as a warning that an admin might want to pay attention to, such as 'Thing X did not comform to our rules'
- CONTRACT BREACH: This is intended as a warning that an admin would want to pay attention to, explicitly that some client broke our pre-condition rules in calling 'DoItAll()', such as 'someParameter was out of bounds'
- ERROR: This means that an exception/error occurred
Note that now Wizbang Widget only has no responsibilities for logging; we only know when certain 'loggable events' occur, and if a client has given us a way to 'talk about' those events, we will 'talk about' them. Just like with Broadcastable Events, Wizbang Widget now no longer knows nor cares what if anything a client does with the information. Thus, things that are not supposed to be Wizbang Widget's responsibility - like figuring out or knowing about the stack trace, etc. - are not forced on it.* Thus Wizbang Widget is closer to the
Single-Responsibility Principle. We've used the same
version of Strategy Design Pattern to effect this (the same as we used for Events Of Interest in the First Improvement), which also brings Wizbang Widget closer to adherence with the
Open/Closed Principle. Both Single-Responsibility and Open/Closed Principles are part of the
SOLID Principles.
Below is the second improvement of the code in WizbangWidget.DoItAll(), as well as code for new tests we can now write, and notes about what logging tests we still cannot write and why. For all of the code, including the client that uses the new version of Wizbang Widget, open the solution Monolithv3.
Here's the code in WizbangWidget.DoItAll():
namespace MonolithComponent1
{
public enum LogEventType {
INFO,
WARNING,
CONTRACTBREACH,
ERROR
}
public class WizbangWidget {
private MonolithLogger _logger = new MonolithLogger ();
private List< Action< string>> _listeners = new List< Action< string>>();
private List< Action< LogEventType, string >> _loggingListeners = new List<Action <LogEventType , string >>();
public void AddLoggingListener( Action< LogEventType, string > loggingListener) {
_loggingListeners.Add(loggingListener);
}
public void AddListener( Action< string> listener) {
_listeners.Add(listener);
}
public void DoItAll( string someParameter) {
NoteLoggableEvent( "Beginning 'DoItAll'", LogEventType .INFO);
var repository = new MonolithRepository();
var coolComponent = new CoolComponent();
if ( new List< string> { "1", "2" , "noconform" }.Contains(someParameter)) {
NoteEvent( string.Format( "someParameter = '{0}'", someParameter));
}
else {
NoteAndLogEvent("someParameter was out of bounds.", LogEventType.CONTRACTBREACH);
}
var thing = repository.GetThing(someParameter);
var doesNotConform = string.Empty;
if (ThingDoesNotConformToTheRulesWeProvideToAddValue(thing)) {
NoteLoggableEvent
( string.Format( "Thing '{0}' did not conform to our sophisticated rules,", thing.Name),
LogEventType.WARNING);
doesNotConform = " But it did not conform to our sophisticated rules.";
}
try {
var theNoise = coolComponent.TheNoiseAThingMakesWhenYouPokeItWithAPointedStick(thing);
if (theNoise.Contains( "Burp!!!")) {
NoteLoggableEvent(
string.Format( "Thing {0} made an inappropriate noise." , thing.Name),
LogEventType.WARNING);
}
var webService = new MonolithWebServiceHelper();
webService.RecordSoundAThingMade(thing, theNoise);
NoteLoggableEvent(
string.Format( "Thing '{0}' made noise '{1}', and that was recorded." + doesNotConform, thing.Name, theNoise),
LogEventType.INFO);
NoteEvent( string.Format( "Thing '{0}' was handled." + doesNotConform, thing.Name));
}
catch ( Exception ex) {
NoteLoggableEvent(
string.Format( "ERROR: {0}", ex.Message), LogEventType .ERROR);
throw;
}
}
private bool ThingDoesNotConformToTheRulesWeProvideToAddValue( Thing thing) {
return thing.Name.ToLower().Contains( "noconform");
}
private void NoteEvent( string eventDescription) {
foreach ( var listener in _listeners) {
listener(eventDescription);
}
new MonolithEventListener().SomethingHappened(eventDescription);
}
private void NoteLoggableEvent(
string eventDescription, LogEventType logEventType = LogEventType.INFO) {
foreach( var loggingListener in _loggingListeners) {
loggingListener(logEventType, eventDescription);
}
}
private void NoteAndLogEvent(
string eventDescription, LogEventType logEventType = LogEventType.INFO) {
NoteLoggableEvent(eventDescription, logEventType);
NoteEvent(eventDescription);
}
}
}
Note that because we don't have direct control over CoolComponent or Repository, we cannot easily test the following regarding logging:
- If, when Poked With A Pointed Stick, the Thing burps in any way, that that fact is broadcast as an Event Of Interest and as a Loggable Event
- If CoolComponent.TheNoiseAThingMakesWhenYouPokeItWithAPointedStick() throws an error, that Wizbang Widget handles that gracefully
- If Repository.GetThing throws an error, that Wizbang Widget handles that gracefully
- If Repository.GetThing returns null, that Wizbang Widget handles that gracefully/appropriately
- That whatever noise the Thing made when Poked With A Pointed Stick, gets recorded
Currently we could set up tests to test these things, but because these tests would rely on things like extra components and on data being set up exactly right, our tests would be vulnerable to the following extraneous problems (i.e. our tests would indicate that they had caught defects when those defects are not in the SUT - System Under Test):
- Data regarding a Thing that is not in the exact state we need it to be in to cause our test conditions. This might be because of data setup issues, because of the data changing due to time or other events in a database, or even because our tests change the data we're using
- Defects in lower-layer components would cause tests failing for reasons other than defects in the SUT
- The tests would be slower than otherwise
All of these problems mean that the tests under these conditions would not be at all suitable for Unit Tests.
In further sections, we'll tackle these problems. For now, here's the code to Unit Test logging as much as we can with the current design:
[TestClass]
public class WizbangWidgetv3UnitTests {
[ TestMethod]
public void WhenDoItAllIsCalledAndSomeParameterIsNotOutOfBoundsThenTheAppropriateLoggableEventsMustBeAnnounced() {
//Arrange
var logMessages = new List< string>();
var wizbangWidget = new WizbangWidget();
wizbangWidget.AddLoggingListener((let, ed) => logMessages.Add(ed));
//Act
wizbangWidget.DoItAll( "1");
//Assert
Assert.AreEqual(
2, logMessages.Count,
"When DoItAll was called with someParameter not out of bounds, the appropiate Loggable Events was not announced.");
}
[ TestMethod]
public void WhenDoItAllIsCalledAndSomeParameterIsOutOfBoundsThenTheAppropriateLoggableEventsMustBeAnnounced() {
//Arrange
var logMessages = new List< string>();
var wizbangWidget = new WizbangWidget();
wizbangWidget.AddLoggingListener((let, ed) => logMessages.Add(ed));
//Act
wizbangWidget.DoItAll( "outofbounds");
//Assert
Assert.AreEqual(
3, logMessages.Count,
"When DoItAll was called with someParameter not out of bounds, the appropiate Loggable Events was not announced.");
}
}
* It's very arguable that the decision about what sort of (logging) event a given event is, is outside the realm of Wizbang Widget's responsibilities. We (i.e. me - Slade - and the imaginary team working on Wizbang Widget) are of the opinion that we're not currently going to worry about that: if it is extra responsibility, it's very very little and we're currently comfortable with it. Part of this decision is because we can't think of a non-ugly way for a client to examine our event messages and determine if they should be logged, whether they're informational or warnings, etc. What do you think? if you wanted to remove that responsibility from Wizbang Widget, how would you go about it? what would that entail for a client using Wizbang Widget?