Thursday, 25 November 2010

Writing good software systems

Writing good software systems cannot be reduced to a simple set of rules, it is a complex and wide-reaching subject where some problems have more than one solution, one which might or might not be better than the others. It is full of compromises and various technologies. However, there are certain fundamentals to good software that are not negotiable, certain frameworks that must be in place if you do not want to survive on luck alone or on the fact that one of your engineers might happen to be very good at their job and you get quality by default rather than by design.


  1. Training - If you employ people who are not trained/qualified/experienced, you cannot expect good software. You would not employ an unqualified doctor and software is not something so easy that a kid can do it. You can write bad software easily but knowledge is required to write good software. You must consider ongoing training as well, partly to adapt to new technologies and partly to challenge the way we get used to working.

  2. Specifications - Coders tend to enjoy coding much more than they enjoy writing specs. This is understandable but it is essential that functionality is specified either by the programmer or in larger outfits by people specifically employed for this purpose. Every hour spent on a spec can save 50 hours of wasted development. Get used to really grilling the functionality in the spec, think of details such as security, validation, error (both expected and unexpected) although much of this can be pulled out into common specs that apply to multiple systems. Get the specs signed off by your customer, even if they are internal. If someone then turns around and says something is wrong in the spec, it becomes a development item (low priority) not a defect to fix immediately. It also makes people take their analysis to the correct stage. "No spec" means we've considered the easy 90% and not the hard 10%, a spec should force these harder details to be considered and potentially to affect the things we thought were easy to do.

  3. Quality control - People make mistakes and are sometimes lazy, if you expect them not to be then you should not be a manager. The trick here is to put things in place that prevent mistakes (automating tasks) and pick them up when they are made (such as code reviews). Although this is a learning exercise and might be slightly different depending on the type of work being carried out, how many people etc, the key here is to start with the basics and learn from yours and others mistakes. Something goes wrong, ask yourself why, how it could be avoided, whether it is worth the extra cost/time compared to the liklihood of the mistake being made and then bring in a process/modify a process to learn from your mistake. This really is not rocket science and it will help you to become Quality Accredited if you seek this in the future.

  4. Regular Reviews - Do not think that quality is a one-person thing. Have regular reviews with your teams and value their feedback. People close to the coalface know what is happening and do not appreciate having all decisions made by people they do not feel understand the problems. Create an objective atmosphere in these meetings so that you can measure the value of suggestions e.g. "your idea will take another hour per day but for this kind of defect, which we've seen once, I would suggest it is not worth it." This avoids arguments between two people who both know what the right answer is.

  5. Get the foundations right - It might often be the case that timescales are tight and engineers aren't given enough time to do things properly so getting the foundations right is essential. Having a solid security model, for instance, is much easier to build in from the start and extremely difficult to fit later on. This comes back to having enough input from your team so that they buy into this foundation.

  6. Do not forecast the future - You cannot pre-judge every change that might be made to a system in advance so do not attempt to build in everything at the beginning. If your code is written well then extending it later should not be too much of a challenge. There are however certain things that are reasonable to assume such as a web site might want its look and feel changed in the future. It is reasonable to factor these into the design. Also, people are not paying for all the future possibilities, if you keep the system simple, it will be more reliable and quicker to build. If the customer then turns round and wants a load more functionality, they can pay for the work arising from this.

Friday, 19 November 2010

Using static in .Net

How often do you use the static keyword? Do you disagree with it from a puritan viewpoint that it demonstrates the hiding of functionality that belongs in a separate class? Do you even know what it means?
A static item essentially means there is only one copy. In C#, a static class cannot be instantiated (like a Singleton) and can only contain static members. A normal class can have static members (methods or fields) but these cannot be called on instances of the class, only on the class itself.
As with many things in life, static can be used well and it can be misused. However, I want to suggest that its use, if correct, is both good for performance and is also good for reliability! How do I know? Well look at the following code:

public class Car
{
private static int carCount = 0;
public static Car BuildCar()
{
++carCount;
// build car and return it
}
}

This is an example of lazy or misinformed programming. Why? Because why should a car 'know' about the number of cars that have been built? In reality, you should probably create a factory class that looks like the above and then it wouldn't have to be static right?

public class Car
{
// etc
}

public class CarFactory
{
private int carCount = 0;
public Car BuildCar()
{
++carCount;
// build car and return it
}
}

