Stuff - mainly geek, a little diabetes, some rant. Possibly all three in the same post. Basically anything I find interesting and/or worth sharing.
These are my personal views, unless I've been hacked.
I stumbled over a videoblog on YouTube over the weekend, Improve your Team Agility in 10 minutes. The main idea on how to improve agility is...(drum roll)... have a retrospective.
One idea that the presenter suggests is this: print out the agile manifesto and the principles of the manifesto. During your retrospective, talk through them, then give each member of the team 2 red dots and 2 green dots. Get the team to swarm the printout of the principles, adding a green dot for what the team does well and a red for what the team needs to improve. Then analyse the results and see if there are any ideas for improvement.
Some rules that I'll be using are:
We need to read and think about the principles before the retrospective;
The green and red dots are to represent what the team does well/badly, not any individual within the team *;
We should at least try to explain why we chose to dot a particular principle;
The dotting is done en masse, so nobody is influenced by previous dots;
We don't have to use all 4 dots.
* I tweeted about this experiment today, and it sounded like it was to root out an individual in the team who wasn't agile enough and do away with him/her, like Piggy in Lord of the Flies. The point of the experiment is to improve the team by making the team realise what it needs to do better, not by culling its weakest member!
I have worked in my current job for nearly a year now. The R & D team's approach to development is as agile as possible; limiting work in progress, close collaboration between developers and testers, pair programming (or at least, peer code reviews), TDD, small checkins, continuous integration, collective code ownership, releasing to production quickly and often, reducing the feedback loop, scrumban, continuous process improvement.
Keystones
The release schedule of our main product is every fortnight; peripheral packages are released the week when our main product isn't being shipped. This means that sometimes our product ships with part-completed (but NEVER untested) features in it. These part-complete features are generally invisible to the user by their nature, sometimes a new database table, sometimes a new repository. These changes are designed to be used by the customer only when the keystone is deployed. As the name suggests, this is the final piece of the feature that allows it to be made visible and used by the customer. It's usually the UI component, but could also be code that's enabled via a config switch.
No Branch, No Merge
In the year I've been at the company, I can count on one hand the number of times we've branched the source code. Each time we have, it was to apply a hot patch to a release. We don't branch the code for concurrent feature development; everyone checks their code in to main / trunk. One reason for this is to avoid the all-too-common 'merge hell', trying to keep each branch up to date with the others' changes. If the code is branched and e.g. a 6 week feature is produced, there could well be thousands of code changes to merge, per branch, with the very real possibility of hundreds of merge conflicts. I have been responsible for merging releases of source code into a main branch, and it is not easy and it's certainly not pleasant. But most of all, it's not necessary.
Sometimes, a feature of the C# language just isn't quite enough to do the job you'd like it to. An alternative view is that it wasn't designed to do that; I hear what you're saying, but let's ignore that for the time being. The built in structure to restrict a list of items to a known set is the enumeration. This is designed to provide a programmer with a choice of strongly named constants, that each map to a numeric value.
The Example
Sometimes the list of items that is needed is not numeric, but a string; take for example the following data structure, which maps a list of features available on to a list of cars.
Given feature.Id is calculated by the database on entry (Sql Server 'IDENTITY', MySql 'AUTO_INCREMENT'), the Id of the feature can't be relied on to be constant across different installations, so the Name column needs to be used as the identifying property.
When searching for whether a particular feature is available on a car, some SQL needs to be constructed, to query the table, e.g.
In order to constrain the list of features available to a developer, the usual approach would be to create an enum containing the features available
enum.ToString()
The enum would then be used to build the Sql by using the ToString() method of the enum:
This method will rely on the enum being named identically to the database value, and will not allow spaces in the name, so "Air Conditioning" and "Heated Seats" are not valid names.
Attributes
An alternative method is to create a custom attribute for the enum to define the database name:
This allows the "name" of the feature to be specified, with the added advantage that the attribute name does not need to match the enum name. However, there is extra plumbing required to reflect the attribute value at runtime:
The Type Safe Enum Pattern
There is an alternative to the above solutions, and it is to effectively roll your own enum. I'm not going to pretend any of this was my idea, I believe there are examples of it in the hieroglyphics from Tutankhamen's tomb.
The best way to describe the pattern is to show an example:
This gives you a class that has static properties with a type of that class... ouch. But it means that the usage of the class is almost exactly the same as the usage of an enum, but without the confusion of an enum being intrinsically a number:
Having read the article, my friend and ex-colleague @NathanGloyn (check his blog here) posed the question as to why I hadn't gone all the way and refactored the code to a switch statement instead of a series of if...else if... blocks.
I really wish I had; as Nathan confirmed, the C# compiler would have refused to compile the code because of the duplicated values, and I could have invoked the keystroke of the gods, Ctrl-Z:
I've been trying to think why I didn't take that step and refactor further. Ironically, I think I was scared of breaking things if I refactored too hard.
One of my favourite coding analogies is Uncle Bob's Boy Scout Rule - "Always leave the campground cleaner than you found it". I try to apply this rule when I have the drains up on a piece of code, if it looks like it needs some love.
I got bitten today, when a piece of boy scouting I'd done hit production. With hindsight it was obvious that I should have taken more care, but the trapdoor I fell down was a humdinger.
The code I thought I was refactoring was similar to this:
So I added some basic optimisation:
All good...
Except, the code I actually refactored to ended up looking like this:
Can you spot the difference? I didn't! It's subtle, but the effect was a huge change in behaviour.
I've learnt lots from this:
tiny refactors can have a huge effect;
code will sometimes come back and bite you;
a peer review is not a safety net;
you can't guarantee a refactor unless you have solid unit tests;
don't assume that code you're about to refactor is in any way optimal;
and, most important
when a ticket comes in around some code you've changed, grab it
Why did you change it? What was wrong with it as it was? Where's the feedback now?
Imagine the scene... you're travelling along the motorway at 69.7mph, 315ft behind the car in front. The windscreen is getting slightly misty so you decide to use your car's blower to direct a blast of air on the windscreen. You risk a glance at the digital climate control console:
That's blowing on the windscreen and your feet.
You look for *the button*
You press it, and risk another glance at the console:
That's now blowing at the windscreen, your feet and your face; that's less effective than the last one, so you press the button again. And another glance:
Hmm, now it's not blowing on the windscreen at all. Just your feet and face. The windscreen's getting more misty now. Let's see what the button does this time:
I don't even know what this symbol means. I get feet and face, but "up"? Symbolised by an empty triangle instead of filled... still, it's not blowing on the windscreen, so let's hit the button. Again. And look. Again.
Ok, it's not pointing at your feet any more. Just your face, and "up". You sigh. The windscreen mists up even more. You press the button. Surely...
No. It's just blowing directly at your face. You wish hard, and press the button again.
At last, you now have air blowing at your windscreen.
But at what potential cost? Assuming you followed the instructions above, you will have taken your eyes off the road 8 times. There's no link between a push of the button and its result. Compare this to the more traditional analogue heater/blower console:
One glance and you can see that to get air on the windscreen, you need to twist the right-hand dial 1/5 clockwise. You'll feel the 1/5 turn, you'll get instant feedback. Simple. Great UX.
I read this blogpost a few days ago and was intrigued enough to have a go at refactoring its monster switch statement myself, during the Grand Prix. Here's my approach:
First task, get the creation of a "TaskClass" into a factory. A factory will need an interface to work with, so I defined the simplest one possible:
public interface ITaskClass { }
...and implemented it on each taskclass
public class Task1Class : ITaskClass { public void ExtractTop10Data(DateTime dtLastRun) { return; } }
Next, I created a TaskClassFactory, and duplicated the monster switch into the Create method
public static class TaskClassFactory { public static ITaskClass CreateTaskClass(string name) { switch (name) { case "Task1": return new Task1Class(); case "Task2": return new Task2Class(); case "Task3": return new Task3Class(); case "Task4": return new Task4Class(); } return null; } }
This gave me the opportunity to write some tests around the creation logic
[TestCase("Task1", typeof(Task1Class))] [TestCase("Task2", typeof(Task2Class))] [TestCase("Task3", typeof(Task3Class))] [TestCase("Task4", typeof(Task4Class))] public void CreateTest(string name, Type expectedType) { Assert.IsInstanceOf(expectedType, TaskClassFactory.CreateTaskClass(name)); }
Then I introduced the factory into the monster switch (please ignore the smell, this is very much work in progress)
Then I consolidated some of the repeated code; first the repeated catch block
try{ // switch on taskname.. //There were 22 task names in total making this method over 600 lines long! switch (task.TaskName) { case "Task1": var task1Class = (Task1Class)TaskClassFactory.CreateTaskClass(task.TaskName); task1Class.ExtractTop10Data(dtLastRun); task.LastRunTime = DateTime.UtcNow; UpdateTaskRunStatus(TaskStatus.Good, task); break; case "Task2": var task2Class = (Task2Class)TaskClassFactory.CreateTaskClass(task.TaskName); task2Class.CreateImageFeed(); task.LastRunTime = DateTime.UtcNow; UpdateTaskRunStatus(TaskStatus.Good, task); break; case "Task3": var task3Class = (Task3Class)TaskClassFactory.CreateTaskClass(task.TaskName); task3Class.DestroyImageFeed(); task.LastRunTime = DateTime.UtcNow; UpdateTaskRunStatus(TaskStatus.Good, task); break; case "Task4": var task4Class = (Task4Class)TaskClassFactory.CreateTaskClass(task.TaskName); task4Class.ProcessData(); task.LastRunTime = DateTime.UtcNow; UpdateTaskRunStatus(TaskStatus.Good, task); break; default: break; } } catch (Exception) { UpdateTaskRunStatus(TaskStatus.Error, task); Logger.LogError("Error details here"); }
and the common sweep-up tasks
try { // switch on taskname.. //There were 22 task names in total making this method over 600 lines long! switch (task.TaskName) { case "Task1": var task1Class = (Task1Class)TaskClassFactory.CreateTaskClass(task.TaskName); task1Class.ExtractTop10Data(dtLastRun); break; case "Task2": var task2Class = (Task2Class)TaskClassFactory.CreateTaskClass(task.TaskName); task2Class.CreateImageFeed(); break; case "Task3": var task3Class = (Task3Class)TaskClassFactory.CreateTaskClass(task.TaskName); task3Class.DestroyImageFeed(); break; case "Task4": var task4Class = (Task4Class)TaskClassFactory.CreateTaskClass(task.TaskName); task4Class.ProcessData(); break; default: break; } task.LastRunTime = DateTime.UtcNow; UpdateTaskRunStatus(TaskStatus.Good, task); } catch (Exception) { UpdateTaskRunStatus(TaskStatus.Error, task); Logger.LogError("Error details here"); }
One of the tasks has a parameter to its processing method. I assumed that out of the 22 others not shown there would be more variation to the processing methods. With a nod of acknowledgement to YAGNI, I decided that each task class that needed a parameter to perform its function should be constructed with these parameters; this would mean that I could create a parameterless processing function in the interface. More on this later. Anyway, to enable different constructor parameters to be used in the factory, I created a parameter object and added it to the Create method
public struct Parameters { public DateTime LastRunDate { get; set; } } ... public static ITaskClass CreateTaskClass(string name, Parameters createParams)
and in the monster switch:
TaskClassFactory.Parameters factoryParams = new TaskClassFactory.Parameters { LastRunDate = task.LastRunTime }; ... var task1Class = (Task1Class)TaskClassFactory.CreateTaskClass(task.TaskName, factoryParams);
This enabled me to define a parameterless DoWork method on the ITaskClass interface
public interface ITaskClass { void DoWork(); }
This meant I needed to implement the DoWork method in each of the TaskClass implementations. For those with no parameters on the processing method, the implementation was a simple call to that method
public class Task2Class : ITaskClass { public void CreateImageFeed() { return; } public void DoWork() { CreateImageFeed(); } }
The one with a parameter needed a little more scaffolding
public class Task1Class : ITaskClass { private DateTime _lastRunDate; public Task1Class(DateTime lastRunDate) { _lastRunDate = lastRunDate; } public void ExtractTop10Data(DateTime dtLastRun) { return; } public void DoWork() { ExtractTop10Data(_lastRunDate); } }
This forced the factory to change, to use Task1Class's new constructor
public static ITaskClass CreateTaskClass(string name, Parameters createParams) { switch (name) { case "Task1": return new Task1Class(createParams.LastRunDate);
This meant I could hide the existing processing methods in the task classes
