Why you should never use code from MSDN

I think part of the problem with bad code is the habit to copy/paste samples from MSDN.

For example, consider this example.

Explanatory Comments

I agree that Microsoft should put explanatory comments into the code – the code is a sample on how to do something.   The problem is that someone who doesn’t know better will copy/paste the entire block into their solution.  I think Microsoft could help its developers by putting a provision at the top of the sample:

The cynic in me would then expect to see that warning also copy/pasted into someone’s project.  Perhaps the copy/paste button on the page could filter the comments.

Big Blocks Of Code

Another problem with the MSDN code “as-is” is that the code is one big block – there are three classes put into 1 block.  It wouldn’t kill Microsoft to put each class into their own block.  The MSDN explanations could then be broken up and when you copy/paste the code, you only get 1 class at a time.  As it stands, all of the explanations is in 1 big paragraph at the top of the 1 big code sample.  A step by step approach might be more helpful – and promote better code.

Constructor Chaining

Finally, MSDN doesn’t chain the constructors of the example. 

public UrlConfigElement(String name, String url)
{
    this.Name = name;
    this.Url = url;
}

public UrlConfigElement()
{

    this.Name = "Contoso";
    this.Url = "http://www.contoso.com";
    this.Port = 0;
}

Should be

public UrlConfigElement(String name, String url)
{
    this.Name = name;
    this.Url = url;
    this.Port = 0;
}

public UrlConfigElement()
    : this("Contoso", "http://www.contoso.com")
{
    
}

That is just sloppy coding and should be fixed…

Property Assignment

I am a big fan of separation of concerns and layering when it comes to application architecture.  Using a dedicated service layer means that all client/calling layers are insulated from all dependencies past the service layer.  In a usual n-tier architecture, that means that the layer(s) calling the service layer have no idea about the data tier behind the service.

Because of that, you can not use the concrete classes from the permanent data store as the data structures that carry data among the layers.  Enter in the EF POCO generator!  Using this add-in, you get EF integrated POCOs that are already WCF-ifed.

The downside is the update (or so I thought on Friday) – because you are using a different instance between service calls, you have to rehydrate a new object, assign in the new values, and then persist.  This is very much the problem of EF classes when EF came out.

So I created a test project to see if I could use System.Reflection to help me do that assignment.  I created a project with the following two classes:

public class Person
{
    public int PersonId { get; set; }
    public String PersonName { get; set; }
}

public class Student: Person
{
    public String SchoolName { get; set; }
}

Following TDD, I created a Test Project with the following test:

[TestMethod()]
public void CreateStudentFromPerson_PersonNameAssigns_Test()
{
    Person person = new Person();
    person.PersonId = 0;
    person.PersonName = "Test";

    StudentFactory studentFactory = new StudentFactory();
    Student student = studentFactory.CreateStudentFromPerson(person);

    string expected = person.PersonName;
    string actual = student.PersonName;

    Assert.AreEqual(actual, expected);
}

I then implemented the factory:

public Student CreateStudentFromPerson(Person person)
{
    Student student = new Student();

    PropertyInfo[] personPropertyInfos = typeof(Person).GetProperties();
    PropertyInfo[] studentPropertyInfos = typeof(Student).GetProperties();

    PropertyInfo studentPropertyInfo = null;
    foreach (PropertyInfo personPropertyInfo in personPropertyInfos)
    {
        studentPropertyInfo = studentPropertyInfos.FirstOrDefault(spi => spi.Name == personPropertyInfo.Name);
        if (studentPropertyInfo != null)
        {
            object personValue = personPropertyInfo.GetValue(person, null);
            studentPropertyInfo.SetValue(student, personValue, null);
        }
    }
    return student;
}

Sure enough, the test ran green.

I then wanted to implement a generic method that would assign property values over regardless of the types in the arguments:

public Student CreateStudentFromPerson(Person person)
{
    Student student = new Student();
    return (Student)AssignObjectPropertyValues(person, student);
}

public Object AssignObjectPropertyValues(Object baseObject, Object targetObject)
{
    PropertyInfo[] basePropertyInfos = baseObject.GetType().GetProperties();
    PropertyInfo[] targetPropertyInfos = targetObject.GetType().GetProperties(); ;

    PropertyInfo targetPropertyInfo = null;
    foreach (PropertyInfo basePropertyInfo in basePropertyInfos)
    {
        targetPropertyInfo = targetPropertyInfos.FirstOrDefault(spi => spi.Name == basePropertyInfo.Name);
        if (targetPropertyInfo != null)
        {
            object basePropertyValue = basePropertyInfo.GetValue(baseObject, null);
            targetPropertyInfo.SetValue(targetObject, basePropertyValue, null);
        }
    }

    return targetObject;
}

