Testing your sharepoint repositories

In this article we will see how to test your sharepoint repositories, as we said before writing unit test for them isn’t easy. I don’t think you can test drive them or that it would help, at the very least it would be too slow.

Therefore SHP repositories are a weak point in your testing strategy, but at the very same time they are the base on which the rest of your application sits on. So it is very important that you do two things:

  1. Write KISS repositories
  2. Write integration tests for them

KISS: Keep It Simple Stupid

Your repositories need to be simple, strip them of any logic that has a reason to be somewhere else. Any constraint that isn’t strictly related to the persistent layer should be checked by another layer.

They should receive an entity from above and persist it like it is, if the entity comes with errors those errors will be persisted (or the layer bellow will throw an exception).

Expose a basic API. Instead of a method Save that check if the entities exists and then decides whether to update it or create a new entity expose three basic methods (Exists, Create, Update) and let the layer above make that decision.

Once you have a KISS repository you need to secure it the best you can. This is done by writing tests integrated with sharepoint. Usually to do this you will have to do the following things:

  1. Create a test site collection
  2. Provision your artifacts (Fields, ContentTypes, Lists…)
  3. For each test
    1. Populate one or more list
    2. Run a test
    3. Clean up
  4. Delete the site collection

This is obviously a very slow process so you won’t run this tests in your normal TDD cycle, but you can run them frequently enough that you will catch most bugs before they can cause any major problem.

Now let’s put aside the theory and write some code, starting from the repository of the previous article

    private const string WebAppUrl = "http://localhost";
    private const string SiteUrl = "http://localhost/sites/Test";
    private IPersonRepository repository;
    private SPSite site;

    [SetUp]
    public void SetUp()
    {
      site = CreateSite();
      repository = new PersonRepository(site.RootWeb);
    }

    [Test]
    public void WhenAddingAPerson__ItShouldAddAPerson()
    {
      var list = site.RootWeb.Lists["Person"];
      CleanList(list);

      var person = AddPerson();

      AssertCreation(list, person);
    }

    private SPSite CreateSite()
    {
      var webapp = SPWebApplication.Lookup(new Uri(WebAppUrl));

      if (SPSite.Exists(new Uri(SiteUrl)))
        webapp.Sites.Delete(SiteUrl);

      var site = webapp.Sites.Add(SiteUrl, "contoso\\admin", "admin@contoso.com");
      site.Features.Add(new Guid("<GUID>"));
      return site;
    }

    private void CleanList(SPList list)
    {
      while (list.Items.Count > 0)
        list.Items[0].Delete();
    }

    private Person AddPerson()
    {
      var person = new Person
      {
        Address = "Address",
        BirthDate = DateTime.Now,
        Name = "Name"
      };
      repository.Create(person);
      return person;
    }

    private void AssertCreation(SPList list, Person person)
    {
      Assert.That(list.Items.Count, Is.EqualsTo(1));
      var item = list.Items[0];
      Assert.That(item["Address"], Is.EqualsTo(person.Address));
      Assert.That(item["Title"], Is.EqualsTo(person.Name));
      Assert.That(item["DateOfBirth"], Is.EqualsTo(person.BirthDate));
    }

Here we see a setup method that creates (or recreates) a site collection at every test run. To speed up the tests a bit, you can move that operation to a Fixture Setup, but that might mean that some tests pass or fail depending on the order of execution, so proceed with care. During the site creation we also enable one (or more) features that provision our sharepoint artifacts.

We then have the test method that checks that the Create method of the repository works fine by:

  1. Removing any item present on the list
  2. Calling the Create method with a valid person
  3. Checking (via standard object model) that was added an item to the expected list with the required properties.

In the same way one might test a Get method. First you clean the list, than you add the expected item (via SSOM), than you get it with your repository and assert that everything is fine. Try it doing it yourself and let me know if you have any problem.

Repository Pattern

If you have ever heard of software design patterns, you have heard of the Gang of Four and their book. It’s the bible on this matter and it would be a disservice not to reference it, but the pattern we will talk about is missing from this book. You instead can find more information here.

In short the repository is an object that sits between your application data and your business logic. It helps (among other things) remove duplicated code, improve testability and provide a strong typed business entity that helps catch errors at compile time.

