Polymorphic Dispatch, Open/Closed Principle, and Notifications

I started on working on a new application last week – one that will read data from various sources, analyze the data, and then notify interested parties about the results of the analysis.  I started working on the notification piece first.  My first premise is that there can be an variety of notification methods and an infinite number of providers of the methods.  For example, a person can receive a notification via an email, a text, a phone call, an IM, an audio signal from their phone app, Twitter, Skype, Facebook, etc…   Each of these methods can be served up by a host of providers.  Since these providers change so quickly and new mechanisms arrive (and quickly become old mechanisms), it makes sense that this part of my application should be extensible as possible.

After trying a variety of scenarios and objects, I decided to concentrate on the core Interface – which is the Alert.  I created an interface like so:

public interface IAlert
{
    void Send();
}

I then created a class called Alertee that contains a collection of IAlerts with some other properties:

public class Alertee
{
    public Alertee()
    {
        this.AlertList = new List<IAlert>();
    }

    public Int32 AlerteeId { get; set; }
    public String FirstName { get; set; }
    public String LastName { get; set; }
    public List<IAlert> AlertList { get; set; }

}

Notice the Command/Query separation.   This class is a data structure that only has public properties.  I initially toyed with calling the Alertee “User” but “User” is such an overused word – most every implementation of IAlert already has a “User” class – and I am a big believer of domain-unique language (and I hate fully-qualifying my Instances) so I chose the less ambiguous if-not-a-real-word: “alertee”.

I then created an AlertFactory (a command class) like so:

public class AlertFactory
{
    public void SendAlerts(List<Alertee> alertees)
    {
        foreach (Alertee alertee in alertees)
        {
            foreach (IAlert alert in alertee.AlertList)
            {
                alert.Send();
            }
        }
    }
}

Notice that by using the IAlert interface, the dependency is inverted and that Alert Factory delegates responsibility of the actual alert implementation via calling the alert.Send().

With the interface, the POCO, and the Factory class ready, I then started implementing the different kinds of alerts.  My first stop was an email alert from the local SMTP server.  I coded this class like so:

public class LocalEmailAlert: IAlert
{
    private String _from = String.Empty;
    private String _to = String.Empty;
    private String _subject = String.Empty;
    private String _body = String.Empty;

    public LocalEmailAlert(String from, String to, String subject, String body)
    {
        if (String.IsNullOrEmpty(from))
            throw new AlertsException("from cannot be null or empty.");
        if (String.IsNullOrEmpty(to))
            throw new AlertsException("to cannot be null or empty.");
        if (String.IsNullOrEmpty(subject))
            throw new AlertsException("message cannot be null or empty.");
        if (String.IsNullOrEmpty(body))
            throw new AlertsException("message cannot be null or empty.");

        _to = to;
        _from = from;
        _subject = subject;
        _body = body;
    }

    public void Send()
    {
        SmtpClient smtpClient = new SmtpClient();
        MailMessage mailMessage = new MailMessage(_from, _to, _subject, _body);
        smtpClient.Send(mailMessage);
    }
}

A couple of things to note.  All of the values that the SMTP needs to use are passed in via the constructor (constructor injection) and these values are stored in private fields.  Also, the values are verified in the constructor and a generic exception is thrown.

I then went to one of the million email providers out there and picked one at random: MailJet.  I then coded up a MailJetEmailProvider like so:

public class MailJetEMailAlert: IAlert
{
    private String _mailServerName = "in.mailjet.com";
    private Int32 _mailServerPort = 465;
    private String _apiKey = String.Empty;
    private String _secretyKey = String.Empty;
    private String _from = String.Empty;
    private String _to = String.Empty;
    private String _subject = String.Empty;
    private String _body = String.Empty;

    public MailJetEMailAlert(String from, String to, String subject, String body)
    {
        if (String.IsNullOrEmpty(from))
            throw new AlertsException("from cannot be null or empty.");
        if (String.IsNullOrEmpty(to))
            throw new AlertsException("to cannot be null or empty.");
        if (String.IsNullOrEmpty(subject))
            throw new AlertsException("message cannot be null or empty.");
        if (String.IsNullOrEmpty(body))
            throw new AlertsException("message cannot be null or empty.");

        _to = to;
        _from = from;
        _subject = subject;
        _body = body;
    }

    public void Send()
    {
        MailMessage mailMessage = new MailMessage();
        mailMessage.From = new MailAddress(_from);
        mailMessage.To.Add(new MailAddress(_to));
        mailMessage.Body = _body;
        mailMessage.Subject = _subject;

        SmtpClient client = new SmtpClient(_mailServerName, _mailServerPort);
        client.EnableSsl = true;
        client.Credentials = new NetworkCredential(_apiKey, _secretyKey);
        client.Send(mailMessage);
    }
}