And wouldn’t you know: Green to go <trademark pending>!

MSDN warns about the performance cost, but I have to believe that my compiler is so good that there is negligible difference.

So then there was one more requirement, all Strings needed to be encrypted.  I created another test like so:

[TestMethod()]
public void CreateStudentFromPerson_PersonNameAssignsEncrypted_Test()
{
    Person person = new Person();
    person.PersonId = 0;
    person.PersonName = "Test";

    StudentFactory studentFactory = new StudentFactory();
    Student student = studentFactory.CreateStudentFromPerson(person,true);

    string expected = "XXXXX";
    string actual = student.PersonName;

    Assert.AreEqual(actual, expected);
}

with the “XXXXX” serving as a proxy for the actual encryption.

I then overloaded the method in the factory like so:

public Object AssignObjectPropertyValues(Object baseObject, Object targetObject, Boolean useEncryption)
{
    PropertyInfo[] basePropertyInfos = baseObject.GetType().GetProperties();
    PropertyInfo[] targetPropertyInfos = targetObject.GetType().GetProperties(); ;

    PropertyInfo targetPropertyInfo = null;
    foreach (PropertyInfo basePropertyInfo in basePropertyInfos)
    {
        targetPropertyInfo = targetPropertyInfos.FirstOrDefault(spi => spi.Name == basePropertyInfo.Name);
        if (targetPropertyInfo != null)
        {
            object basePropertyValue = basePropertyInfo.GetValue(baseObject, null);
            if (targetPropertyInfo.GetType() == typeof(String) && useEncryption)
            {
                targetPropertyInfo.SetValue(targetObject, "XXXXX", null);
            }
            else
            {
                targetPropertyInfo.SetValue(targetObject, basePropertyValue, null);
            }
        }
    }

    return targetObject;
}

and had the original method chain:

public Student CreateStudentFromPerson(Person person, Boolean useEncryption)
{
    Student student = new Student();
    return (Student)AssignObjectPropertyValues(person, student, useEncryption);
}

My test ran red:

Going back to the factory, I change the if condition to this:

if (targetPropertyInfo.PropertyType.Name == "String" && useEncryption)

And I was green to go <trademark pending>.

I then realized that Bob Martin would yell at me because I have boolean arguments.  I then created an interface called IEncryptable like so:

public interface IEncryptable
{
    Boolean IsEncrypted { get; set; }
}

I then adjusted my method signatures to remove the boolean argument.

public Student CreateStudentFromPerson(Person person)
{
    Student student = new Student();
    return (Student)AssignObjectPropertyValues(person, student);
}

and adjusted the AssignObjectPropertyValues to check if the targettype implements IEncryptable:

object basePropertyValue = basePropertyInfo.GetValue(baseObject, null);
 Type targetObjectType = targetObject.GetType();
 if(targetObjectType.GetInterfaces().Contains(typeof(IEncryptable)))
 {
     if (targetPropertyInfo.PropertyType.Name == "String")
     {
         targetPropertyInfo.SetValue(targetObject, "XXXXX", null);
     }
 }
 else
 {
     targetPropertyInfo.SetValue(targetObject, basePropertyValue, null);
 }

Sure enough, the Encrypt Unit test passed but the unencrypt did not

Time to refactor!  I added in a check for IEncryptable like so

if(targetObjectType.GetInterfaces().Contains(typeof(IEncryptable)))
{
    PropertyInfo encryptedPropertyInfo = targetPropertyInfos.FirstOrDefault(tpi => tpi.Name == "IsEncrypted");
    Boolean isEncrypted = (Boolean)encryptedPropertyInfo.GetValue(targetObject,null);

    if (isEncrypted)
    {
        if (targetPropertyInfo.PropertyType.Name == "String")
        {
            targetPropertyInfo.SetValue(targetObject, "XXXXX", null);
        }
        else
        {
            targetPropertyInfo.SetValue(targetObject, basePropertyValue, null);
        }
    }
    else
    {
        targetPropertyInfo.SetValue(targetObject, basePropertyValue, null);
    }
}
else
{
    targetPropertyInfo.SetValue(targetObject, basePropertyValue, null);
}

and I was green to go.  I then noticed all of the nested if..thens and realized I should make some additional classes – EncryptedStudent perhaps.  That would cut down on the cyclomic complexity.

After all that, I was informed of the ApplyCurrentValues method.  This makes the application part of my solution useless – but not the encryption…

Signature Project: Slope of the Signature (Part 1)

