Focusing on the Problem

We recently had some work to do which focused on getting stock more real time on the website.  Here I’m going to take you on the journey of this piece of work.  
We started with the above “requirement”, the real problem however, was subtle different – overselling on the website.  This has the following impact on the customer and business:
  • The customer is annoyed because the order is cancelled, because it can’t be fulfilled
  • The cancellation process is slow, and is time consuming for Customer Services.
So how does stock get on the website?  Stock is calculated every 5 minutes with a batch job then read in two ways on the website:
  1. Through our search system
  2. Through a direct call to our ERP system
The first slice was to make stock come from a single source, this would be easier to change in the future.   So we set about reading the data directly from the ERP system and publishing this to our new stock service, then subsequently reading it in the website.  This was live for a brand within a day or two.  There was still some latency in this as we were still reliant on another batch job finishing before reading directly from the ERP system.
While delivering something quickly, this was the wrong slice as we didn’t really understand how we much we oversold by or when it happened (more on this later).
We started on the second obvious slice, making the stock real tiem, as we were still reliant on the batch job finishing for our stock levels to be up to date.  We decided to “baseline” our stock and then just deal with “goods out” – i.e. what was sold on the site.  
During this second slice, which was now much bigger, and more complicated.  There were some product launches – this meant we had an opportunity to measure the impact of what we had been doing.  We quickly realised that this second slice was wrong as we couldn’t measure the impact of it.  
We pivoted to what our first slice should have been – understand overselling.  We created numerous apps to let us monitor during these product launches so we could understand overselling.  These where manual and needed us to watch them run.  So after the first product launch, once we were happy this data was trustworthy.  We enhanced these to not have to be manually run, so we monitor oversells and unknown product sales 24/7 now.  We also started to capture when people entered the checkout on the website.
From this we could see a number of things about overselling:
  1. About 80% of people where in our checkout; once we’d sold out
  2. About 15% of people where still shopping on the site; once we’d sold out
  3. About 5% where due to the non-real time nature of stock
Even if we’d completed our original second slice – “real time stock” we wouldn’t have solved the problem – overselling!
We came up with a list of things we could do, and worked with the stakeholder to narrow it down to the following:
  • Before placing the order – do a stock check
  • When people enter the checkout – do a stock check
Unfortunately, we’d already made another improvement which we didn’t know the impact of.  Making the stock come from a single place.  This was quantifiable as we’d not been measuring before we’d made any changes.   
We made the changes outlined above and continuously released them and tried to monitor their impact. 
We had some bad/good luck our new stock service started failing just before the weekend, so we had to turn it off.  This was a blessing as we could now see how the site actually performed without any of our changes.  Below is overselling over the weekend:
The coming weeks will be interesting as there are product launches, which tend to increase our overselling.  Our plan is to monitor these and see if we need to do any more work.  
Some things we learned:
  • Metrics – understand the problem & measure the impact of changes
  • Pivot quickly
  • Deliver every commit
  • Logging in apps – understand what your applications are doing, business and application wise

Software Quality a Perspective

I’ve been recently talking to former colleagues about work.  We discuss code and perceived quality in code.  We tend to discuss the merits of good code and what it really looks like.  Each time I come back to the same place in my mind.  Good code is mainly about collective code ownership & having a shared understanding of what you are doing.  So how does one go about helping manifest this?

I think software development is a 80/20 game…  Depending on which you play it governs the impact on this collective code ownership and what some may deem as code quality.

If you work in the following manner 80% code and 20% communication – your code bases will all have a mish mash of styles, poor tests, good tests and really anything in between.  Certain code will be well understood by certain people, and other bits will be understood a lot less by other people.  Some people will open code bases and swoon at it’s clarity for them but others will open it up and want to instantly refactor it!

I’m more than convinced that working the other way 20% code and 80% communication gets a better shared code understanding.  Now I don’t mean that you all sit around in meetings and discuss what good code looks likes.  I suggest that you pair about 80% of your time, this is the communication piece that is lacking the other way around.  This conversation drives a shared understanding through your code bases.  It also helps ideas flow from mind to mind, while producing code to a certain degree.

Communication doesn’t just start or end with pairing, it starts by having the team be a team.  This is something to focus on above and beyond everything else initially.  Once you have this everything else will flow…  More on this in another post.

The Repository Pattern Sucks