Repository are used to isolate from data source like databases, file system or a Sharepoint list, and from other difficult to integrate objects like web services. When all your business logic deals with data retrieval or persistence through a repository, you can test your business logic in isolation replacing the real repository with a test double.

To have a better understanding of what’s going on let’s have a look at some code

    class Person
    {
        public int Id { get; set; }
        public string Name { get; set; }
        public DateTime BirthDate { get; set; }
        public string Address { get; set; }
    }

    interface IPersonRepository
    {
        Person Get(int id);
        void Create(Person person);
        void Update(Person person);
        void Delete(int id);
    }

    class PersonRepository : IPersonRepository
    {
        private readonly SPList list;
        private readonly PersonSPListItemMapper mapper;

        public PersonRepository(SPWeb web)
        {
            list = web.Lists["Person"];
            mapper = new PersonSPListItemMapper();
        }

        public Person Get(int id)
        {
            SPListItem item = list.GetItemById(id);
            Person person = mapper.From(item);
            return person;
        }

        public void Create(Person person)
        {
            SPListItem item = list.AddItem();
            mapper.Populate(item, person);
            item.Update();
        }

        public void Update(Person person)
        {
            SPListItem item = list.GetItemById(person.Id);
            mapper.Populate(item, person);
            item.Update();
        }

        public void Delete(int id)
        {
            SPListItem item = list.GetItemById(id);
            item.Delete();
        }
    }

    class PersonSPListItemMapper
    {
        public Person From(SPListItem item)
        {
            return new Person
            {
                Name = item.Title,
                Id = item.ID,
                BirthDate = (DateTime)item["DateOfBirth"],
                Address = (string)item["Address"]
            };
        }

        public void Populate(SPListItem item, Person person)
        {
            item["Title"] = person.Name;
            item["DateOfBirth"] = person.BirthDate;
            item["Address"] = person.Address;
        }
    }

Here we see, in oder, the business entity we are interested on (Person), with a few basic properties, then the interface of our repository with a few basic CRUD operations, then we implement said interface against a sharepoint list, at the end we have a utility class that’s meant only to map to and from a sharepoint item.

The repository pattern is quite simple to comprehend, what’s important is that:

  • You keep following it
  • Every object in the business layer interacts with the entities through a repository
  • Every interaction is isolated by an interface

The following is a business layer object that expects a repository in its contructor and uses it to create a Person.

    internal class PersonManager
	{
		IPersonRepository repository;
		public PersonManager(IPersonRepository repository)
		{
			this.repository = repository;
		}
		public void Create(string name, string address, DateTime birthDate)
		{
			Person person = new Person
			{
				Name = name,
				BirthDate = birthDate,
				Address = address
			}
			repository.Create(person);
		}
	}

As you can see this class has no knowledge of what is Sharepoint and can be tested quite easily by passing a spy to the constructor instead of the real implementation.

All operations to persist a business entity pass through a repository, and any data retrieved by a query comes from it. In my example you can see a simple get by id query, but more complex queries are allowed. You can implement them in two ways:

  • The repository has a method for each query
  • The repository receives a query object that contains the required information to build a query

Another important advantage of using the repository pattern is that it can provide a centralized caching layer to for the rest of the application. This way if a query has already been served the repository can return a cached result set without having to muck the details of the business layer. This is quite useful when dealing with repositories that mask web services.

In the end, repository help isolate data access code, from the rest of your application, it provides an insertion point to test your business layer but adds another abstraction layer so it can be difficult to understand for developers in your team whom are unfamiliar with this pattern. Also keep in mind that repositories tend to be hard to unit test, so it is better to write integration test for them.

How To Test Your Production Code: Using Fake Objects

In the last post we talked about Mock Objects and what are the difference between them and Spies. We have almost completed our journey through the all the different types of Test Doubles, in this post we are going to introduce the last one: the Fake Object. 

There are sometimes where the need of isolation comes from the fact that your tests are running too slow because you are accessing services that need to read or write to the disk or make a network call. In this cases, the best way to improve the test is to use a Test Double that only mimics the same functionality as the service you are currently using. Of course, you need a way to swap the objects during the test, but the techniques to be sure that you can inject your Test Doubles are going to be discussed in a future post.

Let’s make an example

Let’s pretend that you are developing a method that prepares different kind of SMS messages and then sends them using a service that returns an id for the message so you can query it back and check the processing status of the message. The service also stores all sent messages in a queue for later use and the queue has to be manually emptied.