Notice that the constructor is exactly the same as the local email implementation.  The Mail-Jet specific code/fields are assigned locally and are unique only to the Mailjet class.  I suppose that I could pass those elements in via the constructor and then keep the values in the .config file – but that didn’t seem right to me.  I also could access the config file directly from this class  but that introduces an unneeded dependency – now this class has to worry about the config file (see if it exists, if the section is there, etc…) which is another reason that the class has to change – a violation of the Single Responsibility Principle.  The more I code, the more I think that the config file should be accessed only once and in 1 place (in Main) and those values are then passed into the classes that need it.  That is a blog post for another time – and certainly is against what Microsoft has recommended for many years.

In any event, I don’t think that having the MailJetspecific code embedded into the class is a bad idea  in fact I think it is a good idea.  If these values changes, this class has to be recompiled and redeployed (so what, this isn’t java where the calling assembly has to be recompiled) compared to having the UI being mail-jet aware (via its config) and then having the config file redeployed.  I would rather have a cleaner separation of concerns and have a recompile than a muddied SOC and no recompile.  Both scenarios need to be redeployed anyway.

Back to Mail, I then realized that the mail classes could derive from an abstract base class like so:

public abstract class MailAlert
{

    private String _from = String.Empty;
    private String _to = String.Empty;
    private String _subject = String.Empty;
    private String _body = String.Empty;

    public MailAlert(String from, String to, String subject, String body)
    {
        if (String.IsNullOrEmpty(from))
            throw new AlertsException("from cannot be null or empty.");
        if (String.IsNullOrEmpty(to))
            throw new AlertsException("to cannot be null or empty.");
        if (String.IsNullOrEmpty(subject))
            throw new AlertsException("message cannot be null or empty.");
        if (String.IsNullOrEmpty(body))
            throw new AlertsException("message cannot be null or empty.");

        _to = to;
        _from = from;
        _subject = subject;
        _body = body;
    }

}

The problem is that the derived class still needs to access these private fields – the protected scope solves that:

protected String _from = String.Empty;
protected String _to = String.Empty; 
protected String _subject = String.Empty;
protected String _body = String.Empty;

So the next problem is abstract base class does not have a constructor that takes zero arguments  which is required.  So I added a default empty constructor like so:

public MailAlert():
    this(String.Empty,String.Empty,String.Empty,String.Empty)
{

}

It now complies and I removed the duplicative code.  The sub-lesson here is that, as rule, don’t start with an abstract class.  Start with implementations and if there is lots of repetitive code, the consider refactoring to an abstract class.  Your covering unit tests will tell you if your refactor is wrong  and you are using unit tests, aren’t you?

I then tackled Text pushing via CDyne.  I added a reference to the CDyne Service (I love SOAP) in Visual Studio, received a developer key from CDyne, and wrote the following implementation:

public class CDyneTextAlert: IAlert
{
    private String _phoneNumber = String.Empty;
    private String _message = String.Empty;
    private Guid _licenseKey = new Guid("XXXXXXXXXXXXXXXXXXXXXXX");
    
    public CDyneTextAlert(String phoneNumber, String message)
    {
        if (String.IsNullOrEmpty(phoneNumber))
            throw new AlertsException("phoneNumber cannot be null.");

        if (String.IsNullOrEmpty(message))
            throw new AlertsException("message cannot be null.");

        _phoneNumber = phoneNumber;
        _message = message;
    }

    public void Send()
    {
        IsmsClient client = new IsmsClient();
        SMSResponse response = client.SimpleSMSsend(_phoneNumber, _message, _licenseKey);
    }
}

I then added a Twitter push like so:

public class TwitterAlert: IAlert
{
    private String _twitterUri = "http://twitter.com/statuses/update.json";
    private String _twitterUserId = String.Empty;
    private String _twitterPassword = String.Empty;
    private String _message = String.Empty;

    public TwitterAlert(String message)
    {
        if (String.IsNullOrEmpty(message))
            throw new AlertsException("message cannot be null.");
        
        this._message = message;
    }

    public void Send()
    {

        HttpWebRequest request = (HttpWebRequest)HttpWebRequest.Create(_twitterUri);
        request.Credentials = new NetworkCredential(_twitterUserId, _twitterPassword);
        request.Timeout = 5000;
        request.Method = "POST";
        request.ContentType = "application/x-www-form-urlencoded";
        using (Stream requestStream = request.GetRequestStream())
        {
            using (StreamWriter streamWriter = new StreamWriter(requestStream))
            {
                streamWriter.Write(_message);
            }
        }
        WebResponse response = request.GetResponse();
    }
}

I then added a Phone notification from CDyne:

public class CDynePhoneAlert: IAlert
{
    String _phoneNumber = String.Empty;
    String _message = String.Empty;
    String _callerId = String.Empty;
    String _callerName = String.Empty;
    String _voiceId = String.Empty;
    String _licenseKey = "XXXXXXXXX";

