Friday, 17 May 2013

Code Reviews–What are we looking for?

A few years back I wrote about the issue of when to perform code reviews. Code reviews which happen late in the development cycle can be a waste of time, since it may be too late to act on the findings. However, there is another, much bigger, problem with code reviews, and that is the issue of what are we supposed to be looking for in the first place?

At first this might seem to be a non-issue. After all, the reviewer is simply looking for problems of any sort. But the trouble is that there are so many different types of problem a piece of code might contain that it would take a very disciplined reviewer to consciously check for each one. It would also take a very long time. The reality is that most code reviewers are only checking for a small subset of the potential issues.

So what sorts of things ought we to be looking for?

 

Code Standards

I put this one first as it is the one thing that almost all code reviewers seem to be good at. If you break the whitespace rules, variable naming conventions, or miss out a function or class comment, you can be sure the code reviewer will point this out.

However, these are things that a good tool such as ReSharper ought to be finding for you. Human code reviewers ought to be focusing their attention on more significant problems.

 

Language Paradigms

Other things an experienced developer will be quick to pick up on are when you are using the language in a sub-optimal way. In C#, this includes things like correct use of using statements and the dispose pattern, using LINQ where appropriate, knowing when to use IEnumerable and when to use an array, and making types immutable where appropriate.

Observations like this are particularly useful to help train junior developers to improve the way. But again, many of these issues could be discovered by a good code analysis tool.

 

Maintainability

In a large system, keeping code maintainable is as critical as keeping it bug free. So a code review should look for problems such as high code complexity. I almost always point out a few places in which the code I review could be made “cleaner” by refactoring into shorter methods with descriptive names.

Uncle Bob Martin’s “SOLID” acronym is also a helpful guide. Are there bloated classes with too many responsibilities? Are we constantly making changes to the same class because it isn’t “open to extensibility”? Are we violating the “Liskov Substitution Principle” by checking if an object is an instance of a specific derived type?

Noticing maintainability problems at code review time, such as tight or hidden coupling through the use of singletons, may require some substantial re-work of the feature, but it will be much less painful to do it while it is still fresh in the developer’s mind rather than having to sort the mess out later on in the project lifetime.

 

Project Conventions

Every software project will have some kind of organization and structure, and in an ideal world, all developers are aware of this and make their changes in the “right” place and the right way. However, with large projects, it can be possible for developers to be unaware of the infrastructure that is in place. For example, do they ignore the error reporting mechanism in favour of re-inventing their own? Do they know about the IoC container and how to use it? Did they keep the business logic out of the view code?

For issues like this to be picked up, the code reviewer needs to be someone familiar with the original design of the code being modified. It may be necessary on some occasions for the code reviewer to ask the developer not to copy an existing convention because it has been discovered to be problematic.

 

Finding Bugs

Most discussions of code reviews assume that finding bugs is the main purpose, but from the list of items I’ve already mentioned it is not hard to see how we could actually forget to look for them.

This involves visually inspecting the code looking for things like potential null reference exceptions, or off by one errors. A lot of it is about checking whether the right thing happens when errors are encountered.

The trouble is that if the code is highly complex or badly structured, then it may be close to impossible to find bugs with a visual inspection. This is why the “maintainability” part of the code review is so important. Giray Özil sums this problem up brilliantly:

“Ask a programmer to review 10 lines of code, he'll find 10 issues. Ask him to do 500 lines and he'll say it looks good.”

 

Meeting Requirements

If the code reviewer doesn’t have a firm grasp of the actual requirements that the coder was trying to implement, it is quite possible that code that simply doesn’t meet the requirements gets through the code review. The more understanding a code reviewer has of the actual business problem this code is trying to solve, the more likely they are to spot flaws (or gaping holes) in its implementation.

 

User Interface

Now we move onto some areas for review that relate to specific types of code. The first is user interface code. This adds a whole host of things for a reviewer to check such as correct layout of controls, correct use of colour, all localisable text read from resource files, etc. There will likely be some established conventions in place for user interface, and for a professional looking project, you need to ensure they are adhered to.

There is the also need to review user interfaces for usability. Developers are notoriously bad at designing good UIs, so a separate review with a UX expert might be appropriate.

 

Threading

