Stevie P’s blog

We all know base classes should be used to represent an “Is a” relationship. Yet all too often I have seen developers fall into the trap of refactoring common behaviour into a base class to remove duplication of code. To be honest I used to do this too. Personally I would rather see duplication of code than a complex class hierarchy. Of course, composition over inheritance is the golden rule of good object orientated code. I would sooner re-factor common behaviour into dependencies than a class hierarchy and here are my reasons why:

• Inheritance introduces more complexity and class hierarchies are more difficult to
  understand
• Base classes make code more rigid and therefore more difficult to change
• Using a base class for all the wrong reasons and you often end up with base classes with bad names.
• Makes testing far more difficult as you now have to set up the state of the system under test rather than mock a dependency.
• Base classes are less modular and therefore more difficult to reuse.
• You cannot swap implementations / behaviour of base classes out at runtime, i.e. no IOC.
• More difficult to re-factor the behaviour in isolation because all inheritors must now use the new implementation.
• Moving to composition becomes more difficult
• Responsibilities of classes become blurred and inevitably you end up with too many.
• You can’t inherit multiple base classes. This results in even more complex class hierarchies

CodeProject

So following on from my post on “Silverlight Mobile: A Simple workaround to the lack of a Data Type property on the DataTemplate type” I thought I would post my solution to the lack of a command property. Its yet another attached behaviour but I’m sure there will be pitfalls with this one and so this will hopefully be the start of a good discussion.

The problem:

So Silverlight mobile and MVVM are not exactly getting along like a house on fire at the moment. My second hurdle was avoiding click handlers in my code behind so I came up with an attached behaviour for a button. This will probably be re-factored to be more abstract or more likely strategized so that it can apply to more control types but I have no need for that at the moment. If I do end up strategising it I will be sure to post it.

The solution:

So here’s the attached property :

public class Attached : Button
    {
        public static read-only DependencyProperty CommandProperty =
            DependencyProperty.RegisterAttached(
            "Command",
            typeof(ICommand),
            typeof(Button),
            new PropertyMetadata(OnPropertyChanged));

        private static void OnPropertyChanged(object sender,
           DependencyPropertyChangedEventArgs args)
        {
            var obj = sender as Button;            

            obj.Click += new RoutedEventHandler((a, b) =>
            {
                var command = args.NewValue as ICommand;
                if (command != null)
                {
                    command.Execute(null);
                }
            });
        }

        private static RoutedEventHandler Clicked()
        {
            throw new NotImplementedException();
        }

        public static void SetCommand(
           DependencyObject obj, string tabStop)
        {
            obj.SetValue(CommandProperty, tabStop);
        }

        public static object GetCommand(DependencyObject obj)
        {
            return (object)obj.GetValue(CommandProperty);
        }
    }

And the usage:

...
<Button Grid.Row="1" Content="Settings"
Attached:Attached.Command="{Binding Path=SettingsCommand}" />
...

Many readers of my blog entry on “Beginning a WPF/MVVM application : Navigating between views” requested the source code be made available. I have bowed to the pressure so I have made it available on the original post. Click here for the original post and source code. Enjoy…

To my delight a colleague started blogging this week. His first blog was on an enum replacement pattern he tought me a couple of years back which is a great OO alternative to using enums.

He would like to remain anonymous for now but click here for the blog entry

So my password changes every month so setting up mapped drives that connect automatically on login is a bit of a pain. I created a powershell script that I could run when I needed the drives. I use a different administrative account to access these drives so I needed to capture the username and password at runtime. I also wanted to distribute the script to the rest of the team so the username and password should be captured at runtime using a SecureString for the password….