class SmsManager
{
  public IMessageService SMSService { get; set; }
  public void SendSMS()
  {
    string message = PrepareSMS();
    Guid key = SMSService.Send(message);

    while (!SMSService.Sent(key))
    {
      Thread.Sleep(100);
    }
    SMSService.Delete(key);
  }

  private string PrepareSMS()
  {
    return String.Empty;
  }
}

Maybe we can configure the service to avoid sending the SMS message while we are in test, but we still have to wait for the whole round trip to happen, causing a sensible delay in getting the results from the test, and if we have to run lot’s of tests  those delays will start to add up. After a while, you end up with tests that take you minutes to run and you stop running them often.

Uncle Bob always talks about how he and his team developed FitNesse without needing a database. But how do they tested the functionalities that dealt with persistent data? well, they isolated the persistence layer and they faked it with an in-memory representation of it.

This is what Fake Objects are for: you swap slow external dependencies of your SUT with the Fake Object and you use them in your test as if they were real.

Can It Be THAT Simple?

Well, it depends. The problem is that Fake Object must have the same functionality as the real one, at least all the functionality the SUT uses, but implemented in a simpler way; for example, our fake SMS service could be implemented like this:


public interface IMessageService
{
  Guid Send(string message);
  bool Sent(Guid key);
  void Delete(Guid key);
}

public class FakeSmsService : IMessageService
{
  Dictionary<Guid, string> inmemorydb = new Dictionary<Guid, string>();
  public Guid Send(string message)
  {
    var guid = new Guid();
    inmemorydb.Add(guid, message);
    return guid;
  }
  public bool Sent(Guid key)
  {
    return inmemorydb.ContainsKey(key);
  }
  public void Delete(Guid key)
  {
    inmemorydb.Remove(key);
  }

}

Internally the fake service retains a dictionary of key-value pairs where it saves all the messages that the method Send receives. Even though it’s pretty simple, this implementation is powerful because we can trust that the SUT will believe that it’s using the SmsService that makes the HTTP calls and all that, but in fact, all the operations stay in the memory of the running process, so it’s waaay faster.

Could you give an example of a Fake Object recently used?

In one recent project we used Effort to isolate us from the database.

We were using Entity Framework code first to create the database and then access it, and we discovered this Effort that was an Entity Framework Provider: it implemented the same interfaces and functionality of EF but it did it in an in-memory database so we didn’t have to worry about cleaning the DB in the TearDown phase of our test, we simply destroyed it and created it again, and it was super-fast.

What to do next?

Find a simple functionality on the code you are maintaining that uses an external service and start implementing a fake object based on that functionality; then use it in a test suite to cover your method.  If the service is huge and with lots of functionality, don’t get overwhelmed, start small, just a simple implementation that helps you test the method you decided to cover.

 

After reading this series of posts you have no more excuses, now set up your environment and start testing the heck out of your code!

Most importantly help a friend by explaining him the difference between mocks and spies and if he doesn’t understand, send him the link to this post.

How to test in sharepoint

We have talked about how to set up your test environment, how to write tests and what to do with the freedom that the test give you (write code that is clean). Now we start to get into the nitty gritty of being a code cleaner, we need to start to clean some legacy code. I’m young, so probably my experience is not the same as most professional code cleaners, but when I think of legacy code, I think of sharepoint programming.

Programming with sharepoint is hard, you need to know how to work your way around many weird behaviours is sharepoint like the fact that in a field XML schema  False != FALSE.

But you get used to that stuff. The biggest problem with sharepoint is that it wasn’t built with the idea of enabling testability. No, most interesting classes are sealed (can’t be extended), and a value that can be approximated with 100% of the classes doesn’t inherit from an interface. You have to pass around concrete classes all around your code. This is a problem because you can’t replace an SPListItem with a test double. And to make matters worst, the biggest advantage of using sharepoint is also its biggest testability problem. Sharepoint is a monster, it has every functionality you could think of: CMS, Document management, Search, OCR, Cache, line of business integration, social netnork, blog, forum, ecc. It wants you to use its features, it wants you to integrate with it, but if you do in a naive way you are lost. There’s no way you will be able to TDD your application.