Multi-threaded code is notoriously hard to get right. Are there potential race conditions, dead-locks, or resources that should be protected with a lock? It is important that a code reviewer is aware of which parts of the code might be used in a multi-threaded scenario, and give special attention to those parts. Again, the code needs to be as simple as possible. A 4000 line class that could be called on multiple different threads from dozens of different places will be close to impossible to verify for thread safety.

 

Testing

A good code reviewer should be asking what has been done to test the code under review, including both manual and automated tests. If there are unit tests, they too ought to be code reviewed for correctness and maintainability. Sometimes the best thing a code reviewer can do is suggest additional ways in which the code ought to be tested (or made more testable – the preference should always be for automated tests where possible).

 

Security

One of the most commonly overlooked concerns in code reviews is security. There are the obvious things like checking the code isn’t vulnerable to a SQL injection attack, or that the passwords aren’t stored in plaintext, or that the admin-level APIs can only actually be run by someone with the correct privileges. But there are countless subtle tricks that hackers have up their sleeves, and really your code will need regular security reviews by experts.

 

Performance

This is another commonly overlooked concern. Of course, much of our code doesn’t need to be aggressively performance tuned, but that doesn’t mean we can get away without thinking about performance. This requires knowledge of what sort of load the system will be under in “real-world” customer environments.

 

Compatibility

Another big issue particularly with large enterprise systems that the code may need to run on all kinds of different operating systems, or against different databases. It might need to be able to load serialized items from previous versions of the software, or cope with upgrading from the previous version of the database. If these problems are not spotted in code review, they can cause significant disruption in the field as customers find that the new version of the software doesn’t work in their environment.

Where possible, automated tests should be guarding against compatibility breakages, but it is always worth considering these issues in a code review.

 

Merging

If you are in the situation where you need to actively develop and maintain multiple versions of your product, then a code reviewer needs to consider the impact of these changes on future merges. As nice as it might be to completely reformat all the code, rename all the variables and reorganize all the code into new locations, you may also be making it completely impossible for any future merges to succeed.

 

Summary

Looking at the list above you might start to think that a code reviewer has no hope of properly covering all the bases. For this reason I think it is important that we clarify what a particular code review is intended to find. There may need to be special security, performance or UI focused reviews. I also think we should be using automated tools wherever possible to find as many of these issues for us.

I’d love to hear your thoughts on this topic. Which of these areas ought a code review to focus on? Have I missed any areas?

Wednesday, 15 May 2013

Knockout - What MVVM should have been like in XAML

I've not got a lot of JavaScript programming experience, but I've been learning a bit recently, and decided to try knockout.js. I was pleasantly surprised at how quickly I was able to pick it up. In part this is because it follows a MVVM approach very similar to what I am used to with WPF. I've blogged a three-part MVVM tutorial here before as well as occasionally expressing my frustrations with MVVM. So I was interested to see what the MVVM story is like for Javascript developers. What I wasn't expecting, was to find that MVVM in JavaScript using knockout.js is a much nicer experience than I was used to with XAML + C#. In this post I'll run through a few reasons why I was so impressed with knockout.

Clean Data-Binding Syntax


The first impressive thing is how straightforward the binding syntax is. Any HTML element can have a data-bind property attached to it, and that can hold a series of binding expressions. Here's a simple binding expression in knockout that puts text from your viewmodel into a div:

<div data-bind="text: message" ></div>

To bind multiple properties, you can just add extra binding statements into the single data-bind attribute as follows:

<button data-bind="click: prev, enable: enablePrev">

In XAML the syntax for basic bindings isn't too cumbersome, but a bit more repetitive nonetheless:

<TextBox Text="{Binding Message}" 
Enabled="{Binding EnableEditMessage}" />

Event Binding

One frustration with data binding in XAML is that you can't bind a function directly to an event. So for example when an animation finishes, it would be great to be able to call into a method on our ViewModel with something like this:

<Storyboard Completed="{Binding OnFinished}" />

Sadly, that is invalid XAML, but with knockout.js, binding to any event is trivial:

<div data-bind="click: handleItem"/>

Arbitrary expressions in binding syntax


XAML data binding does allow you to drill down into members of properties on your ViewModel. For example, you can do this:

<TextBlock Text="{Binding CurrentUser.LastName}" />

And it also does give you the ability to do a bit of string formatting, albeit with a ridiculously hard to remember syntax:

<TextBlock Text="{Binding Path=OrderDate, StringFormat='{}{0:dd.MM.yyyy}'}" />

But you can't call into a method on your ViewModel, or write expressions like this