$username = Read-Host "Please enter your username..."
$securePassword = Read-Host -assecurestring "Password..."
$marshal = [Runtime.InteropServices.Marshal]
$password = $marshal::PtrToStringAuto
($marshal::SecureStringToBSTR($securePassword))
$net = new-object -ComObject WScript.Network
$net.MapNetworkDrive("X:", "\\NETWORKPC1\c$", $false, $username, $password)
$net.MapNetworkDrive("Y:", "\\NETWORKPC2\d$", $false, $username, $password)
$net.MapNetworkDrive("S:", "\\NETWORKPC3\d$", $false, $username, $password)
$net.MapNetworkDrive("T:", "\\NETWORKPC4\d$", $false, $username, $password)
$net.MapNetworkDrive("V:", "\\NETWORKPC5\c$", $false, $username, $password)
$net.MapNetworkDrive("W:", "\\NETWORKPC6\d$", $false, $username, $password)
$net.MapNetworkDrive("Z:", "\\NETWORKPC7\c$", $false, $username, $password)

Refactoring code should not be frowned upon or feared but it should be encouraged to make code ‘better’. That is, better and not just different to how it was before.

Ok, firstly let me make one thing absolutely clear: If there are no tests (unit tests or otherwise) for the code you are about to re-factor then you are not refactoring it at all. In fact you are just changing it and you must ask yourself why you are about to spend valuable time and resource changing perfectly ‘good’ code. While you’re at it ask yourself how you will verify it still does what it originally intended if you have no post development metrics (tests).

Someone went to great lengths to ensure that the code you are about to pick apart was working for the customer the way they wanted it. Your efforts will be disruptive at best and detrimental to the client and your team at worst.

So how do you re-factor code that has no unit test?

Regardless of whether I am refactoring or writing new code I try to start with the tests. When writing retrospective unit tests the job is sometimes easier because you already know roughly what the code should be doing. You can then make your own assumptions and write your own tests independent of the implementation detail of the existing code. For example: What happens when I pass null as an argument? what should it do? Throw an exception ? return null ?

Once I have come up with as many sensible assumptions (tests) myself it’s time to look at the code and try to pin down any assumptions that were made in the code. These are usually quite easy to spot in If / switch statements and pre iteration code (e.g. what was passed into a loop / method compared with the parameters).

So at the end of this exercise I should have a list of tests that kind of look like stories :

When null is passed into the exception handler:
    - Should not throw an exception
    - Should not attempt to display a message to the user
    - Should add an entry into the application log that says :
"A null exception was passed into the exception handler.
Please see the call stack. This should not happen!"
    - Should add the call stack to the call stack report
service with the session id
etc...

Ideally I can then take this to the original author and ask them if I have missed anything. Now the code is under test and I will be much more confident that once I have finished my refactoring, the code does what it should.

But why bother going through this? Why bother refactoring? Ultimately there must be a cost benefit to refactoring code. There should be a clear long term goal. For example, you want to make the code more testable so that you can introduce unit tests. In this case there is a clear long term goal of the refactoring.

So today I came across a beautiful race condition! It was a synchronous command that opened up a form but was relying on some data that was being loaded in the background. The code assumed that when the ‘Selected Item’ property was set, the view model had completed loading the data which was a correct assessment on the part of the developer that wrote it. So the code was hooking up to the PropertyChanged event and checked for property name = “SelectedItem” the handler ran the form logic which displayed the form that relied on the asynchronous data and then detached the handler. This ran perfectly 80% of the time but on some occasions the form was not being shown. This was because on some occasions where the data loads were faster the SelectedItem was being set before the command had a chance to hook onto the PropertyChanged event. Beautiful !

The Solution: To a developer looking at the code with no knowledge of the multithreaded aspect, the code looked illogical. Why don’t we just show the form? Why are we showing the form on PropertyChanged? This may sound like a naive question but I want consumers of my code to be naive and not have to worry about the internal working of the code if possible. So to make the code more elegant I wanted to hide the fact that the view model was loading the data asynchronously. Obviously the fix was to make access to the properties wait / block until the fetch had completed. I used a ManualResetEvent if you are interested.

This is the main reason I do not approve of listening for PropertyChanged events to glue bits of my application together. Other reasons :