Luckily there’s a better way, its hard work, specially in the beginning when you need to change your frame of mind and the way you work, but there’s a better way, and I’m here to guide you.

We will start with a brief detour talking about a building block, the repository pattern, then we will see:

  • How to test your repositories
  • How to test your business logic
  • Sharepoint tdd tips and tricks
  • Why remote provisioning helps
  • How to do remote provisioning
  • Building repositories with SPMetal
  • More ways to isolate from the framework

How To Test You Production Code: Using Mock Objects

By using Test Spies we can capture all the indirect outputs of our software to check that it is working correctly. Often times, making the assertions for all the outputs in the testing function leads to lot’s of duplicated code that after a while starts to hinder the purpose of the tests. Sometimes, we would like to do our assertions in the context of the running code and not in the context of the test suite.

Think for example that one of the indirect outputs of your SUT is a disposable object. How you verify that it is in the correct state when it get’s injected to your dependency if it gets destroyed as soon as the method you are testing exits?

Let’s try to understand this problem with an example: let’s say we want to verify that the method CreateAndConfigureDisposable passes the correct object to DependencyObject:


public interface IScanner
{
  void SetStream(FileStream fileStream);
}

public class FileLoader
{
  private readonly IScanner _scanner;

  public FileLoader(IScanner scanner)
  {
    _scanner = scanner;
  }
  public void LoadStreamAndPosition(string path)
  {
    using (var s = new FileStream(path, FileMode.Create))
    {
      s.Position = 500;
      _scanner.SetStream(s);
    }
  }
}

We could write a test method using a Test Spy and it will look like this:

public class ScannerSpy : IScanner
{
  public FileStream SpyStream { get; set; }
  public void SetStream(FileStream stream)
  {
    SpyStream = stream;
  }
}
[Test]
public void TestDisposableOutput1()
{
  // Arrange
  var scannerSpy = new ScannerSpy();
  var loader = new FileLoader(scannerSpy);
  // Act
  loader.LoadStreamAndPosition(@"c:\teststream.txt");
  // Assert
  Assert.AreEqual(500, scannerSpy.SpyStream.Position);
}

this test looks correct, we configure the SUT, we pass in the TestSpy, we exercise the method we want to test and the we do the assertions. The problem is that as soon as we exit the using block inside LoadStreamAndPosition the disposable object gets, well, disposed and trying to access its state will cause an ObjectDisposedException.

The best way to test this code is to do the assertions in the context of the running code that we want to test. This is where Mock Objects come into play: you put all the assertions inside the Test Double’s method that receives the indirect output.

public class ScannerMock : IScanner
{
  public long Position { get; set; }
  public void SetStream(FileStream stream)
  {
    Assert.AreEqual(Position, stream.Position);
  }
}

With this new Test Double the assertions run inside the using block while the DisposableObject still exists. By introducing the Mock Object the test logic flow changes quite a bit, the assertion step is missing and is substituted by a step for configuring the Test Double.

[Test]
public void TestDisposableOutput2()
{
    // Arrange
    var scannerSpy = new ScannerMock();
    scannerSpy.Position = 500;
    // Act
    var loader = new FileLoader(scannerSpy);
    loader.LoadStreamAndPosition(pathToTestStream);
}

So… what’s the big difference then between Spy Objects and Mock Objects

With a Mock Object you mock an object and with a Spy you spy on it 😉

Just kidding, I read a similar explanation while researching this topic.

The real explanation is that spies are used to collect information about the behavior of the code and then do all the assertions in the testing context while mocks may collect some information (like how many times a method gets called) but all the assertion are done in the context of the running code.

This distinction is important for a few reasons of which this are the two most important in my opinion:Some indirect outputs may not exists outside of the SUT context;

  • Some indirect outputs may not exists outside of the SUT context;
  • The code you are trying to test may swallow the exceptions generated by the assertions;

The first one is a case when you are forced to use a mock object because you need to do the assertions while the code is still running; we have already discussed this problem so let’s check the second one.

Let’s pretend that

public void LoadStreamAndPosition2(string path)
{
  try
  {
    var s = new FileStream(path, FileMode.CreateNew);
    s.Position = 500;
    _scanner.SetStream(s);
  }
  catch
  {
    //Gotta catch 'em all!
  }
}

