Refactoring of logging rules

I’ve been working on a logging system for RemoteX Applications, the system is going Live any day now. (In fact since this post will be scheduled it might already have).

So I started out doing the code as straight forward as possible, but it quickly started to nest if-statements as the different logging conditions grew. I got something similar to this:

publicvoid HandleNotification(NotificationEventArgs notificationEventArgs) { if(notificationEventArgs.Type == typeof(Case) ) { var c = getCase(notificationEventArgs.Reference) AddForLogging("Case status changed to "+c.Status); } if(notificationEventArgs.Type == typeof(Resource)) { AddForLogging("Resource changed"); } if(notificationEventArgs.Type == typeof(Cost)) { var p = getProduct(notificationEventArgs.Reference); AddForLogging("Cost updated for product" + p ); } }

Now I wanted the if statements to execute after one anther since they would eventually build up to a single entry in the Log. However as the information that should be logged increased. This method became increasingly hard to maintain.

Also this point is a natural extension point for adding new rules for what should be logged and when. Thus I wanted it to be easy and simple to add things for logging.

By extracting an interface using a variation of the Specification pattern, I get the ILoggingRule.

Each if-statements body is moved to and implementation of the Execute method, and the Boolean expression is expressed in the AppliesFor methods execution. With this and a simple foreach-loop. The above code becomes the following:

publicvoid HandleNotification(NotificationEventArgs notificationEventArgs) { foreach( var loggingRule in _rules.Where( loggingRule => loggingRule.AppliesTo( notificationEventArgs ) ) ) { loggingRule.Execute( notificationEventArgs, AddForLogging); } }

.csharpcode, .csharpcode pre { font-size: small; color: black; font-family: consolas, "Courier New", courier, monospace; background-color: #ffffff; /white-space: pre;/ } .csharpcode pre { margin: 0em; } .csharpcode .rem { color: #008000; } .csharpcode .kwrd { color: #0000ff; } .csharpcode .str { color: #006080; } .csharpcode .op { color: #0000c0; } .csharpcode .preproc { color: #cc6633; } .csharpcode .asp { background-color: #ffff00; } .csharpcode .html { color: #800000; } .csharpcode .attr { color: #ff0000; } .csharpcode .alt { background-color: #f4f4f4; width: 100%; margin: 0em; } .csharpcode .lnum { color: #606060; }

Now this is simple enough to extend using Castle, and it moves the complexity away from the generic log handling. Thus it allows for more complex logging-rules as well.

Here is an example of a logging rule:

publicclass CaseStatusLoggingRule : ILoggingRule { conststring CaseStatusChangeTemplate = "$L{{Log.Case}} $L{{Log.StatusChange}} $L{{Log.Case.Status.{0}}} $L{{Log.To}} $L{{Log.Case.Status.{1}}}."; /// /// returns true if this rule applies to the notification/// publicvirtualbool AppliesTo( NotificationEventArgs notificationEventArgs ) { return notificationEventArgs.EntityType == typeof( Case ) && notificationEventArgs.NotificationType == NotificationType.PropertyChanged; } /// /// Executes the logging rule/// A callback that will execute the actual logging, first parameter is href, second is messagepublicvirtualvoid Execute( NotificationEventArgs notificationEventArgs, Action<string, string> logCallback ) { var caseHref = "cases/" + notificationEventArgs.EntityRestId; string message = String.Format( CultureInfo.InvariantCulture, CaseStatusChangeTemplate, notificationEventArgs.Changes[0].OldValue, notificationEventArgs.Changes[0].NewValue); logCallback(caseHref, message); } }

As you can see the if this would have been placed in the first code the if-statements and their bodies would wreak havoc on the readability of the code. Now instead its moved to it’s own class, and the logging can naturally be extended without adding complexity other than what’s required for the specific logging criteria.

What’s more funny is that about a month ago I did something similar to how Shuffle handles its serialization of Track entities. It uses the XmlPullParser since I’ve read that it’s the highest performing parser on the Android Platform (please let me know if I’m wrong).

Now parsing a single entity from the Tracks-service follows this procedure:

public ParseResult parseSingle(XmlPullParser parser) { EntityBuilder builder = createBuilder(); E entity = null; boolean success = true; try { int eventType = parser.getEventType(); while (eventType != XmlPullParser.END_DOCUMENT && entity == null) { String name = parser.getName(); switch (eventType) { case XmlPullParser.START_DOCUMENT: break; case XmlPullParser.START_TAG: Applier applier = appliers.get(name); if(applier != null) { success &= applier.apply(parser.nextText()); } break; case XmlPullParser.END_TAG: if (name.equalsIgnoreCase(mEntityName)) { entity = builder.build(); } break; } eventType = parser.next(); } } catch (IOException e) { Log.d("Exception", "IO EXception", e); returnnew ParseResult(); } catch (XmlPullParserException e) { Log.d("Exception", "pullparser exception", e); returnnew ParseResult(); } returnnew ParseResult(entity, success); }

As you can see there is nothing entity specific about this code, it’s a template method. The parsing is instead controlled through the dictionary containing something I badly called Appliers. An Applier takes a xml tag and applies it to the Shuffle entity builder.

All parsing is done using this method and the implementing template method classes instead create a list of appliers that can react to an expected xml tag.

An applier looks something like this:

appliers.put("name", new Applier(){ @Override public boolean apply(String value) { specificBuilder.setName(value); returntrue; } });

I’d appreciate feedback, I’m quite certain there’s a pattern lurking here and I don’t know if the Specification pattern is the correct one.