We could then keep a single instance of this class somewhere and access it to make our cars. This is OK but it misses a trick when it comes to performance. In simple systems, to be fair, it is unlikely to be noticeable but in the above example, although we are only intending for there to be 1 CarFactory instance, the compiler doesn't know this and therefore every call to one of the instance methods has to resolve which instance (i.e. which block of memory) is being accessed. By using static, we tell the compiler that we only want one 'instance' and therefore the class is loaded when the assembly/program is loaded and is instantly available - bit like a direct pointer to methods/fields. This is good!
The same basic truth also concerns methods on classes that can be static i.e. any methods that do not access instance variables in the class. So although they do not have to be static, by making them so, we avoid the resolution of instance. Look at the following:

public class CarFactory
{
public void PaintCar(ref Car car, Color colour)
{
car.Colour = colour;
}
}

The method does not have to be marked static but in this case, it only operates on the values passed into the method and therefore should be marked static for performance reasons. Since, like all good programmers, we never modify methods after they are released, we would never need to change it to non-static, we would simply add another non-static method to do the work differently! In some cases, classes have static methods that operate on passed in values and instance methods that do the same thing on the instance itself:

public class Car
{
public static void PaintCar(ref Car car, Color colour)
{
car.Colour = colour;
}
public void PaintCar(Color colour)
{
this.Colour = colour;
}
}

The second and not to be ignored benefit of using static is that the compiler will enforce the fact that a static method is not allowed to access instance fields and that a static class cannot be instantiated. Why is this important? Because this improves reliability. Take the code snippet above. If our Car class had 20 member fields (instance, not static) and we wanted to test the two functions above, how many combinations would we need to test? In reality, we might look at a function and see that it doesn't access member fields but what if it called other methods which called other methods? Ideally we would need to consider every possible combination of instance fields as well as the parameters passed in. For the static method? We know that we cannot accidentally read or write any instance fields so we simply test for all valid permutations of the input (we might test in our case for a null Car, a null Color, a valid Car and a valid Color to ensure our code is robust).
Of course we always ignore reliability because we think we are good engineers and won't make stupid mistakes. To be perfectly honest, we make plenty and need as much help as we can to avoid them. That's why you should use static wherever possible!

What do you use Service Broker for?

SQL Server has a system called Service Broker which is a queued, asynchronous system enabling calls to be made to procs or simply messages posted onto a queue which can be removed at will and processed. I have seen various people ask when you would use service broker, with its complexity, over a simple call to a stored proc.
The two key words here are queue and asynchronous. A queue ensures that things are processed in order so if, for example, you are running an important financial system, presumably various things must happen in order even if there are thousands happening all the time. In other words if you have 100s of people making changes, you might want to ensure that they are placed in a queue as a single point of entry and which will ensure that the first-come is first-served. You have an amount of control over the queue and whether messages are retained, the way in which the conversation is carried out etc but it is a way of controlling parallel environments.
The second and perhaps most important is the asynchronous nature of the service broker. Why wait for something that you do not need to wait for? If you are sending a letter to someone, you don't post it and then wait at the post box until the recipient tells you they have received it. You either don't care or if you do, you set up a converation with the recipient who can tell you if/when it arrives. Many scenarios exist like this in computing. If I want to cancel my credit card account, once I have requested the close, the Customer Services agent should not have to wait for the whole process to execute, simply to start the ball rolling and then assume it has worked unless the other end fails and reports back. This is especially important if the sequence is fired from a user interface which is waiting for the code to complete before updating the screen.
SQL Server allows you to invoke a stored proc on receipt of a message into the queue which might be useful but you can also leave the queue to fill and service it on a job every so many hours which might help avoid server load or meet various Quality of Service (QOS) requirements (such as all accounts are always updated at 6pm).
My general rule of thumb for software is to keep it simple, although you can sometimes guess how intensive an operation is and go straight for something like service broker, our assumptions can be wrong (a view with 10000 rows runs in a few seconds whereas another seemingly more simple view takes 5 minutes) so best to approach it with a 'if it aint broke, don't fix it' since it is reasonably easy to retrofit Service Broker in place of a stored proc at a later date as needed.

Service Broker Queue not calling Activation Procedure

More fun with the Service Broker today on SQL Server 2005. I had got everything working on a test database and wanted to move the changes into the live database, both on the same server. I scripted all the stuff up and then realise service broker wasn't running. I found this because when I selected from sys.transmission_queue (which should be empty if the message is delivered to a queue), the exception column told me this. I tried to alter database to enable service broker but it wouldn't work with users logged in and I didn't want to boot all few hundred of them off! I wrote a SQL server job that ran the enable code at 5:50am with the following code:

CREATE PROC [dbo].[procEnableBroker]
AS
BEGIN TRY
ALTER DATABASE MyDb SET ENABLE_BROKER WITH ROLLBACK IMMEDIATE ;
END TRY
BEGIN CATCH
ALTER DATABASE MyDb SET NEW_BROKER WITH ROLLBACK IMMEDIATE ;
END CATCH

I needed the second bit in the catch which was some weird thing about needing a new service broker ID? The rollback boots out any uncommitted transactions and does the alter.
This all seemed OK so the messages were now being put on the queue but the queue was not calling the activation proc. If I ran the proc manually, it worked OK so I ended up thinking it was something to do with permissions. I followed some MS advice which as usual is buried amongst a lot of smoke and mirrors and did the following:

EXECUTE AS USER='dbo';
EXEC [dbo].[procQueueActiviationProc]

in order to match the user that it would normally run as (EXECUTE AS OWNER on the queue) and then I got the old, "the remote server could not be accessed because the current security context is not trusted". All I had to do then was to call:

ALTER DATABASE MyDb SET TRUSTWORTHY ON

And then after a little while and poking the queue again, it all came into life. I quite like the idea of Asynchronous calls for some things, this is a proc that updates a load of readonly fields and which takes up to about 10 seconds, something that we needn't wait for.

Friday, 12 November 2010

Cannot retrieve message from Service Broker Queue

I thought I had followed the examples to the letter and yet when I tried to pass a simple message, it seemed to send it but wasn't received by the queue. Eventually I found a view called sys.transmission_queue which holds all messages until they are posted onto a queue (I thought it was just a log so wasn't suspicious that it was still full of messages!). In the transmission_status column for all my messages was "Error 15517 State 1: Cannot execute as the database principal because the principal 'dbo' does not exist, this type of principal cannot be impersonated, or you do not have permission."
It was caused because I was working on a database backup where the security doesn't really map back up properly since I am sysadmin on the server. Anyway, thanks to http://btburnett.com/2008/05/sql-server-2005-service-broker-error-15517.html I realised that all I had to do was call
ALTER AUTHORIZATION ON DATABASE::[dbname] TO [SA]
which I presume sets the database owner to sa and then I was able to make it work. All the previous messages were re-serviced and all ended up on my receiving queue.

Monday, 8 November 2010

Why web security is simply not good enough

The Royal Navy with egg on their face today as some hacker grabs private information from one of their sites and releases it to the public. The trouble here is that what will happen is the site will be secured more tightly and we will "ensure it doesn't happen again" but these things never address the underlying issue.
There are organisations like OWASP who are trying to push for a common framework for web application security but everyone needs to be onboard if it is to implemented. We are talking about service providers, software vendors, the public and governments, if only a small measure of these are involved, it won't happen.
Do people really understand the problems though? Although the common weaknesses in web sites are well known, it would appear that many people are either unaware of these weaknesses or otherwise they are unable or unwilling to do anything about it. Just to give you a heads up, here are lots of common problems which lead to insecure web application processes:

  1. There are simply not enough skilled people for the volume of web sites

  2. The web allows people to experiment and build their own sites which are not easy to distinguish from good sites. The end-user trusts each of these to more-or-less the same degree even though the amateur sites are unlikely to follow any good practices.

  3. You can buy or obtain off-the-shelf products to build your own sites and simply assume they are secure by default. These systems are not necessarily built well but you have to rely on experience and a wide user base to find and/or fix any insecure areas.

  4. There is no way to physically stop people from either not implementing security or making a mistake when they code a site.

  5. Many sites are fixed and updated regularly so it is only likely that at some point, unless there is a secure deployment regime, that a security hole is going to be created.

  6. People tend to go with the easy approach rather than the secure approach. For instance, people use the database admin login to access the database meaning a hacked site is as good as an open door to all your data

  7. Even when using secure practices, it is possible to use them incorrectly (such as using a flawed regular expression when testing for valid user input). Some of these might have inherent bugs or incomplete functionality.

  8. People who write and deploy web applications do not require formal training of any sort to at least demonstrate they have been taught about secure practices, even if they cannot be forced to use them.

  9. Although frameworks like ASP.Net and PHP have various secure controls available, they are easy to ignore, whereas these frameworks could actually insist on secure programming even if some people don't like it. For instance, by default, a text box control should not allow unusual punctuation, you should have to manually allow it on a per-item basis if required. Safety by default

  10. Whoever has written the site, there is no worldwide accepted certification that ensures that a site is secure. There are mechanisms to prove who the site belongs to and various specific bodies like the Payment Card Industry who have their own audit procedures but nothing required for the general population.

  11. By default, once someone has connected their computer to the internet they are both a potential menace and a potential weakness, it is like a load of criminals being moved into a residential area without telling anyone and then being surprised that the crime rate has increased.