In this case, we instantiate a non-disposable object that will live after the method LoadStreamAndPosition exists; We still could use a Mock Object if it wasn’t for the Pokémon Exception Handling that is going to catch the exceptions generated by the testing framework hiding all the failing assertions and making us think that the test run correctly.

Clean Errors – Clean Code

If you write feature code, you also have to write error handling code. There’s no clean code without clean error handling, if you don’t do it, your user has to deal with it in the form of your application crashing.

There are a few simple rules you should follow to, starting with using exceptions rather than return codes, we already talked about it when dealing with clean functions. Using unchecked exceptions let’s you concentrate in the flow of your code without interweaving it with a myriad of if-else statements to check every return code (or worse forget to do it and propagate an error state).

You should throw your exception thinking about how your caller will deal with it. So you need to provide context about the error in the message of the exception, and don’t litter your caller with very specific exception classes, if all of them will be dealt with in the same way (log and show error).

Remember that exceptions should be used to handle the weird flow not your normal flow. Don’t use them like if statements. If you have to integrate with a library that does just that wrap it and expose a sane interface that the rest of your system can deal with.

Avoid nulls like a plague. This probably deserves a whole article but to give you the gist of it, don’t return null, return something else, like an empty list or a special case object. Don’t pass null as a parameters. You don’t want to deal with it neither the writer of the function you are calling (likely yourself), don’t pass a null as an argument, unless it’s a function of a third-party library.

How To Test Your Production Code: Test Spy

By using Dummy Objects and Test Stubs, we are able to control the inputs of the System Under Test to exercise every part of it during our tests. But of course, not every method that we want to test is going to have one just output or is going to change only the internals of the class that is under test. Most methods have side effects and are going to call other libraries or methods of other objects and we want to be sure that those method calls happen the right amount of times and with the correct parameters.

Let’s suppose we have a method that prepares an invoice when someone check’s out at a hotel. That method is part of a Registration class that takes in a Room and a Guest object and the BookingInformation with all the information about the booking period, like check-in and check-out dates number of people etc.

public class Registration
{
  private Room _room;
  private Guest _guest;
  private BookingInformation _booking;
  private readonly IPrinter _printer;
  public Registration
          (Room room, Guest guest,
          BookingInformation booking, IPrinter printer)
  {
    _booking = booking;
    _guest = guest;
    _room = room;
    _printer = printer;
  }
  public void CheckOut()
  {
    Dictionary<string, string> invoiceInformation =
        PrepareInvoiceData();
    _printer.Print(invoiceInformation);
  }

  private Dictionary<string, string> PrepareInvoiceData()
  {
    ////
  }

}

 

Then, when the method checkout gets called, it prepares a dictionary of strings and passes it to a library that prints the invoice. This dictionary an indirect output of our method and we want to check if it’s properly set, while also avoiding calling the real method that sends the file to the printer.

We notice that the a printer object is passed in the constructor of the class and that it implements the interface IPrinter. That means we could pass a special implementation of that interface and log the calls and the inputs passed to its methods.

public class PrinterSpy : IPrinter
{
  public int PrintCallCount = 0;
  public Dictionary<string, string> InvoiceParameters;
  public void Print(Dictionary<string, string> parameters)
  {
    InvoiceParameters = parameters;
    PrintCallCount++;
  }
}

In particular we want to capture the dictionary that is passed to the print method and expose it as a public field to do the assertions. We could also check that we are calling the method just once, or that we are not calling it for when an error occurs.

[Test]
public void CheckPrintMethodIsCalledWithTheCorrectConfiguration()
{
  // Arrange
  var guest = new Guest
  {
    Name = "GuestName",
    Surname = "GuestSurname",
    Id = 1,
    Phone = 111111111,
    Address = "SomeAddress",
    City = "SomeCity",
    Country = "SomeCountry"
  };
  var room = new Room
  {
    Number = 1,
    Type = RoomTypes.Suite,
    Price = 1
  };
  var booking = new BookingInformation
  {
    CheckInDate = new DateTime(2016, 1, 1, 14, 0, 0),
    CheckOutDate = new DateTime(2016, 1, 2, 10, 0, 0),
    NumberPpl = 1,
  };

  // Act
  var printer = new PrinterSpy();
  var registration = new Registration(room, guest, booking, printer);
  registration.CheckOut();

  // Assert
  var output = printer.InvoiceParameters;
  Assert.AreEqual(1, printer.PrintCallCount);
  Assert.AreEqual("GuestSurname GuestName",
                    output["GuestName"]);
  Assert.AreEqual("111-111-1111", output["GuestPhone"]);
  Assert.AreEqual("SomeAddress, SomeCity (SomeCountry)",
                  output["GuestAddress"]);
  Assert.AreEqual("01/01/2016", output["CheckInDate"]);
  Assert.AreEqual("02/01/2016", output["CheckOutDate"]);
  Assert.AreEqual("Suite", output["RoomType"]);
  Assert.AreEqual("1 Day", output["NumberOfDays"]);
  Assert.AreEqual("1.00 €", output["TotalCost"]);
}