    public CDynePhoneAlert(String phoneNumber, String message, String callerId, String callerName, String voiceId)
    {

        if (String.IsNullOrEmpty(phoneNumber))
            throw new AlertsException("phoneNumber cannot be null.");

        if (String.IsNullOrEmpty(message))
            throw new AlertsException("message cannot be null.");

        if (String.IsNullOrEmpty(callerId))
            throw new AlertsException("callerId cannot be null.");

        if (String.IsNullOrEmpty(callerName))
            throw new AlertsException("callerName cannot be null.");

        if (String.IsNullOrEmpty(voiceId))
            throw new AlertsException("voiceId cannot be null.");

        _phoneNumber = phoneNumber;
        _message = message;
        _callerId = callerId;
        _callerName = callerName;
        _voiceId = voiceId;

    }


    public void Send()
    {
        PhoneNotifySoapClient client = new PhoneNotifySoapClient("PhoneNotifySoap");
        NotifyReturn callReturnValue = client.NotifyPhoneBasic(_phoneNumber, _message, _callerId, _callerName, _voiceId, _licenseKey);
    }
}

I noticed that the message variable is being passed into the constructor in every IALert implementation so perhaps that should be added to the interface definition Send(String message) but then realized that each implementer might have its own requirements on the message.  For example, Twitter needs to check the number of characters:

public TwitterAlert(String message)
{
    if (String.IsNullOrEmpty(message))
        throw new AlertsException("message cannot be null.");

    if(message.Length > 140)
        throw new AlertsException("message is too long for Twitter.");
    
    this._message = message;
}

Also, what happens when the message is no longer a string – perhaps a .mp3 file with an recorded voice message to be sent to the phone?  Then the Send() method would have to overload an audioStream argument.

So instead of trying to build some class (or worse, some enum) of the different types messages that are passed in, I left it to the specific implementation.  This makes sense of the fluid nature of the distribution mechanisms in today’s technology landscape  – the API needs to be as flexible as possible.

So far, I have a solution that follows the Open/Closed principle.  As new distribution mechanisms come available, I just need to add a new class that implements the IAlert interface, add it to the Alertee’s Alerts collection, and it implements.  No other code needs to change.  I then ran into a bump – adding an Alert to the Alertee’s Alerts collection.

I added 1 more class to the project  – a Provider that creates alertes with instantiated Alerts.  I added a SqlAlerteeProvider figuring I will store each Alertee’s desired Alert mechanism (0 to infinity) in a Sql Server database so it looks like so:

public class SqlAlerteeProvider
{
    public Alertee GetAlertee(Int32 alerteeId)
    {
        return null;
    }

    public List<Alertee> GetAllAlertees()
    {
        return null;
    }

    public void UpdateAlertee(Alertee alertee)
    {

    }

    public void InsertAlertee(Alertee alertee)
    {

    }

    public void DeleteAlertee(Alertee aleertee)
    {

    }
}

However, I am deferring the implementation of the data store for as long as possible.  Who knows, I may find a different place to store the data.

So does application follow the open/closed principle?  It does up to the point of the Alertee Provider.  If a new alert needs to be created, a new class is added (ok with O/C) AND the GetAlertee code needs to be changed to account for the new provider (new fields in the database, etc..) (not OK with the O/C).  I suppose I can dig into making the provider follow the O/C, but that is a task for another day.

The important thing is that the calling application DOESN’T change – It just keeps calling SendAlerts() on the AlertFactory regardless of what else happens.

Business Logic: I Know It When I See It!

So we all know that business logic is supposed to be separate from any other layer in your application.  Other logic like database access and user interface logic should not be mixed in with the business logic.  This is all well and good but it begs the question – what is business logic?  How do I recognize it in code?

Is this business logic?

String sql = "Select * from Customer";

The answer is no – it is database access logic?

Is this business logic?

String sql = String.Format("Select * from Customer where customerId = {0}", customerId);

It might be.  If the application’s UI is simply displaying a customer’s record – then this is not business logic? However, if the application does something different, does a calculation, etc.. depending on the result of this statement, then it is business logic?

How about this?

Int32 customerTypeId=0;
customerTypeId = this.customerTypeDropDownList.SelectedIndex;
if(customerTypeId == 1)
{
    Response.Redirect("RegularCustomer.aspx");
}
else
{
    Response.Redirect("SpecialCustomer.aspx");
}

Is basic form navigation considered  business logic? 

And what about this?

if(customerTypeId == 1)
{
    InsertNewOrder();
}
else
{
    UpdateNewOrder();
}

Is basic control of flow of the application considered business logic?

Finally, what about?

foreach (LineItem lineItem in order.LineItems)
{
    total += lineItem.Price;
}

total += total * salesTax;