Obviously any discussion about what could be done would be met with disapproval and accusations of censorship or the like but in reality, the system does not need everybody's approval since it could be done as an "exclusive club". You would need a way to achieve and prove certification including proving that your certification is valid (which could use digital certificates) and this would involve an audit of procedures/processes and functionality - possibly against a specific release and possibly in general for your organisation or as an individual? This certificate then qualifies you for the 'green flag club' which indicates to end users that a site is about as good as it could be and which would then ultimately allow people to choose to run in ultra-protected mode (for their safety/benefit) which would then cause people who write sites to get their site certified so that they can then access the club (and provide the benefit of a more secure site). The un-certified sites can live in the badlands!
Right, when do we start?

Tuesday, 2 November 2010

Devil in the detail - customising the ASP BulletedList

When you do standard easy programming, you can cover a lot of ground very quickly. Modern tools and class libraries are very useful and do most things very well. When it comes to more specific programming, the devil is very much in the detail. For example, the other day I wanted to replace some hard-coded HTML generation, which made a tabbed divider, with a more standard ASP control. Since our old tab control was based on a <ul>, I found the closest control was an ASP .Net BulletedList.
It was easy at first since by setting various options on the list, I could get it to generate a <ul> with items and using the link button mode, it all looked pretty good. I applied the original CSS styles and I thought I was 99% there.
Oh, I thought, I really need to distinguish the selected item in the list since they represent tabs, I want to style the selected one differently. Since the bulleted list is supposed to be, well, a bulleted list, all the item selection from the base class ListControl has been overridden with NotImplementedExceptions and even if an item is selected by getting the individual ListItem and setting Selected = true, the item is not rendered any differently.
Well, this wasn't a problem since OO allows us to inherit and extend the functionality, I implemented my own version of RenderContents() in a new class, inherited from BulletedList which renders an attribute for class in the LI if it is selected, this was all great - OO is amazing! I then wanted it to remember the selected item between postbacks and to fire the SelectedIndexChanged ListControl event when the tab had changed. This is where it all started kicking off. For a start, since BulletedList has overridden the ListControl SelectedIndex and SelectedValue setters, I could not call the original versions which were fine, I had to use the Red Gate reflector tool to find the original code and duplicate it in my class overriding the overridings! I then found various references to internal and private functions which, again, I needed to duplicate in my code in order to use the code I had copied from the internal workings of the ListControl (I only wanted to make small changes!). I then realised that since the BulletedList already handle the click event and did not fire the SelectedIndexChanged even from ListControl, I would not easily be able to do what I wanted.
In the end, I decided the easiest thing was to inherit directly from ListControl which would give me all the Item selection code and then add in anything from the BulletList class that I needed (fortunately not all of it!) and then I could make my own class handle the postback event when it was clicked and fire the OnSelectedIndexChanged event from ListControl.
I removed any of the display modes and bullet types that I was not using to make it neater and got rid of the start bullet number. I also got rid of the various local variables used to "cache" values for loops. It seems that MS did not consider very well how their code would be specialised in sub-classes which is why there is a hotchpotch of public, protected and private/internal functions meaning that trying to base your code on the original does not work without lots of duplication (unless there are other public utlities to do the same things).
Anyway, if you're interested in the code:

///
/// A specialisation of the list control which is based on link buttons and tab styles
///