As you can see, in this test we pass to the Registration constructor the Room, the Guest and the BookingInformation objects properly configured and then we pass also a our Test Double that will get called and will record it’s inputs. After the method CheckOut gets called we have complete access to the information that it prepares and then passes over to the Print method. The SUT (System Under Test) doesn’t know that it’s talking to a test double so if the test passes we are sure that the method will work correctly in production when it will pass the dictionary to the object that will in fact print the invoice.

It’s not always that easy!

Sure, if you are working with some legacy code, chances are that your class doesn’t have the dependency that receives the indirect output nicely injected in the constructor. You could have something like this:

public class Registration2
{
  private Room _room;
  private Guest _guest;
  private BookingInformation _booking;

  public Registration2
    (Room room, Guest guest,
      BookingInformation <span class="hiddenGrammarError" pre=""><span class="hiddenGrammarError" pre="">booking)
  {
    _booking</span></span> = booking;
    _guest = guest;
    _room = room;
  }
  public void CheckOut()
  {
    Dictionary<string, string> invoiceInformation =
      PrepareInvoiceData();
    /////////////////////////////////////////////
    Printer printer =
      new Printer( /* PUT HUGE LIST OF PARAMETERS HERE */);
    /////////////////////////////////////////////
    printer.Print(invoiceInformation);
  }

  private Dictionary<string, string> PrepareInvoiceData()
  {
    ////
    /// 
    return null;
  }

}

How could you test the indirect output now?

Depending on the code base and how confident you are in refactoring it you could try different strategies like extracting the ugly part of your code to a protected method and then extend the class with a special that you use class just for testing. Another way to address this problem is to isolate your code wrapping the external dependency in a class that you can override like this:

internal interface IPrintWarpper
{
  void Print(Dictionary<string, string> invoiceInformation);
}
internal class PrintWrapper : IPrintWarpper
{
  private Printer printer;

  public PrintWrapper()
  {
    Printer printer =
      new Printer( /* PUT HUGE LIST OF PARAMETERS HERE */);
  }
  public void Print(Dictionary<string, string> invoiceInformation)
  {
    printer.Print(invoiceInformation);
  }
}

Now you have isolated the dependency and have an interface that you can implement with your Test Spy

Now it’s your turn

Take a small function that has just one well-defined indirect output extract it and test the behavior YOUR code.

Also, don’t forget to share this post with your friends that have problems testing their code!

Clean Formatting – Clean Code

You just need to have a look at any minified JS file to realize how important proper formatting is.I don’t think any one would be able to program a whole system constantly minified, but most don’t give a concerned effort in improving the formatting of their code base, not even using the autoformatting features of the IDEs.

So in order of effort:

  1. If you are using Visual Studio type periodically Ctrl+K, Ctrl+D. This will auto format the current file giving you a nice starting point
  2. Install Code Maid. It’s a nice Visual Studio extension that helps formatting your code, you can configure it to do many little things, like grouping methods by its visibility (public, protected, private), removing unused using statements. It’s more powerful than the standard reformatting, and can also be configured to clean up at every save.

Code Maid goes a long way in improving your formatting, but when working on a team, it’s important that most of the team members use it, if not on auto mode, at least before any check-in. Otherwise, the commits done by the team members that use it risk being dirty with a lot of reformatting.

One thing I wasn’t able to do with code maid was enforce the news paper order to my methods. What’s that? Well, you see, a news paper article is organized in a way that first it gives you broad idea, then it goes a little bit in detail, then a little more, and then you get to the full picture.

In code you have high level functions calling low-level functions. So the same principle should apply. At the top of the file you should find the high level methods, then you should go down a level, and keep going till it the end.