That sure looks like business logic – because it is doing a calculation?

Examples aside, can every line of code be categorized?  If so, what are the categories?

  • User Navigation?
  • Input Validation?
  • Database Interaction?
  • Business Logic?
    I went and did a search on “business logic” and there is little out there –and certainly nothing canonical.
    I would assume that business logic is context-free.  So If I took a line of code from one place in my application and put it in another, it still would be business logic.

I assume that code can morph into (or away from) business logic.  For example, that simple sql select statement might not be business logic, but once I all all these crazy where clauses, it becomes business logic – or if I add an order by statement it becomes presentation logic.

Another consideration is that the visual database controls that comes our of the box in Win and Web Forms

image

Automatically mix database and presentation logic together and if you add any conditional statements to the control, if becomes a three-headed monster of database, presentation, and business logic.

Similarly, all stored procedures that have an “IF” or “CASE” in them are probably business logic – business logic that is tightly coupled (indeed embedded) in the database CRUD.

Does that mean that any application that uses a visual control or a stored procedure has an incorrect architecture?  Maybe.  Perhaps separation of concerns is like the Open/Closed principle – you never get there but it is worth trying.

One Simple Change Would Redefine How Software Is Created

If Companies would add technical debt to their 10-Qs and 10-Ks, perhaps more people would listen to the clean coders who are pleading for the right tools, the right SDLC methodologies, and the correct investment in software professionals.

For example, consider Google.  I took their most recent 10-K and added a line item for technical debt.  I have no idea about the quality of their code.  My understanding is that they do all night “hack-a-thons” and hire people who consider themselves the best in their field.  As a general rule => Large Ego + Lack Of Sleep == Bad Code.  I bet this plug number is understated:

image

Is technical debt a “real number?”  It is a real as Goodwill or Intangible Assets.  What kinds of companies would have the most technical debt?

  • Companies that sell software but make most of their money on support
  • Companies that have software assets but make most of their money on suing other companies
  • Companies that are 100% dependent on their IT systems but don’t aren’t serious about owning their own code base

A result  of reporting the technical debt on the 10-Qs and Ks is that the free lunch of hacking together spaghetti code (usually offshore) lowest-bidder dev shop stops: the income statement gain of lower costs is offset by an increase of technical debt on the balance sheet.  And this is not a 1 to 1 relationship, I bet every dollar of savings on the income statement increases technical debt by a factor of two.

Another result is that companies that are hacking together code to get acquired will have their purchase price offset by the mess that they are dumping on the acquiring company.  Lower valuations would possibly force companies to get their act together before they are put up for sale.  But if their act is together, perhaps they wouldn’t be for sale?

In any event, my guess is that once executives are focused on technical debt and have to confront it as a real number that affects their annual report, companies will becomes more serious about eliminating their debt. 

So what pattern is this

I was working with Rob Seder on an interesting problem.  I have a 3rd party assembly that I am writing a façade over – this façade will be used by other developers in their applications.  Because of licensing, the 3rd party assembly cannot be installed on workstations.  The assembly can be installed on a WCF service that applications can then call – a façade calling another façade.

Following the ADO.NET model, we created a Connection that inherited from DBConnection and a Command that inherited from DbCommand.  My initial thought was to create two different commands that reflect the different connection methods: a WebServiceCommand and a DirectCallCommand with the individual implementations in each command’s ExecuteScaler() method.  Each command would take in a connection that that is specific to the connection type.

After some discussion, we decided to do the opposite.  We created an interface for the connections that have 1 method, Execute, and that takes in the type of command needed.

interface IFooConnection
{
    object Execute(FooCommand command);
}

The FooCommand derived from DbCommand and newed the Connection property:

image

 

We then created two connections that implement the IFooConnection interface, for example:

image

and

image

 

In the execute method, we implemented the connection-specific code.  The WebService call in the FooWebServiceConnection and the direct API calls in the FooDirectCallConnnection.

Then, we overrided the FooCommand ExecuteScaler, that calls the connection’s specific implementation:

public override object ExecuteScalar()
{
    return this.Connection.Execute(this);
}

I like this solution because it is extensible – new kinds of FooConnections come in, we just have to create specific implementation in the Execute method.  I have some questions in my head:

  • Does this follow any established design-pattern?  I re-read the GOF book this AM and could not find one that matched.
  • Is this an example of any SOLID principle?
  • Is this an example of Dependency Injection?

The Coolness Of Inheritance

I was writing a simple request/response to a non-WCF web service.  The service’s request SOAP looked like this:

<?xml version="1.0" encoding="utf-8" ?>
<WorkItem AppName='TBD'>
  <UserName nametype='familiar'>Jamie</UserName>
  <UserItem>Pencil</UserItem>
</WorkItem>

created some classes that matched the request:

[Serializable]
public class WorkItem
{
    [XmlAttribute(AttributeName="AppName")]
    public string ApplicationName { get; set; }
    [XmlElement]
    public UserName UserName { get; set; }
    [XmlElement]
    public string UserItem { get; set; }
}

and

[Serializable]
public class UserName
{
    [XmlAttribute(AttributeName = "nameType")] 
    public string NameType { get; set; }
    [XmlText]
    public string Value { get; set; }
}

I then created a function that populates these classes with the data:

static WorkItem CreateWorkItem()
{
    WorkItem workItem = new WorkItem();
    UserName userName = new UserName();

    userName.NameType = "familiar";
    userName.Value = "Jamie";

    workItem.ApplicationName = "TBD";
    workItem.UserName = userName;
    workItem.UserItem = "Pencil";

    return workItem;
}

Finally, I created a helper function that takes the classes and serializes them as XML:

static XmlDocument CreateXMLDocument(WorkItem workItem)
{

    XmlSerializer serializer = new XmlSerializer(typeof(WorkItem));
    XmlSerializerNamespaces namespaces = new XmlSerializerNamespaces();
    namespaces.Add(String.Empty, String.Empty);
    StringWriter stringWriter = new StringWriter();
    serializer.Serialize(stringWriter, workItem, namespaces);
    stringWriter.Close();

    XmlDocument xmlDocument = new XmlDocument();
    xmlDocument.LoadXml(stringWriter.ToString());
    return xmlDocument;
}

When I run it, things look great… except that the Encoding is wrong:

image

The path of least resistance would be to set the Encoding property of the StringWriter class.  However, that property is read-only.  After playing around with the different classes in System.IO that expose encoding (usually though the constructor), I stumbled upon this great article.  The easiest way to get UTF-8 encoding in a stringWriter is to override the default implementation in the constructor.  I went ahead and created a new class and overrode the Encoding property.

public class UTF8StringWriter : StringWriter
{
    Encoding encoding;
    public UTF8StringWriter()
        : base()
    {
        this.encoding = Encoding.UTF8;
    }

    public override Encoding Encoding
    {
        get
        {
            return encoding;
        }
    }
}

Note that I used a local variable.  Thank goodness the StringWriter uses its property (not a private variable) in the Serialize method.  A big thank you to whoever wrote that class in a proper way.  I then changed the stringWriter variable to a UTF8WringWriter like this:

UTF8StringWriter stringWriter = new UTF8StringWriter();

The output now renders correctly:

image

StreamWriter – Don’t Forget To Flush

I started working through the examples in the CLR Profiler documentation over the weekend.  The 1st one looks like this:

StreamReader streamReader = new StreamReader(@"C:\Users\Jamie\Desktop\Demo1.dat");
string line;
int lineCount = 0;
int itemCount = 0;
while ((line = streamReader.ReadLine()) != null)
{
    lineCount++;
    string[] items = line.Split();
    for (int i = 0; i < items.Length; i++)
    {
        itemCount++;
    }
}
streamReader.Close();

Console.WriteLine("{0} lines, {1} items", lineCount, itemCount);

To make that Demo1.dat file, I needed to write a quick function that spits out the expected values.  I coded up this:

StreamWriter streamWriter = new StreamWriter(@"C:\Users\Jamie\Desktop\Demo1.dat");
string word = "0123456789 ";
for(int i=0; i<2000; i++)
{
    StringBuilder stringBuilder = new StringBuilder();
    for(int j = 0; j < 10; j++)
    {
        stringBuilder.Append(word); 
    }
    streamWriter.WriteLine(stringBuilder.ToString());
}

When I ran the function, the last chunks of data was not showing up:

image

I talked to my friend Rob Seder and he said “You dummy, you forgot to flush!*”  Sure enough, when I set this:

streamWriter.AutoFlush = true;

the results came out as expected.

 

*He really didn’t say “You Dummy”, that is what I told myself once he mentioned it…..

In-Line Props

I am working on a brownbag for entity framework using Northwind as my database. When coding up a POCO, I decided to use the in-line property setting available since VS2008:

Product product = new Product()
{
    ProductId = (Int32)sqlDataReader[0],
    ProductName = (String)sqlDataReader[1],
    Supplier = new Supplier(),
    Category = new Category(),
    QuantityPerUnit = (String)sqlDataReader[4],
    UnitPrice = (Int16)sqlDataReader[5],
    UnitsInStock = (Int16)sqlDataReader[6],
    UnitsOnOrder = (Int16)sqlDataReader[7],
    ReorderLevel = (Int16)sqlDataReader[8],
    Discontinued = (Boolean)sqlDataReader[9]
};

The problem is that when I ran it, one of the properties was cast incorrectly. However, by using in-line, I can’t tell which one is the problem.

clip_image002

If I did it the OFW (Old-Fashion Way) like this":