<Button IsEnabled="{Binding Items.Count > 0}" />

However, in knockout.js, we have the freedom to write arbitrary bits of JavaScript right there in our binding syntax. It's simple to understand, and it just works:

<button data-bind="click: next, enable: index() < questions.length -1">Next</button>

This is brilliant, and it keeps the ViewModel from having to do unnecessary work just to manipulate data into the exact type and format needed for the data binding expression. (Anyone who's done MVVM with XAML will be all too familiar with the task of of turning booleans into Visibility values using converters)

Property Changed Notifications


Obviously, knockout needs some way for the ViewModel to report that a property has changed, so that the view can update itself. In the world of XAML this is done via the INotifyPropertyChanged interface, which is cumbersome to implement, even if you have a ViewModelBase class that you can ask to do the raising of the event for you.

private bool selected;
public bool Selected 
{
   get { return selected; }
   set   
   {
        if (selected != value)
        {
            selected = value;
            OnPropertyChanged("Selected");
        }
   }
}

Contrast this with the gloriously straightforward knockout approach which uses a ko.observable:

this.selected = ko.observable(false);

Now selected is a function that you call with no arguments to get the current value, and with a boolean argument to set its value. It's delightfully simple. I can't help but wonder if a similar idea could be somehow shoehorned into C#.

To be fair to the XAML MVVM world, you can alleviate some of the pain with Simon Cropp's superb Fody extension, which allows you to add in a compile time code weaver to automatically raise PropertyChanged events on your ViewModels. I use this on just about all my WPF projects now. It is a great timesaver and leaves your code looking a lot cleaner to boot. However, in my opinion, if you have to use code weaving its usually a sign that your language is lacking expressivity. I'd rather directly express myself in code.

Computed properties


Computed properties can be a pain in C# as well, as you have to remember to explicitly raise the PropertyChanged notification. (Although Fody is very powerful in this regard and can spot that a property getter on your ViewModel depends on other property getters.) Here's an example of a calculated FullName property in a C# ViewModel:

private string firstName;
public string FirstName 
{
   get { return firstName; }
   set   
   {
        if (firstName!= value)
        {
            firstName= value;
            OnPropertyChanged("FirstName");
            OnPropertyChanged("FullName");
        }
   }
}

public string FullName 
{
    get { return FirstName + " " + Surname; }
}

Knockout's solution to this is once again elegant and simple. You simply declare a ko.computed type on your ViewModel:
this.fullName = ko.computed(function() {
     return this.firstName() + " " + this.lastName();
}, this);

Elegant handling of binding to parent and root data contexts


Another area that causes regular pain for me with XAML databinding is when you need to bind to your parent's context or the root context. I think I've just about got the syntax memorized now, but I must have searched for it on StackOverflow hundreds of times before it finally stuck. You end up writing monstrosities like this:

...Binding="{Binding RelativeSource={RelativeSource FindAncestor, 
AncestorType={x:Type Window}}, Path=DataContext.AllowItemCommand}" ...

In knockout, once again, the solution is simple and elegant, allowing you to use $parent to access your parent data context (and grandparents with $parent[1] etc), or $root. Read more about knockout's binding context here.

<div data-bind="foreach: currentQuestion().answers">
    <div data-bind="html: answer, click: $parent.currentQuestion().select"></div>
</div>

Custom binding extensions!


Finally, the killer feature. If only we could add custom binding expressions to XAML, then maybe we could work around some of its limitations and upgrade it with powerful capabilities. Whilst I have a feeling that this is in fact technically possible, I don't know of anyone who has actually done it. Once again knockout completely knocks XAML out of the water on this front, with a very straightforward extensibility model for you to create your own powerful extensions. If you run through the knockout tutorial, you'll implement one yourself and be amazed at how easy it is.


Conclusion

I've only used knockout.js for a few hours (here's what I made) and all I can say is I am very jealous of its powers. This is what data binding in XAML ought to have been like. XAML has been around for some time now, but there have been very few innovations in the data-binding space (we have seen the arrival of behaviours, visual state managers, merged dictionaries, format strings, but all of them suffer from the same clunky, hard to remember syntax). And now we have another subtly different flavour of XAML to learn for Windows Store 8 apps, it seems that XAML is here to stay. Maybe it is time for us to put some pressure onto Microsoft to give XAML data binding power to rival what the JavaScript community seem to be able to take for granted.