Code maid isn’t smart enough to look at who calls who and order the methods with that in mind, but you have a brain and should use it 🙂

Other rules you should keep in mind are:

  • Add vertical spacing between concepts
  • Group vertically related concepts
  • Declare local variables close to their use
  • Declare instance variable at the top
  • Caller function should be close to callee and should come first

As always dealing with smaller files is easier to apply this rule. As a general guideline, Uncle Bob suggest keeping them at about 200 lines, with a maximum of  500 LoC.

In the same vein he suggest to set a maximum of 100-120 characters for a line of code, while having most of it being less than 60.

indentation is really important to help legibility, so don’t collapse a scope even if it is possible.

 

if(value == null) return;
//...

While it is a valid statement the collapsed block might be overlooked by a reader wasting his time.

if(value == null)
    return;  //This is more readable
//...

Remember, trying to trick the 5 lines maximum rule for a function by collapsing statements only hinders the readability of your source file.

One last note, a project should look like it was written by a single individual, it should look consistent. Is not acceptable to have different parts of it have different coding styles. The team rules, so at the start gather your group and decided on which rules to follow, than follow them.

How to Test your Production Code: Using Test Stubs

In our last post we explored how to use Dummy Objects in order to test our code when we are dealing with objects that are difficult to create. One important restriction on the usage of this kind of test double is that we have to avoid touching the dummy object during our tests. We can easily check that LoadInvoicesByDate throws when from is higher that threshold:

public string WarningMessage;
public IList<Invoice> Invoices;
public void LoadInvoicesByDate(DateTime from, DateTime to, DateTime threshold, IDbFactory dbFactory)
{
  if (from > threshold)
    throw new DateOverThresholdException("from");

  var count = dbFactory.CountInvoices(from, to);
  if (count == 0)
  {
    WarningMessage = "No Invoices in given rage";
    return;
  }
  Invoices = dbFactory.GetInvoices(from, to);
}

But what happens if we have to pass the first ‘if’ and check that the text of WarningMessage is updated when count equals zero?

In this case the variable count is said to be an indirect input of the code we want to test and in order to control it’s value we need to instantiate a special version of dbFactory that we have under our control. Of course we don’t want to use the real instance of DbFactory because who knows the state of the database… we could never get a count that equals 0. So in our test we are going to need a Test Double called, you guessed it, Test Stub.

The Test Stub will provide you with the indirect input you need no matter what how it gets called. Let’s explain with a test that checks  that the WarningMessage field is set when dbFactory.CountInvoices returns zero:

private class DbFactoryStub : IDbFactory
{
  public int CountInvoices(DateTime from, DateTime to)
  {
    return 0;
  }
  // other IDbFactory methods
}
[Test]
public void WarningMessageWhenCountIsZero()
{
  IDbFactoy dbFactory = new DbFactoryStub();
  invoicesManager.LoadInvoicesByDate(
      DateTime.Now, DateTime.Now, DateTime.Now,
      dbFactory);
  Assert.AreEqual("No Invoices in given rage",
                  invoicesManager.WarningMessage);
}

What’s happening here?

First I create a new class that implements IDbFactory and set the return value of CountInvoices to zero, which is exactly the return value we want to have. With this test double we are forcing our code through a path that could have been difficult to simulate with the real implementation.

Moreover, we can generalize a bit our dummy class and set the wanted return values in the tests

private class DbFactoryDummy : IDbFactory
{
  public int DummyInvoicesCount;
  public IList<Invoice> DummyInvoices;
  public int CountInvoices(DateTime from, DateTime to)
  {
    return DummyInvoicesCount;
  }
  public IList<Invoice> GetInvoices(DateTime from, DateTime to)
  {
    return DummyInvoices;
  }
}

[Test]
public void WarningMessageWhenCountIsZero()
{
  IDbFactoy dbFactory = new DbFactoryDummy();
  dbFactory.DummyInvoicesCount = 0;
  invoicesManager.LoadInvoicesByDate(
      DateTime.Now, DateTime.Now, DateTime.Now,
      dbFactory);
  Assert.AreEqual("No Invoices in given rage",
                  invoicesManager.WarningMessage);
}