Product product = new Product();
product.ProductId = (Int32)sqlDataReader[0];
product.ProductName = (String)sqlDataReader[1];
product.Supplier = new Supplier();
product.Category = new Category();
product.QuantityPerUnit = (String)sqlDataReader[4];
product.UnitPrice = (Decimal)sqlDataReader[5];
product.UnitsInStock = (Int16)sqlDataReader[6];
product.UnitsOnOrder = (Int16)sqlDataReader[7];
product.ReorderLevel = (Int16)sqlDataReader[8];
product.Discontinued = (Boolean)sqlDataReader[9];

I would have had the break on 1 line and easily identified the offending line of code:

clip_image004

The morale of the story is the same as many of my other stories – separate – don’t be lazy.

Ruminations About Enumerations

 

I have been busy refactoring my Windows Phone 7 game and I started using enumerations more to increase the readability of my code.  Some of the game rules are so complex and convoluted that I found enumerations a very handy way to express my intentions.  For example, consider this:

1 2 if (targetUnit.Equipment.EquipmentSubClass.EquipmentClass.EquipmentGroup.EquipmentGroupId == 0 3 && targetTile.Terrain.TerrainType.TerrainTypeId == 1 4 && targetUnit.CanMove 5 && targetUnit.CanAttack) 6 { 7 this.menuFactory.ShowUpgrade = true; 8 }

to this:

1 if (targetUnit.Equipment.EquipmentGroupEnum == EquipmentGroupEnum.Land 2 && targetTile.Terrain.TerrainTypeEnum == TerrainTypeEnum.Port 3 && targetUnit.CanMove 4 && targetUnit.CanAttack) 5 { 6 this.menuFactory.ShowUpgrade = true; 7 }

If you dropped into this project, which one would you understand quicker?  This readability comes with a cost though – I now have to create separate files of code that I need to maintain. A second problem is that I am duplicating data – the lookup values from my database are hard-coded into the business layer.  If I add a value to the database and want to use it in my business logic, I will have to recompile.  A final problem is one of naming – I have a class and an enum that means exactly the same thing but I don’t want to name them the same thing (see Domain specific language), even if used different namespaces.  In this example, I appended “enum” to the suffix of the class: TerrainType and TerrainTypeEnum.  Taking these problems in order:

Problem #1: separate code to maintain – Yes, though it seems like a small price to pay.  The trend in our industry is to have more classes/things that do only 1 thing (Single Responsibility Principle) so a large number of files is a natural side-effect.  Fortunately, VS2010 makes file cataloging easy so it does not really get in the way of developer productivity.

Problem #2: duplicate data.  Yes, but…. The enums are data that is to be used in design time (as part of the business/programming logic).  The data that is stored in the classes are for the end-user.  That data is volatile  and changes can be reflected at run-time without recompiling, mostly.  I would maintain that due to the separate audiences, it is ok to have 2 “things” represent the same data.  I would also argue that enums are only for classes that are lookups – reference tables in your database for example.  In that case, even if you make a change to a reference table and want to implement some additional logic in your code, guess what?  You are still recompiling.  You just have to remember to add that value to the enum – which should be fairly straightforward if your spent some time to make sure your projects are consistent and well-organized.

Problem #3 Naming.  Since the project now has two things (the class and the enum) representing the same data, what should be done about naming.  The .NET API/Framework Design Guidelines is not much help here – Color is an enum only, there is no color class.  If more colors are added/created, they are NOT reflected in the .NET Color enum, you have to use the RGB values.  I  appended the enum suffix on the name of the class because it is pretty clear the intention of the files:

image

I punted – but at least I punted consistently.

After all that, I find that my code is MUCH more readable and I am writing business rules much faster and finding my logic errors much quicker. 

As a side note, do you notice that some organizations they dub “SMEs” that are just walking enums?  For example, they can recite what ICD-9 code for a broken leg off of the top of their head.  I wonder if a well-designed system replaces this kind of expert….

Optimize Searching Using Value Types

As mentioned in my blog post here, I need to write a searching algorithm of hexes adjacent to the active unit to display movable/visible/attackable hexes.  The original code was a bit of a beast across 3 functions:

 