A couple of years ago I would have told you that if you are accessing the a database you need a repository pattern. This allows you to hide away your data access code behind a nice interface – then at least you can mock the data access layer and change your data access code if you ever wanted to do that.

So say we are in a web application and want to get a “customer” by a specific reference. This is how we would do it with the repository pattern…

How different my opinion is today. I can’t help but feel that the pedal pushed repository pattern (in the C# .Net world) that is on the internet is fundamentally broken. The key thing for me is that everyone seems to want to create the following interface:

 public interface IRepository<TEntity, in TKey> where TEntity : class  
IEnumerable<TEntity> GetAll();
TEntity GetBy(TKey id);
void Save(TEntity entity);
void Delete(TEntity entity);

Not bad so far hey? This seems pretty “standard” apart from the fact you’ve broken single responsibility (SRP). How many responsibilities does the IRepository already have – 4 public methods which do very different things…

The other thing I’ve found is that things can & generally will get worse from here – then follows:

 public class Repository<TEntity, TKey> : IRepository<TEntity, TKey> where TEntity : class  
private readonly IUnitOfWork _unitOfWork;
public Repository(IUnitOfWork unitOfWork)
_unitOfWork = unitOfWork;
public IEnumerable<TEntity> GetAll()
return _unitOfWork.Session.QueryOver<TEntity>().ToList();
public TEntity GetBy(TKey id)
return Session.QueryOver<TEntity>().FirstOrDefault(x => x.Id == id);
public void Save(TEntity entity)
public void Delete(TEntity entity)

So now all you need to do to get access to something is as follows:

 public class CustomerRepository : Repository<Customer, int>, ICustomerRepository  
public CustomerRepository(IUnitOfWork unitOfWork) : base(unitOfWork)

Does this look familiar? It does to me – I’ve done this and I’m pretty sure its wrong!

Again there are a couple of things that appear wrong:

  1. SRP is broken
  2. What happens if you’ve got a complicated queryable object and you are using an ORM – you’ll end up with lots of SELECT N+1.
  3. We’ve over complicated a simple task with – 3 interfaces, a base class (coupling), and 2 classes.
  4. If you want to read from one data store and write to another data store how would you do this?

We’ve introduced so many layers and complexity that it seems that even to do a simple thing it’s been massively over complicated. I’ve forgot to show you the IUnitOfWork which is equally badly suggested and implemented on the internet…

Now for the alternative:

 var customer;  
using(var transaction = Session.BeginTransaction())
customer = Session.QueryOver<Customer>().FirstOrDefault(x => x.Id == customerId) ?? new NullCustomer();

Wow, is this simple or what? But what about testing? Well I tend to use NHibernate with an InMemoryDatabase so I don’t have to mock the data access layer… I don’t tend to use mocks but that’s another story… The code is simple and clear.

Now if things advanced and I wanted to put it in a class then I’d put it behind something that only reads customer information:

 public class ReadCustomers  
private ISession _session;
public ReadCustomers(ISession session)
_session = session;
public Customer GetBy(int customerId)
var customer;
using(var transaction = _session.BeginTransaction())
customer = _session.QueryOver<Customer>().FirstOrDefault(x => x.Id == customerId) ?? new NullCustomer();
return customer;

Again with storing information if you really wanted to push it in a class then something like this:

 public class StoreCustomers  
private ISession _session;
public StoreCustomers(ISession session)
_session = session;
public void Add(Customer customer)
using(var transaction = _session.BeginTransaction())

The classes have clear responsibility & a single responsibility – you can even easily change where you read and write data from and too! Plus the code is much simpler and easier to optimise.

From “Golden Master” to Unit Tests

So I’m currently working on some code which has been written but needs refactoring to add some additional business requirements – there are very few tests.  So I started by wrapping the code that I want to work with a “golden master”/characterisation test below is the pseudo code for this:

[Test, Ignore (Run this once at the begining)]
void GenerateGoldenMaster()
Call service
Serialize result to xml
Save to file

void CompareGoldenMaster()
Call service
Serialize result to xml
Save file
Compare with master & assert correctness

I save the file so that I can see the results side by side – sometimes this is helpful rather than assertion failure in the test runner.

If you’ve never heard of the “golden master”/characterisation test then it’s a testing technique to ensure that as you refactor some legacy code the output is the same.  Anyhow, as I started the refactor the code to add the new functionality, I noticed a couple of bugs.  Now while you’ve got the “golden master” I would advise against fixing these bugs until a little later in the refatoring stage, I prefer to highlight and fix these bugs by using tests.  This is where you should be going – as I don’t tend to like to run comparison tests in a CI environment.

A couple of things happened while running through the refactoring – the “golden master” didn’t have enough test combinations in.  I therefore had to reconfigure the data in the database to give this additional test combinations.  I only did this once I had everything passing with the original “golden master” that I had created.  This gave me the confidence that the changes to the data would generate the necessary failing assertion.

Once I’d got to a point where I was happy that “most” of the combinations had been covered.  I then started to add the tests which will run in the CI environment.  Essentially, as this is a service with a db, I start by creating an in memory database, adding some data and asserting that the service response contains these bits of data.  I also use random data (guids) generated each test to add some testing invariance…

Now is when I want to highlight the bugs that I’ve found before as I now feel confident that the current refactored code is the same as it was before, but now I can add the tests to fix these bugs.  Insert the data as required then check that the output is incorrect – this will obviously create a red test which can now be fixed with confidence.

As the tests grow the “golden master” can be removed and deleted from the code base.

I’ve used the “golden master” a number of times within production code with no tests, if you find yourself in this position, then see if you can wrap the original code with a “golden master” and then start refactoring.  I think you’ll be surprised how effect they are…

Test Clean-Up

I’ve recently had a massive clean up of dependencies between our Core solution, tests & everything else that comes to mind.

During this time I noticed a number of tests which fell in to the following categories:

WTF?  Are they testing!
They’ve been in our test suite & build server for a long time in there current guise.  The first set are similar to:
public void BuildMeAPanda()
     var panda = new Panda(“Chi Chi”);
     Assert.That(“Chi Chi”, panda.Name);
As you can see we are simply testing the logic of a constructor.  Not too much value in this.  Although it looks good on our code coverage stats!!!  
The second are ignored tests which seem to have been written with a future intend – i.e:
[Test, Ignore(“Really need to work out how to test this!”)]
public void InteractWithDb()
    var panda = new Panda();
    var repo = new PandaRepository();
    // work out what to do here!
Broken & [Ignored]
The third are the they broke the build, I must get the build in, quickly ignore the test ones:
[Test, Ignore(“This breaks the build I’ll re-instate it later”)]
public void Bug52358()
    // Arrange 
    … Lots of code
   // Act
   … Some more code
   // Assert
   … Some assertion
My view on these tests is to eradicate them.  If they’ve not been in your build cycle for some time then they are not adding any value.  They are simply adding overhead to your build time, your thoughts and general clunk within your solution.
I would also try to re-instate the Broken but ignored tests, however, if they fail and you can’t understand them within a time boxed amount of time delete them…
The only occurrence that I’ve not done this – is on those integration tests where you use them occasionally – a prime example of this is testing email & sms sends.  However, these should appear in a different project called:
  • .IntegrationTests
They should be clearly labelled with:
  • [Ignore(“Integration tests!”)]
And over time you probably want to delete these too!?!

Collective Code Ownership

I recently asked the following question – What does good team code ownership look like?

  • Everyone adding (good) test coverage
  • Discussing the code
  • Design strategy
  • Adhere to code standards
  • Tool box of patterns

Everyone adding (good) test coverage

This is especially important, tests make everyone feel secure when making changes to the code base.  Everyone in the team must know the benefits of tests, so must maintain them and add other good tests.

Discussing the Code

The code is essentially a living entity, i.e. we add functionality, we have to look after it, it is often sick!  So it’s important that the team talks about the code base.  This is powerful because it gives people are deeper understanding of the code base.

Design strategy

A way to improve the design – most code bases have areas that don’t give us the necessary ways to change the code, or they are hard to test.  So they need to be refactored & improved upon.  We are currently doing this with our UI – using MVP.

Adhere to code standards

Currently we don’t have any standards, however, I do think we have a good code style.  This is mainly garnered from Uncle Bob’s clean code videos – which are excellent by the way – you should watch them if you’ve not (or read his book Clean Code)!

Toolbox of Patterns

You’ve got to be able to change the code, this becomes linked to discussing the code.  Patterns can help with discussing the code…  Having a design strategy – links to this!

This is not a comprehensive list but it’s a good start…

“It’s important not to own code you’ve written in a code base; however it’s important that people respect the way the code was written”