- When a property name changes, parts of the application will break until all the PropertyChange handlers are also changed.
- Not easy to test
- Not explicit enough. What does it mean when “SelectedItem” has changed from a application domain perspective? How does that correlate to a possible asynchronous data fetch? Or simpler still; how can a consumer of that class infer the above assumption without prior knowledge? Instead I would prefer to expose an event that was more explicit such as ‘ViewModelDataFetched’ or even a SelectedItemChanged event would be better exposed. This makes the code easier to read and more maintainable.

The offending code:

public class RunActionCommand : ICommand{
...
public void Run(IDictionary<string, object> stateBag){
    var pluralViewModel = _mainViewModel.CurrentView as IPluralViewModel;
    if (pluralViewModel != null){
        pluralViewModel.PropertyChanged += PluralViewModelPropertyChanged;
    }
}
...
private void PluralViewModelPropertyChanged(object sender,
System.ComponentModel.PropertyChangedEventArgs e){
    if (e.PropertyName == "SelectedItem"){
        _synContext.Post(RunAction, sender);
    }
}

private void RunAction(object state){
    RunAction((IPluralViewModel)state);
}

private void RunAction(IPluralViewModel pluralViewModel){
...
var initialAction =
pluralViewModel.SelectedItem.Actions.First();
initialAction.Execute();
...
pluralViewModel.PropertyChanged -= PluralViewModelPropertyChanged;
...
}
...
}

The re-factored code :

public RunActionCommand(
...
public override void Run(){
    RunAction(Container.GetA<IPluralViewModel>());
}

private void RunAction(IPluralViewModel pluralViewModel){
...
var initialAction = pluralViewModel.CurrentActions.First();
initialAction.Execute();
}
}

//and in the pluralViewModel
...
private ManualResetEvent _manualResetEvent = new ManualResetEvent(true);
public IEnumerable<ActionDTO> CurrentActions
{
    get
    {
        _manualResetEvent.WaitOne();
        return SelectedItem.Actions;
     }
}
...

We reset the ManualResetEvent at the start of the fetch and set on the completion call-back of the asynchronous load operation.

Its obvious to see that the code is now much simpler from a consumer point of view.

I would love to hear of any other better ways of doing this so please leave your comments.

Having led teams in a variety of fields from software to property development here are some of the things I have learned over the last 5 years:

Don’t explicitly reject ideas:

There’s no need. You are the boss so if u don’t like the idea then simply don’t implement it! Nurturing a team should be treated like a constant brainstorming session. Rejecting ideas explicitly, discourages people from voicing their opinions later on. Without the contributions of your team you may as well outsource everything.

Acknowledge peoples roles and abilities:

You should be able to introduce every member of your team with some skill or ability or achievement.

Team selection:

Involve your core and experienced team members in the hiring process of new team members. Your new recruits must fit both on a personal and practical level. Your team will feel valued as a result and the less senior team members will feel a sense of achievement when they are eventually included in the process.

Respect other peoples work:

Never redo someone’s work without first giving them the opportunity to do so. Redoing should be backed up with a tangible benefit and discussed with the team as a whole so that there is a sense of a team consensus rather than just one guy doing everything again because he or she can.

This brings me to my next point:

Always try to justify things in terms of cost:

The gain must always out way the cost. In software the cost is mainly in man hours. Calculate the cost of development and ensure that this cost can be balanced with a client focus.

Give ownership and responsibility:

Give it freely, willingly and genuinely all the way through to execution. Never give pseudo responsibilities where you intervene, disregard and undermine the ideas and hard work already done. Even if you think someone is wrong. This serves two purposes. A) it is a far more valuable experience if mistakes are made and learned from and your team will become more autonomous as a result. If you always intervene then no one will do anything without first asking you. To some this may sound like a good thing and in very rare cases it might be but in the majority of situations there is a client involved. All you have done is create another client (you) and two clients are harder to please than one. If you keep intervening then why did you assign the responsibility in the first place? Remember that autonomous teams and individuals enable companies to grow organically both financially and in experience.