public class SelectableTabControl : ListControl, IPostBackEventHandler
{
protected override void OnInit(EventArgs e)
{
base.OnInit(e);
this.CssClass = "tablist";
}

///
/// Handle the data being bound
///

///
/// Ensure a default item is selected
protected override void OnDataBound(EventArgs e)
{
base.OnDataBound(e);
if (SelectedIndex == -1 && Items.Count > 0)
{
SelectedIndex = 0;
}
}

///
/// Add attributes of this control to the HTML
///

/// The HTML output stream
protected override void AddAttributesToRender(HtmlTextWriter writer)
{
string uniqueID = this.UniqueID;
if (uniqueID != null)
{
writer.AddAttribute(HtmlTextWriterAttribute.Name, uniqueID);
}
base.AddAttributesToRender(writer);
}

///
/// Render the HTML for this control
///

/// The HTML output stream
protected override void Render(HtmlTextWriter writer)
{
if (this.Items.Count != 0)
{
base.Render(writer);
}
}

///
/// Prevent this control from being given child controls
///

///
protected override ControlCollection CreateControlCollection()
{
return new EmptyControlCollection(this);
}

///
/// Render the individual elements for this list
///

///
/// The default implementation does not render a class for selected items
protected override void RenderContents(HtmlTextWriter writer)
{
for (int i = 0; i < this.Items.Count; i++)
{
if (this.Items[i].Attributes != null)
{
this.Items[i].Attributes.AddAttributes(writer);
}
writer.AddAttribute(HtmlTextWriterAttribute.Class, this.Items[i].Selected ? "selecteditem" : String.Empty );
writer.RenderBeginTag(HtmlTextWriterTag.Li);
this.RenderBulletText(this.Items[i], i, writer);
writer.RenderEndTag();
}
}

///
/// Return the tag to use for the overall control
///

protected override HtmlTextWriterTag TagKey
{
get { return HtmlTextWriterTag.Ul; }
}


///
/// Render the individual items from the list
///

///
///
///
/// Replacement for base class function which references inaccessible values. This only includes the LinkButton style
protected void RenderBulletText(ListItem item, int index, HtmlTextWriter writer)
{
if (!this.Enabled || !item.Enabled)
{
writer.AddAttribute(HtmlTextWriterAttribute.Disabled, "disabled");
}
else
{
writer.AddAttribute(HtmlTextWriterAttribute.Href, this.GetPostBackEventReference(index.ToString(CultureInfo.InvariantCulture)));
}
if (AccessKey.Length != 0)
{
writer.AddAttribute(HtmlTextWriterAttribute.Accesskey, AccessKey);
}
writer.RenderBeginTag(HtmlTextWriterTag.A);
HttpUtility.HtmlEncode(item.Text, writer);
writer.RenderEndTag();

}

#region Code duplicated from internal .Net classes
///
/// From System.Web.UI.WebControls.BulletedList
///

///
///
private string GetPostBackEventReference(string eventArgument)
{
if (this.CausesValidation && (this.Page.GetValidators(this.ValidationGroup).Count > 0))
{
return ("javascript:" + GetClientValidatedPostback(this, this.ValidationGroup, eventArgument));
}
return this.Page.ClientScript.GetPostBackClientHyperlink(this, eventArgument, true);
}

///
/// From System.Web.UI.Util
///

///
///
///
///
private static string GetClientValidatedPostback(Control control, string validationGroup, string argument)
{
string str = control.Page.ClientScript.GetPostBackEventReference(control, argument, true);
return (GetClientValidateEvent(validationGroup) + str);
}

///
/// From System.Web.UI.Util
///

///
///
private static string GetClientValidateEvent(string validationGroup)
{
if (validationGroup == null)
{
validationGroup = string.Empty;
}
return ("if (typeof(Page_ClientValidate) == 'function') Page_ClientValidate('" + validationGroup + "'); ");
}

///
/// From System.Web.UI.Control but minus supports event validation which is far too esoteric (and inaccessible)
///

///
///
internal void ValidateEvent(string uniqueID, string eventArgument)
{
if ((this.Page != null))
{
this.Page.ClientScript.ValidateEvent(uniqueID, eventArgument);
}
}
#endregion

///
/// Gets whether the All tab is selected (which is always the last one)
///

public bool AllTabSelected
{
get { return SelectedIndex == Items.Count; }
}

#region IPostBackEventHandler Members
///
/// Handle a postback event occuring.
///

/// The value of the event argument
/// This event is raised when the page is posted back and if the postback was caused by this control then the eventArgument is the index of the tab that was clicked
public void RaisePostBackEvent(string eventArgument)
{
ValidateEvent(this.UniqueID, eventArgument);
if (this.CausesValidation)
{
this.Page.Validate(this.ValidationGroup);
}
SetPostDataSelection(Convert.ToInt32(eventArgument));
this.OnSelectedIndexChanged(EventArgs.Empty);
}

#endregion
}