1 public List<Hex> GetTotalAdjacentHexes(Hex currentHex, Int32 maxDistance) 2 { 3 List<Hex> hexes = new List<Hex>(); 4 List<Hex> tempList = GetAdjacentHexes(currentHex); 5 foreach (Hex hex in tempList) 6 { 7 if (!InHexList(hexes,hex)) 8 GetAdjacentHexes(currentHex, hexes, hex, tempList, 0, maxDistance); 9 } 10 return hexes; 11 } 12 13 public void GetAdjacentHexes(Hex currentHex, List<Hex> hexes, Hex hex, List<Hex> currentList, int distanceCounter, int maxDistance) 14 { 15 distanceCounter++; 16 { 17 foreach (Hex tempHex in currentList) 18 { 19 hexes.Add(tempHex); 20 if (distanceCounter < maxDistance) 21 { 22 List<Hex> tempList = GetAdjacentHexes(tempHex); 23 GetAdjacentHexes(currentHex, hexes, hex, tempList, distanceCounter, maxDistance); 24 } 25 } 26 } 27 } 28 29 public List<Hex> GetAdjacentHexes(Hex currentHex) 30 { 31 32 List<Hex> adjacentHexes = new List<Hex>(); 33 Hex targetHex = null; 34 //Above 35 int targetColumnNumber = currentHex.ColumnNumber; 36 int targetRowNumber = currentHex.RowNumber - 1; 37 targetHex = GetHex(targetColumnNumber, targetRowNumber); 38 if (targetHex != null) 39 adjacentHexes.Add(targetHex); 40 //Top Left 41 targetColumnNumber = currentHex.ColumnNumber - 1; 42 if (currentHex.ColumnNumber % 2 == 0) 43 targetRowNumber = currentHex.RowNumber - 1; 44 else 45 targetRowNumber = currentHex.RowNumber; 46 targetHex = GetHex(targetColumnNumber, targetRowNumber); 47 if (targetHex != null) 48 adjacentHexes.Add(targetHex); 49 //Bottom Left 50 targetColumnNumber = currentHex.ColumnNumber - 1; 51 if (currentHex.ColumnNumber % 2 == 0) 52 targetRowNumber = currentHex.RowNumber; 53 else 54 targetRowNumber = currentHex.RowNumber + 1; 55 targetHex = GetHex(targetColumnNumber, targetRowNumber); 56 if (targetHex != null) 57 adjacentHexes.Add(targetHex); 58 //Below 59 targetColumnNumber = currentHex.ColumnNumber; 60 targetRowNumber = currentHex.RowNumber + 1; 61 targetHex = GetHex(targetColumnNumber, targetRowNumber); 62 if (targetHex != null) 63 adjacentHexes.Add(targetHex); 64 //Bottom Right 65 targetColumnNumber = currentHex.ColumnNumber + 1; 66 if (currentHex.ColumnNumber % 2 == 0) 67 targetRowNumber = currentHex.RowNumber; 68 else 69 targetRowNumber = currentHex.RowNumber + 1; 70 targetHex = GetHex(targetColumnNumber, targetRowNumber); 71 if (targetHex != null) 72 adjacentHexes.Add(targetHex); 73 //Top Right 74 targetColumnNumber = currentHex.ColumnNumber + 1; 75 if (currentHex.ColumnNumber % 2 == 0) 76 targetRowNumber = currentHex.RowNumber - 1; 77 else 78 targetRowNumber = currentHex.RowNumber; 79 targetHex = GetHex(targetColumnNumber, targetRowNumber); 80 if (targetHex != null) 81 adjacentHexes.Add(targetHex); 82 83 return adjacentHexes; 84 }

I recently had a duh moment when I tried to optimize the code for speed (replacing hexes with tiles) – each hex has an unique row/column value that I could use to calculate distance.  In addition, instead of looping and collecting heavy reference types, I am using 2 integers in 1 loop, so the performance is dramatically improved.  Finally, since I drop from 84 lines of code to 54, I am making my code more maintainable.

 

1 public List<Tile> GetTotalAdjacentTiles(Tile startTile, Int32 maxDistance) 2 { 3 List<Tile> tiles = new List<Tile>(); 4 int startingColumn = startTile.ColumnNumber - maxDistance; 5 if(startingColumn < 0) 6 { 7 startingColumn = 0; 8 } 9 int endingColumn = startTile.ColumnNumber + maxDistance; 10 if (endingColumn > Game.CurrentBoard.NumberOfColumns) 11 { 12 endingColumn = Game.CurrentBoard.NumberOfColumns; 13 } 14 15 int startingRow = startTile.RowNumber - maxDistance; 16 if (startingRow < 0) 17 { 18 startingRow = 0; 19 } 20 int endingRow = startTile.RowNumber + maxDistance; 21 if (endingRow > Game.CurrentBoard.NumberOfRows) 22 { 23 endingRow = Game.CurrentBoard.NumberOfRows; 24 } 25 26 Tile tile = null; 27 int combinedDistance = 0; 28 for (int i = startingColumn; i <= endingColumn; i++) 29 { 30 for (int j = startingRow; j <= endingRow; j++) 31 { 32 tile = GetTile(i, j); 33 if (!tiles.Contains(tile)) 34 { 35 combinedDistance = GetDistanceBetweenTwoTiles(startTile,tile); 36 if (combinedDistance <= maxDistance) 37 { 38 tiles.Add(tile); 39 } 40 } 41 } 42 } 43 44 //Remove Starting Tile 45 tiles.Remove(startTile); 46 47 return tiles; 48 } 49 50 public List<Tile> GetAdjacentTiles(Tile currentTile) 51 { 52 return GetTotalAdjacentTiles(currentTile, 1); 53 }

The Daily WTF

I took a break from coding for the RDU Code Camp and went over to the daily WTF. This is a great WTF that I decided to dig into.

I created a project and added in an enum with the following signature:

1 public enum ApplicationTypeEnum 2 { 3 Stradegy2, 4 AdDetector, 5 MagAdvisor, 6 AdDetectorAlerts, 7 MarketAdvisorAdTel, 8 NewspaperAdvisor, 9 MarketAdvisorMarketSpender, 10 StradegyOnline, 11 AdSpender, 12 Evaliant, 13 eBooks, 14 FrenchAdex 15 } 16

I then added an Application Class and copy/pasted in the method that was in the post:

1 public class Application 2 { 3 private int SetApplicationId() 4 { 5 switch (ApplicationUserInfo.Current.ApplicationType.ToString()) 6 { 7 case "Stradegy2": return Convert.ToInt32(ApplicationTypeEnum.Stradegy2); 8 case "AdDetector": return Convert.ToInt32(ApplicationTypeEnum.AdDetector); 9 case "MagAdvisor": return Convert.ToInt32(ApplicationTypeEnum.MagAdvisor); 10 case "AdDetectorAlerts": return Convert.ToInt32(ApplicationTypeEnum.AdDetectorAlerts); 11 case "MarketAdvisorAdTel": return Convert.ToInt32(ApplicationTypeEnum.MarketAdvisorAdTel); 12 case "NewspaperAdvisor": return Convert.ToInt32(ApplicationTypeEnum.NewspaperAdvisor); 13 case "MarketAdvisorMarketSpender": return Convert.ToInt32(ApplicationTypeEnum.MarketAdvisorMarketSpender); 14 case "StradegyOnline": return Convert.ToInt32(ApplicationTypeEnum.StradegyOnline); 15 case "AdSpender": return Convert.ToInt32(ApplicationTypeEnum.AdSpender); 16 case "Evaliant": return Convert.ToInt32(ApplicationTypeEnum.Evaliant); 17 case "eBooks": return Convert.ToInt32(ApplicationTypeEnum.eBooks); 18 case "FrenchAdex": return Convert.ToInt32(ApplicationTypeEnum.FrenchAdex); 19 20 } 21 return 0; 22 } 23 } 24

I then had to tackle this line of code to get the app to compile:

1 switch (ApplicationUserInfo.Current.ApplicationType.ToString())

I created a ApplicationUserInfo class like so:

 

1 public class ApplicationUserInfo 2 { 3 public Current Current { get; set; } 4 } 5

And then a Current class like so:

1 public class Current 2 { 3 public ApplicationTypeEnum ApplicationType { get; set; } 4 } 5

Since properties can’t be static in C#, I needed to create an instance of the ApplicationUserInfo class in the Application class via a property:

1 public ApplicationUserInfo ApplicationUserInfo { get; set; }

I changed the scope of the SetApplicationId method to public and was then ready to throw a couple of unit tests on this:

1 [TestMethod()] 2 public void SetApplicationId_SetStradgy2ToApplicationUserInfo_Returns0_Passes_Test() 3 { 4 Application application = new Application(); 5 application.ApplicationUserInfo = new ApplicationUserInfo(); 6 application.ApplicationUserInfo.Current = new Current(); 7 application.ApplicationUserInfo.Current.ApplicationType = ApplicationTypeEnum.Stradegy2; 8 int expected = 0; 9 int actual = application.SetApplicationId(); 10 Assert.AreEqual(expected, actual); 11 } 12 13 [TestMethod()] 14 public void SetApplicationId_DoNotSetApplicationUserInfo_Returns0_Passes_Test() 15 { 16 Application application = new Application(); 17 application.ApplicationUserInfo = new ApplicationUserInfo(); 18 application.ApplicationUserInfo.Current = new Current(); 19 int expected = 0; 20 int actual = application.SetApplicationId(); 21 Assert.AreEqual(expected, actual); 22 } 23

The tests point out 2 WTFS immediately. The classes depend on an external variable that may or may not be instantiated. There is no argument validation. Also, note how 0 is a return value for “Stradegy2” and “None provided”. If “Stradegy2” is supposed to be the default value, it would be put into the “defult” section of the switch…case. Oh wait, no default – WTF #3.

Finally, SetApplicationId can be cleaned up with this nugget:

1 public int SetApplicationId() 2 { 3 return Convert.ToInt32(ApplicationUserInfo.Current.ApplicationType); 4 } 5

If you read the comments of the question, this was suggested in various forms (some even with the right syntax). WTF #4.