Tell what not how:

Not much more to this one. You are not more competent than your team otherwise you would not have hired them so leave the implementation to them.

Never take credit for other people’s ideas:

Even if the idea has been developed further by the team you should always acknowledge where the original idea came from. This feeds back into the above point about acknowledgement of peoples contributions.

Don’t treat people equally:

By that I mean never give the same rewards (financial or otherwise) to less performing members of the team. Doing so will bring everyone’s performance down towards that of your lowest performer.

Listen:

Listen to your team. They collectively have more experience than you do so most often they will be right. If you do reject the team consensus you had better make sure you are right because team productivity will fall dramatically during that period. If you are wrong then you will lose respect and credibility. If you are right then this usually pays off in team confidence but it’s a big gamble and the more you do it the more extreme the team reaction. It’s usually more common to take the gambles with less experienced teams for example a team leader with 20 years experience leading a team of 10 interns. But I would argue that in this situation you have the role of teacher and not boss and they are definitely not the same thing. Perhaps another blog post!

Job satisfaction, you are not the killer whale but the plankton:

Find out what your team like about their jobs and create opportunities for it to happen more often. As the boss you should be the one making things happen and creating work and opportunity for your team feeding them through their career.

Create opportunity:

Create lots of it and ensure that your long term and short term vision is known to all and remind everyone of it. Everything you do should take you a step closer to your vision and it will help people to make decisions on your behalf which means you can do your job and create yet more opportunities leaving your team to keep the engine running.

Consistency:

Be consistent in your decisions and your discipline. Inconsistency will create rebellion amongst your team and no one will understand how to conduct themselves.

Don’t try to be everyone’s friend:

They key word here is ‘try’. There will come a time when you need to discipline or even fire a member of staff. Let’s face it, you don’t do that to your friends so get used to the fact that you are their boss and not their friend. At the same time be yourself and have respect for your team. If you follow the previous point about team fit then you won’t have a problem with getting along with your team members and chances are you will naturally have a healthy and friendly relationship with your team members as they will be like minded individuals. By respect I mean, don’t be authoritarian for the sake of it because on that day you felt like people forgot you were the boss! You are the boss silly, remember ?

Whist writing this post, being a father of two, I realised that most of these points apply to parenthood too which totally makes sense.

I recently learned a very harsh lesson and I thought I should post it just to remind myself for future reference and offer up my mistakes so that others won’t make them:

When someone asks you to solve a problem don’t ask them to tell you what to do: But that sounds like the safest approach doesn’t it? After all if it goes wrong later you could go back and say: “but that is what you told me to do.” Big mistake ! Find out what they actually want / need and use your skills and your intelligence to deliver the solution by reading between the lines. Take a step back and think about what they are asking for. Be confident in your abilities and don’t be afraid of coming across as x ! Just be yourself, you won’t get very far by playing it safe all the time!

If you try to model a software solution too closely to the problem domain you will end up re-creating the problem or the complexity of the problem in the software. This is exactly the opposite to what software should achieve!

So what is my point? You cannot have an elegant solution if you try to be too cute about your object model. By cute I mean coming up with abstractions that somehow very closely resemble the real world problem domain. For example : When coming up with a concept for an object we should not get too bogged down with the original intent of that object because this only forces us to create new objects to deal with different scenarios rather than use the abstractions we already have. This principle is the fundamental basis of design patterns.

One of the most beautiful things about object orientation is code reuse. Creating small single responsibility objects that can be reused is the ultimate goal. So when faced with a situation where I need an object with some similar behaviour to some other object I created in the past but it doesn’t quite fit the scenario in question; this is usually a code smell for me. It means that object may have too much responsibility. I should re-factor the code so that I can reuse code I have already written rather than create a brand new concept in my application. Why? Because the new concept will almost always end up being the same concept but named something else and implemented slightly differently as if that somehow justifies its existence !