[Test]
public void InvoicesSetGetInvoicesReturnedValue()
{
  IDbFactoy dbFactory = new DbFactoryStub();
  dbFactory.DummyInvoicesCount = 1;
  dbFactory.DummyInvoices = new List<Invoice> { new Invoice() };
  invoicesManager.LoadInvoicesByDate(
      DateTime.Now, DateTime.Now, DateTime.Now,
      dbFactory);
  Assert.AreEqual(string.Empty,
                  invoicesManager.WarningMessage);
  Assert.AreEqual(dbFactory.DummyInvoices,
                  invoicesManager.Invoices);
}

With this change to DbFactoryDummy we are able to configure its return values while testing in order to use the same dummy class for different test cases.

Remember that our goal isn’t that of testing the behaviour of the particular implementation of DbFactory but how our code behaves when stimulated with different indirect inputs.

What if we want to test the indirect outputs of the system? for that we use Mock Objects, that will be explained in the next post.

Clean Comments

One of the most interesting things I’ve found out about people arguing on the internet is related to code comments. Every time there’s an article about how to properly comment code it is followed by a heated discussion in the comment section. Two sides. One saying that comments are really, really important, and the other saying the exact opposite. Comments should be avoided when possibile.

What I’ve realized is that your stance on comments depends heavily on your experience, and it isn’t a linear progression from NO COMMENTS to PRO COMMENTS (or viceversa), it is more of a wave function. You usually start in the NO COMMENTS camp, when you are alone writing code for fun, then you go all the way to the PRO COMMENTS when you start dealing with other peoples code (or your old code) and spending hours trying to understand a function, and wish that someone had commented it. Then suddenly you go to the NO COMMENTS again when you have to deal with old lying comments that mislead you, and realize that developers won’t update the comments as the code changes. Then (at least this is where I’m now, who knows tommorow what I’ll think) you find some middle ground where you avoid comments where possibile and write them when you fail to write code that’s self documented.

That last phrase “self documented code” is used by people on the NO COMMENTS side, both experienced and not experienced programmers, so people on PRO COMMENTS think that when arguing about comments they are dealing with noobies, and maybe they don’t even know that exist experts whom are one the NO COMMENTS camp. These guys on the other hand have a really hard time breaking that first impression and everything they say just lumps them more no the noobie group.

There. Internet wars explained. Now let’s stop ranting and let’s see how to write good comments and what comments we should avoid.

First a  general guideline. I can’t recall where I read it but the best rule of thumb for comments I’ve found is this

Don’t write comments that say what the code does. Write comments that explain why.

I’ve seen comments like this, and I immediatly delete them

string x = start + end; //Concatenate start and end
int y = a + 1; //increment a

Those comments don’t help in any way. They just say in more words exactly what the code is alreday saying. They are noise, and the human brain is really good ad ignoring noise. I don’t want my brain ignoring comments, because comment can contain really important information, so when I see a comment that is noise I delete it.

What could have been useful in the previous snippet was a comment explaining why a was being incremented. Right now I’m left wondering, and if the code is written properly I should be able to understand while reading  more of it. But sometimes it isn’t possible to express intent with code alone, in these cases a comment can help clear things up.

The why behing a piece of code can come in multiple forms, like:

  • Explaining intent
  • Warning of consequences
  • Justifiing something that without proper context could look wrong – like in the case of performance reasons

Another kind of good comments are documentation comments on public API, but only if they actually add relevant information that can’t be inferred from the signature of a method.

The worst kind of comment are out of date comments, because it spreads lies. Obviously no programmer has ever written an old comments. Comments become obsolete after they where written, because they tend to not being updated.

Comments that you should avoid are:

  • Comments that leave too much to interpretation – if you are taking the time to add a comment, make sure it is clear
  • Redundat – like in the example before
  • Mandated comments – policies shouldn’t require programmers to write comments, because they will write lots of redundant comments. Policies should require clean code
  • Comments that try to do the job of a version control system. Like tracking who edited what, when and why. Use a VCS

A special place in hell should be reserved to commented out code. Be rutheless and delete it as soon as you see it.

It’s important to remember to never use a comment when a name would do a better job, like in the following examples

DateTime date; //last modified
DateTime lastModified;

void functionName(){
   //Do this
   ...
   //Do that
   ...
   //Do something else
   ...
}
void functionName(){
   DoThis();
   DoThat();
   DoSomethingElse();
}