Continuing the signature project, I then wanted to calculate the slope of each signature to then normalize the different slopes to determine the match % of the signatures’ scatterplots.  To that end, I needed to figure out the “center” line of each signature’s scatterplot.  Remembering the “Sum of Least Square” method from biostatistics 20 years ago, I realized that I needed to plot my best “guess of the line with 50% of the points above the line and 50% below.  I could then create the line based on this Y coordinate and the far left and right X Coordinate.  Note that I could then figure out the dispersion of the points away from this line.  Perhaps that could then tell me if someone signed the second signature more messily and were perhaps under duress.  Leaving that analysis for another day – I just wanted to figure out the center line.

Following TDD, I created a test:

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetCenterYPoint_TwoPoints_ReturnsAverage_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();
    List<Point> points = new List<Point>();
    points.Add(new Point(0, 0));
    points.Add(new Point(0, 2));

    int expected = 1;
    int actual = target.GetCenterYPoint(points);
    Assert.AreEqual(expected, actual);
}

I then wrote the GetCenterYPoint method (note the syntax to access private methods):

private Int32 GetCenterYPoint(List<Point> points)
{
    int totalYValue = 0;
    foreach (Point point in points)
    {
        totalYValue += point.Y;
    }
    return (Int32)(totalYValue / points.Count);
}

The test ran green so I tackled the Left and Right X Points:

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetLeftMostXPoint_TwoPoints_ReturnsLowest_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();
    List<Point> points = new List<Point>();
    points.Add(new Point(1, 0));
    points.Add(new Point(0, 2));

    int expected = 0;
    int actual = target.GetLeftMostXPoint(points);
    Assert.AreEqual(expected, actual);
}

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetRightMostXPoint_TwoPoints_ReturnsHighest_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();
    List<Point> points = new List<Point>();
    points.Add(new Point(0, 0));
    points.Add(new Point(1, 2));

    int expected = 1;
    int actual = target.GetRightMostXPoint(points);
    Assert.AreEqual(expected, actual);
}

The methods are pretty much what you expect:

private Int32 GetLeftMostXPoint(List<Point> points)
{
    int mostLeftXValue = points[0].X;
    foreach (Point point in points)
    {
        if (point.X < mostLeftXValue)
        {
            mostLeftXValue = point.X;
        }
    }
    return mostLeftXValue;
}

private Int32 GetRightMostXPoint(List<Point> points)
{
    int mostRightXValue = points[0].X;
    foreach (Point point in points)
    {
        if (point.X > mostRightXValue)
        {
            mostRightXValue = point.X;
        }
    }
    return mostRightXValue;
}

Those also ran green so I created the CenterLineOfPoints tests.  The first was a Horizontal line should return the same value:

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetCenterLineOfPoints_HorizontalLine_ReturnsSame_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();

    List<Point> points = new List<Point>();
    points.Add(new Point(0, 0));
    points.Add(new Point(4, 0));

    Line expected = new Line(0, 0, 4, 0);
    Line actual = target.GetCenterLineOfPoints(points);
    Assert.AreEqual(expected, actual);
}

I wrote my method:

private Line GetCenterLineOfPoints(List<Point> points)
{
    Point leftPoint = new Point();
    leftPoint.X = GetLeftMostXPoint(points);
    leftPoint.Y = GetCenterYPoint(points);

    Point rightPoint = new Point();
    rightPoint.X = GetRightMostXPoint(points);
    rightPoint.Y = leftPoint.Y;

    return new Line(leftPoint, rightPoint);
}

It ran green, so I ran the next test – two vertical lines should return the average distance between them:

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetCenterLineOfPoints_VerticalLine_ReturnsSamePointInMiddle_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();

    List<Point> points = new List<Point>();
    points.Add(new Point(0, 0));
    points.Add(new Point(0, 4));

    Line expected = new Line(0, 2, 0, 2);
    Line actual = target.GetCenterLineOfPoints(points);
    Assert.AreEqual(expected, actual);
}

That also ran green (Note that some TDDers put both of those tests in the same test method.  I don’t like to do that – even if it means red/green/refactor crosses several tests.  In any event, I then added the final test to confirm the awesomeness of my calculation.  A line at 45 degree angle should return the same line with a .5 slope.

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetCenterLineOfPoints_45degreeSlopeLine_ReturnsAverage_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();

    List<Point> points = new List<Point>();
    points.Add(new Point(0, 0));
    points.Add(new Point(4, 4));

    Line expected = new Line(0, 0, 4, 4);
    Line actual = target.GetCenterLineOfPoints(points);
    Assert.AreEqual(expected, actual);
}

And I ran it and got RED!  Crap!

I am doing something wrong in my calculation.  It was pretty obvious, the Y point for that center line is NOT the same for both points.  If it was, the slope would always be 0.0. What I need to do was channel my inner Gauss and use his formula for calculating SLS.  I then binged around and found this great step by step article to the finding the center line.

Forgetting TDD for a second, I attempted to implement the formula using C#.  I came up with this:

private Line GetCenterLineViaSumOfLeastSquares(List<Point> points)
{
    Int32 sumOfXValue = 0;
    Int32 sumOfYValue = 0;
    Int32 sumOfXValueSquared = 0;
    Int32 sumOfXValueMultipledByYValue = 0;
    Int32 numberOfPoints = 0;

    foreach (Point point in points)
    {
        sumOfXValue += point.X;
        sumOfYValue += point.Y;
        sumOfXValueSquared += point.X ^ 2;
        sumOfXValueMultipledByYValue += point.X * point.Y;
        numberOfPoints ++;
    }
    Double xMean = sumOfXValue / numberOfPoints;
    Double yMean = sumOfYValue / numberOfPoints;

    Double numerator = sumOfXValueMultipledByYValue - ((sumOfXValue * sumOfYValue) / numberOfPoints);
    Double denomiator = sumOfXValueSquared - ((sumOfXValue ^ 2)/numberOfPoints);
    Double slope = numerator / denomiator;
    Double yIntercept = yMean - (slope) * sumOfXValue;

    Point startPoint = new Point(0, (Int32)yIntercept);
    Point endPoint = new Point(0, 0);

    return new Line(startPoint, endPoint);
}

Note that I didn’t know how to calculate the Xaxis intercept yet, so I left it as 0,0.  Some of you might ask why did I do this

Double xMean = sumOfXValue / numberOfPoints;

When I could have done this?

Double xMean = sumOfXValue / points.Count;

In a word.  Readability.  Using explanatory variables does not waste any meaningful processing time, and the code is more readable – it matches the mathematical formula’s text.

In any event, I then realized that I don’t need a line to represent the middle line – I just needed the slope.  And lookie right there – I have a variable called… slope.  I changed the method to return the slope of the middle line:

private Double GetSlopeOfScatterplot(List<Point> points)
{
    //Bunch of code
    return numerator / denomiator;

}

Jumping back to TDD, I realized I need a bunch of tests to verify that my slope calculation is correct.   I deleted all of my prior work – thank goodness for source control and here is what I came up with:

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetSlopeOfScatterplot_HorizontalLine_ReturnsZero_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();

    List<Point> points = new List<Point>();
    points.Add(new Point(0, 2));
    points.Add(new Point(1, 2));
    points.Add(new Point(3, 2));
    points.Add(new Point(4, 2));

    Double expected = 0.0;
    Double actual = target.GetSlopeOfScatterplot(points);
    Assert.AreEqual(expected, actual);
}


[TestMethod()]
[ExpectedException(typeof(DivideByZeroException))]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetSlopeOfScatterplot_VerticalLine_ReturnsUndefined_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();

    List<Point> points = new List<Point>();
    points.Add(new Point(2, 0));
    points.Add(new Point(2, 1));
    points.Add(new Point(2, 2));
    points.Add(new Point(2, 3));

    Double expected = 0.0;
    Double actual = target.GetSlopeOfScatterplot(points);
    Assert.AreEqual(expected, actual);
}

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetSlopeOfScatterplot_45DegreeLine_ReturnsOne_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();

    List<Point> points = new List<Point>();
    points.Add(new Point(0, 0));
    points.Add(new Point(1, 1));
    points.Add(new Point(2, 2));
    points.Add(new Point(3, 3));

    Double expected = 1.0;
    Double actual = target.GetSlopeOfScatterplot(points);
    Assert.AreEqual(expected, actual);
}

The thing – The vertical line is coming back as 0 – when it should be divideByZero.  Crap X 2!

!

I then went back and looked at my code and I realized I made a rookie mistake!

sumOfXValueSquared += (point.X ^ 2);

is wrong and

sumOfXValueSquared += (point.X * point.X);

is right.  I then got this exception:

NaN.  Wahoo!!!!  I thought I would get a DividByZeroException, but I was wrong.  I then used the Double.Nan function in that unit test

[TestMethod()]
[DeploymentItem("Tff.Signature.Comparison.dll")]
public void GetSlopeOfScatterplot_VerticalLine_ReturnsUndefined_Test()
{
    ScatterplotComparisonFactory_Accessor target = new ScatterplotComparisonFactory_Accessor();

    List<Point> points = new List<Point>();
    points.Add(new Point(2, 0));
    points.Add(new Point(2, 1));
    points.Add(new Point(2, 2));
    points.Add(new Point(2, 3));

    Boolean expected = true;
    Boolean actual = Double.IsNaN(target.GetSlopeOfScatterplot(points));
    Assert.AreEqual(expected, actual);
}

and I was “green to go” <trademark pending>

Now I can compare 2 signatures and analyze the slope between them.