Code Quality – The Blog of Colin Mackay

Setting up Code Coverage in Team City

The project that I’m working on has a rather spotted history for unit tests until now. There would be periods of people actually writing them, but then come unstuck when a random test failed and eventually the tests would be allowed to rot a bit, until something spurned everyone back to life again.

However, unit testing is something we, as a team, are getting better at and to aid the continual improvement of code base I turned on code coverage metrics in Team City earlier this week, and set it to fail the build if the metrics fell below a certain threshold. That way if test coverage dropped the build would fail, and since we’re also driving towards implementing a continuous delivery process failing builds will mean we can’t deploy either. This is good. It means we will not be able to deploy poor quality code.

What does code coverage on a build server get you?

Put simply, it shows you what code was covered during the automated tests run on the build server. You can then use this information to direct focus on areas that are not sufficiently tested to drive up code quality.

Turning on code coverage means that Team City will be able to display those metrics to you after the build is complete. You can drill in to the metrics to get a better understanding of what code your tests are exercising, and perhaps more importantly, what code isn’t being exercised.

When Team City builds a project it will run several build steps, at least one of which should be some sort of testing suite and you may have more if you have a mix of test types (e.g. unit tests and integration tests). The code coverage metrics are generated during the testing steps. At the end, you can examine the results just like any other aspect of the build.

This is an example of what the “overview” looks like after a build it run with code coverage turned on:

Nightly Build Overview

I like that it also shows the differences between this and the previous build as well as the overall numbers.

The statistic that I think is probably the most important is the statements percentage as that is unlikely to change as much as if you split out code in to separate classes or methods. It also means that you get a better idea of coverage if you have larger methods with high cyclomatic complexity or large classes.

Incidentally, splitting large methods and classes into smaller ones should make things easier to test, but I’d rather not have that impacting the metric we are using for code coverage. The number of statements remains relatively similar even after splitting stuff out and also since there so many more statements than methods or classes even if there is a slight change in the number the overall impact on the percentage metric is minimised.

If you want to drill into the detail, you can go into “full report” and see the break down by assembly. For example:

Overall Coverage Summary

As you can see it gives a breakdown of coverage by assembly. Each of the assembly names is a link allowing you to drill in further.

Overall Coverage Summary for Assembly

Eventually you can get as far down as the individual file and this is where the power really lies. When viewing an individual file you can see individual lines of code that have been exercised in some way (in green) and ones that have not (in red).

Coverage Summary for Type

It is worth noting that just because a line is green it does not mean it was exercised well. Code Coverage is just a metrics tool. It just says something was done, not that it was correct. This is vitally important to remember. However, it is still a valuable tool to be able to see areas where there is no code coverage at all.

Setting up Code Coverage in Team City

On the Team City dashboard go to the project you are interested in, click the down arrow next to the build name and select “Edit Settings” from the menu.

Team City Dashboard with Build context menu

On the Build Configuration Settings page, click on “Build Steps” in the side bar.

Build Configuration Settings sidebar

Then, find the step that performs the tests, and click edit.

Towards the bottom of the settings is an entry marked “.NET Coverage tool”. Select “JetBrains dotCover” as this is built in to Team City. You can of course choose one of the other coverage options, but you’ll have to configure that yourself.

.NET Coverage settings

You don’t need to enter a path to dotCover unless you are installing a different version to the one Team City has built in. (We’re using Team City 8.1 and it has an older version of dotCover, but it works well enough for what we want.)

In the filters section you need to put in details of the code you want to cover. I’ve just put in a line for all assemblies, then some lines to exclude other assemblies. I found that it sometimes picks up assemblies that were brought in via NuGet and shouldn’t be covered as it will skew your statistics. You can see the assemblies it picked up automatically by drilling into the code coverage from the overview page for the build as I showed above. I also excluded assemblies that are purely generated code, like the assembly that contains just an EDMX and the code autogenerated from it.

The basic format of the filters is:
+:assembly=<assembly-name> to include an assembly, wild cards are supported.
-:assembly=<assembly-name> to exclude an assembly, wild cards are supported.

However, you can filter on more that just the assembly level. More detail is available from Team City’s online help.

Once you’ve set this up hit “Save” at the bottom of the page.

You can now perform a test build to see how it works by pressing “Run…” at the top of the page.

When the run is complete you can view the results by clicking on the build on the main page.

Getting to the build report

From the overview you can drill in and see the assemblies that were picked up, as shown above. If there are any assemblies that shouldn’t be there (e.g. third party libraries) you can then add them to the filter in the build step configuration to make sure they are excluded from the code coverage reports.

Finally, to ensure that you don’t fall backwards and ensure that you maintain the code coverage above a certain threshold you can add a failure condition to the Build Configuration Settings.

Go back to the build settings page, and then to the “Failure Conditions” section.

Failure Conditions section

Then press “+ Add failure condition” and a dialog will pop-up.

Add Failure Condition popup

Select “Fail build on metric change” from the drop down.

Then, in the “Fail build if” section, select “its” “percentage of statement coverage”, “Is compared to” “constant value” “is” “less” than <percentage-value> “default units for this metric”

For <percentage-value> take the value form your first reference build subtract about half a percent then round down. There will naturally be minor fluctuations up-and-down on each build and you don’t want to be too strict either.

Then press “Save”. The page will refresh and you’ll see the failure condition added to the table at the bottom of the page.

Additional Failure Conditions

Now each time this build is run it will run code coverage and if it drops below the threshold it will fail the build.

Code Review: Making a drop down list out of an enum

I’ve come across code like this a couple of times and it is rather odd:

IEnumerable<CalendarViewEnum> _calendarViewEnums =
    Enum.GetValues(typeof(CalendarViewEnum)).Cast<CalendarViewEnum>();
List selectList = new List<SelectListItem>();
foreach (CalendarViewEnum calendarViewEnum in _calendarViewEnums)
{
  switch (calendarViewEnum)
  {
    case CalendarViewEnum.FittingRoom:
      selectList.Add(new SelectListItem { 
          Text = AdminPreferencesRes.Label_CalendarViewFittingRoom, 
          Value = ((int)calendarViewEnum).ToString(CultureInfo.InvariantCulture), 
          Selected = (calendarViewEnum == CalendarViewEnum.FittingRoom) 
          });
      break;
    case CalendarViewEnum.Staff:
      selectList.Add(new SelectListItem { 
          Text = AdminPreferencesRes.Label_CalendarViewStaff, 
          Value = ((int)calendarViewEnum).ToString(CultureInfo.InvariantCulture), 
          Selected = (calendarViewEnum == CalendarViewEnum.Staff) 
      });
    break;
    case CalendarViewEnum.List:
      selectList.Add(new SelectListItem { 
          Text = AdminPreferencesRes.Label_CalendarViewList, 
          Value = ((int)calendarViewEnum).ToString(CultureInfo.InvariantCulture), 
          Selected = (calendarViewEnum == CalendarViewEnum.List) 
      });
    break;
    default:
      throw new Exception("CalendarViewEnum Enumeration does not exist");
  }
  return selectList.ToArray();
}

So, what this does is it generates a list of values from an enum, then it loops around that list generating a second list of SelectListItems (for a drop down list box on the UI). Each item consists of a friendly name (to display to the user), a integer value (which is returned to the server on selection) and a Boolean value representing whether that item is selected (which is actually always true, so it is lucky that MVC ignores this the way the Drop Down List was rendered, otherwise it would get very confused.)

Each loop only has one possible path (but the runtime doesn’t know this, so it slavishly runs through the switch statement each time). So that means we can do a lot to optimise and simplify this code.

Here it is:

List<SelectListItem> selectList = new List<SelectListItem>();
selectList.Add(new SelectListItem { 
    Text = AdminPreferencesRes.Label_CalendarViewFittingRoom, 
    Value = ((int) CalendarViewEnum.FittingRoom).ToString(CultureInfo.InvariantCulture) 
  });
selectList.Add(new SelectListItem { 
    Text = AdminPreferencesRes.Label_CalendarViewStaff, 
    Value = ((int)CalendarViewEnum.Staff).ToString(CultureInfo.InvariantCulture) 
  });
selectList.Add(new SelectListItem { 
    Text = AdminPreferencesRes.Label_CalendarViewList, 
    Value = ((int)CalendarViewEnum.List).ToString(CultureInfo.InvariantCulture) 
  });
return selectList.ToArray();

There is one other another bit of refactoring we can do. We always, without exception, return the same things from this method and it is a known fixed size at compile time. So, let’s just generate the array directly:

return new []
{
  new SelectListItem { 
      Text = AdminPreferencesRes.Label_CalendarViewFittingRoom, 
      Value = ((int) CalendarViewEnum.FittingRoom).ToString(CultureInfo.InvariantCulture) 
    },
  new SelectListItem { 
      Text = AdminPreferencesRes.Label_CalendarViewStaff, 
      Value = ((int)CalendarViewEnum.Staff).ToString(CultureInfo.InvariantCulture) 
    },
  new SelectListItem { 
      Text = AdminPreferencesRes.Label_CalendarViewList, 
      Value = ((int)CalendarViewEnum.List).ToString(CultureInfo.InvariantCulture) 
    }
};

So, in the end a redundant loop has been removed and a redundant conversion from list to array has been removed. The code is also easier to read and easier to maintain. It is easier to read because the cyclomatic complexity of the method is now one, previously it was 5 (one for the loop, and one for each case clause in the switch statement [assuming I’ve calculated it correctly]). The lower the cyclomatic complexity of a method is the easier it is to understand and maintain as there are less conditions to deal with. It is easier to maintain because now instead of a whole new case statement to be added to the switch statement a single line just needs to be added to the array. The redundant Selected property has also been removed.

There are still ways to improve this, but the main issue (what that loop is actually doing) for anyone maintaining this code has now been resolved.

Code Review: FirstOrDefault()

I regularly review the code that I maintain. Recently, I’ve come across code like this fairly often:

someCollection.FirstOrDefault().Id

I cannot rightly comprehend why anyone would do this.

FirstOrDefault() returns the first item in a sequence or the default value if it doesn’t exist (i.e. the sequence is empty). For a reference type (classes, basically) the default value is null. So using the value returned by FirstOrDefault() without a null check is only valid for when the sequence contains a value type (e.g. int, decimal, DateTime, Guid, etc.)

In the example above if someCollection is an empty list/array/collection/whatever then FirstOrDefault() will return null and the call to the Id property will fail.

Then you are left with a NullReferenceException on line xxx but you don’t know if it is someCollection, or the returned value from FirstOrDefault() which then wastes your time (or the time of someone else who is having to debug it).

So, if the sequence must always contain items then use First(), in the exceptional event that it is empty the call to First() will throw a more appropriate exception that will help you debug faster. If it is perfectly valid for the sequence to be empty then perform a null check and change the behaviour appropriately.

Why should you be returning an IEnumerable

I’ve seen in many places where a method returns a List<T> (or IList<T>) when it appears that it may not actually really be required, or even desirable when all things are considered.

A List is mutable, you can change the state of the List. You can add things to the List, you can remove things from the List, you can change the items the List contains. This means that everything that has a reference to the List instantly sees the changes, either because an element has changed or elements have been added or removed. If you are working in a multi-threaded environment, which will be increasingly common as time goes on, you will get issues with thread safety if the List is used inside other threads and one or more threads starts changing the List.

Return values should, unless you have a specific use case in mind already, be returning an IEnumerable<T> which is not mutable. If the underlying type is still a List (or Array or any of a myriad of other types that implement IEnumerable<T>) you can still cast it. Also, some LINQ expressions will self optimise if the underlying type is one which better supports what LINQ is doing. (Remember that LINQ expressions always take an IEnumerable<T> or IQueryable<T> anyway so you can do what you like regardless of what the underlying type is).

If you ensure that your return values are IEnumerable<T> to begin with yet further down the line you realise you need to return an Array or List<T> from the method it is easy to start doing that. This is because everything accepting the return value from the method will still be expecting an IEnumerable<T> which List<T> and Array implement. If, however, you started with a List<T> and move to returning an IEnumerable<T> then because so much code will have the expectation of a List<T> without actually needing it you will have a lot of refactoring to do just to update the reference types.

Have I convinced you yet? If not, think about this. How often are you inserting items into a collection of objects after the initial creation routine? How often do you remove items from a collection after the initial creation routine? How often do you need to access a specific item by index within a collection after the initial creation routine? My guess is almost never. There are some occasions, but not actually that many.

It took me a while to get my head around always using an IEnumerable<T>, until I realised that I almost never require to do the things in the above paragraph. I almost always just need to loop over a collection of objects, or filter a collection of objects to produce a smaller set. Both of those things can be done with just an IEnumerable<T> and LINQ.

But, what if I need a count of the objects in the List<T>, that would be inefficient with an IEnumerable<T> and LINQ? Well, do you really need a count? Oftentimes I just need to know if there are any objects at all in the collection, I don’t care how many object there actually are, in which case the LINQ extension method Any() can be used. If you do need a count LINQ is clever enough to work out that the underlying type may expose a Count property and it calls that (anything that implements ICollection<T> such as arrays, lists, dictionaries, sets, and so on) so it is not iterating over all the objects counting them up each time.

Remember, there is nothing necessarily wrong with putting a ToArray() to ToList() before returning as a reference to an IEnumerable<T> something to which a LINQ expression has been applied. That removes the issues that deferred execution can bring (e.g. unexpected surprises when it suddenly evaluates during the first iteration but breaks in the process) or repeatedly applying the filter in the Where() method or the transformation in the Select() method.

Just because an object is of a specific type, doesn’t mean you have to return that specific type.

For example, consider the services you actually need on the collection that you are returning, remembering how much LINQ gives you. The following diagram shows what each of the interfaces expect to be implemented what a number of the common collection types implement themselves.

Incidentally, the reason some of the interfaces on the Array class are in a different colour is that these interfaces are added by the runtime. So if you have a string[] it will expose IEnumerable<string>.

I’d suggest that as a general rule IEnumerable<T> should be the return type when you have anything that implements it as the return type from the method, unless something from an ICollection<T> or IList<T> (or any other type of collection) as absolutely desperately in needed and not just because some existing code expects, say, an IList<T> (even although it is using no more services from it that it would had it been an IEnumerable<T>).

The mutability that implementations of ICollection<T> and IList<T> give will prove problematic in the long term. If you have a large team with members that don’t fully understand what is going on (and this is quite common given the general level developer documentation) they are likely to change the contents of the collection without understanding its implications. In some situations this may fine, in others it may be disastrous.

Finally, if you absolutely do need to return a more specific collection type then instead of returning a reference to the concrete class, return a reference to the lowest interface that you need. For example, if you have a List<T> and you need to add further items to it, but not at specific locations in the list, then ICollection<T> will be the most suitable return type.

Tip of the Day #19: Create a list of objects instead of many lists of values

I?ve been reviewing some code and I came across something that jars. What is wrong with this is many-fold. Essentially, instead of encapsulating an related data into an entity that describes the whole the developer had created silos of data values, and you’d better hope that nothing went awry with any of it.

It looked like this:

List<string> addOnNames = new List<string>();
List<string> addOnDescriptions = new List<string>();
List<string> addOnCodes = new List<string>();
List<decimal> addOnBasePrice = new List<decimal>();

Okay, so if you don’t yet find it jarring, here are some of the things that are wrong with this structure.

To iterate through and operate on a single “add on” you have to do something like this:

for(int i = 0; i < addOnNames.Count; i++)
{
    string name = addOnNames[i];
    string description = addOnDescriptions[i];
    string code = addOnCodes[i];
    decimal basePrice = addOnCode[i];
    // Do stuff that operates on an "Add On" here
}

That?s quite a bit of work just to get at the values you need in a particular loop iteration.

If you need to pass the lists on to methods, you have to pass each in its own parameter. Method signatures start to become needlessly large. In the simplified example above (yes, the real code had a lot more in it) there are three extra parameters that need to be passed around to each method call that needs to act on a collection of add-ons.

private void DoSomethingToACollectionOfAddOns(
                     List<string> addOnNames,
                      List<string> addOnDescriptions,
                      List<string> addOnCodes,
                     List<decimal> addOnBasePrice)
{
    // Do something.
}

It is also quite difficult to use LINQ to query what is effectively an AddOn entity.

Unless you are very careful, your lists can become out of sync, and when that happens all sorts of strange and difficult to trace bugs enter into the system. In the code I was reviewing there was an edge case where by one of the lists didn?t get updated when it was initially populated. Because all the lists are assumed to be the same length the first time that they were iterated over, what should have been the final entity couldn?t be retrieved on one of the lists because it didn?t exist. As this happened well after the creation of the lists and in a method called several levels deep it was quite a job working out where the original bug was coming from.

Solution

What needs to happen here is that an entity class is created for an add on. Each of these lists indicates a field or property in an entity class. The class might simply look something like this:

public class AddOn{
    public string Name { get; set; }
    public string Description { get; set; }
    public string Code { get; set; }
    public decimal BasePrice { get; set; }
}

This encapsulates everything about a single add on entity in one place. If you want a collection of these objects you can do something like this:

    List<AddOn> addOns = new List<AddOn>();

If you want to loop over them you don?t have to write lots of code to get all the elements out of a variety of lists a simple foreach will suffice (unless you need to also know the index)

    foreach(AddOn addOn in addOns){}

If you need to pass the entity around, or a collection of the entities around then you only need to pass one parameter into a method.

    private void DoSomethingToACollectionOfAddOns(List<AddOn> addOns) {}

Using LINQ becomes much easier because now you have everything encapsulated in the one place. I can?t even imagine the convoluted joins that would be needed to process the individual values in a LINQ expression otherwise.

When creating the initial collection, if any particular property is not needed then it can be simply ignored, if need be a default value can be set in the constructor. Then you never have an issue with one collection being out of sync with another. You no longer have to worry about synchronising collections, everything to do with a single instance of the entity is in one place.

Tip of the Day #18: Dealing with data rounding issues

If you have data coming in from a database, web service or other source external to your application and it contains, say for example, price information then do not round it. Don?t attempt to apply any form of formatting to it regardless of how much the client insists that the data will be in this form (e.g. all prices at go live will be in round pounds).

Yes, having whole rounded pounds, without having to display the pennies, on the front end looks nice and pretty. However, the display of whole rounded pounds on the front end is a rendering issue and should always be left to the code that is rendering the user interface. The rounding should never take place close to the point that the information is extracted from its source.

Why? Because eventually the client will want things displayed differently, to put prices with pennies in it, and you have to display those pennies. Now you don?t have to just update the rendering code to display the pounds and pence, but you also have to track down where that rounding occurs and stop it from happening in the first place.

But that?s not all. If you round data, effectively truncating it, early then subsequent calculations will be wrong. In fact, rounding isn?t just like truncating the data. Truncating is just removing precision. Rounding can change the value. That can cause even more havoc later on.

So, why round that early anyway, especially if the data is arriving in a certain way in the first place? Well, I can only guess that perhaps because back before anything went live, in the test system they weren’t. So, just to make the test system look like the eventual live system the prices were rounded the moment they arrived in the application just to be sure they were in round pounds.

The lesson, clients change their minds and business logic should always operate on the cleanest data, unsullied by rendering constraints wherever possible. This means that calculations based on that data will remain correct, even if other factors change.

Follow up on what not to develop

Back in May I wrote about a substandard website I attempted to use in an article entitled “What not to Develop”. I also sent the hotel an email at the same time telling them of the failing of their website, however, I never got a response.

When the post went live initially, I got asked on twitter to name and shame the company in question. I suppose publically decrying a company has the effect that if people start doing that then companies will be pressurised in to providing a better service or product. These days I do not to put in a blog post the name of the company in question until I’ve given them a chance to respond to any email I might have sent. I sent the email on 16 May 2009 at 17:21 (BST), I think that’s quite enough time for a response.

I’ve decided to publish some more details so that people can at least learn from the mistake and not repeat them elsewhere. Essentially, this is an extract of the email (slightly reformatted to fit this blog)

Hello,

I tried to book on your website last night and it didn’t work – it advertised a rate to me then refused to book it. I then tried to use your Contact Us page to send you a message and that also broke and said “The web site you are accessing has experienced an unexpected error. Please contact the website administrator. ”

I don’t know who the web site administrator is, but I can guess it is someone employed by TIG Global given this news story: http://www.hospitalitynet.org/news/4036652.search. Personally, if that is the quality they are delivering I wouldn’t use them again as they are not very good and are at best turning away potential customers and at worst exposing you to needless risk.

In order to [help you to] track down the errors I’ve gone back and replicated the initial problem annotating the pages as I go. You will find a number of graphics files attached.

In [the above image] I show the initial details of my availability search. Check in Friday 31st July, check out Sunday 2nd Aug. 1 adult, 0 children.

In [the above image] I show the next page. This was a pop-up, so opened a new window. The details at the top are correct and match what I’d previously entered. The description of the “Weekend Advanced Purchase” sounds perfect “Valid Friday-Sunday throughout 2009”. I see that it is £150 for the “Total price of the stay”. I press the book button.

In [the above image] I show the next page. This was another pop-up, so opened a second window. I now have 3 windows open just for your hotel. (Is this really necessary?). I spot that the number of nights has increased to 3, so I go to change it back to two. I then get an unhelpfully terse error message that says “Minimum stay: 3” [See the next image]

At this point I’m some what irritated by the experience so go hunting for your contact us page. I see that it is a form only without an email address. I fill in the form and when I’m ready I press the “Submit” button. At this point I get an error page back that includes the message “The following information is meant for the website developer for debugging purposes.” You might want to tell those developers that this information is also useful for attackers and they shouldn’t be displaying it to the public. If the developers were any good what they would have done is get the website to log the information internally and display a general message to the user. If they wanted to tie up a user’s experiences with what is in the log then they might also include a randomly generated (say a GUID – globally unique identifier) identifier that is put in the log and displayed so a user can refer to when explaining what problems they were having at the time.

The error message that should have never been displayed is [as follows].

The details in the error page also contain my original complaint. I think I now understand where the American formatting of culture specific information (e.g. dates) is coming from.The company that produced your website was American and in their arrogance just assumed everyone else was just as comfortable using MONTH/DAY/year. I suspect that same arrogance was also responsible for the other failings I’ve pointed out here.

Regards,

Colin.

So, there you are. The hotel is the Southwark Rose Hotel, and their website was produced by TIG Global. (I’ve recently noticed it actually says that at the bottom of the web pages and I need not have searched for relevant press releases!). Incidentally, you can click on any of the graphics to be taken to my Flickr account to see the full sized version.

Tip of the Day #12 (Hard coded values)

Don’t hard code VAT or other values that can change, even if they don’t change all that often. Additionally, if you really must hard coded values in a program then make it a named constant rather than a literal value so that it can be tracked down by name. That will make it easier on the person having to maintain the program.

What not to develop

I was recently looking to book a hotel in Southwark in London. I thought I’d found the perfect hotel, it was inexpensive (by London standards) and close to where I would be visiting. They also had availability on an offer for £75 per night, so long as you checked in and out on specific days, which I happened to be doing. It looked perfect.

But then things started to go wrong.

I selected the rate from the availability page and clicked the “Book” button. The next page popped up (it opened a new window) and the details were pre-populated. However, it had changed the number of nights from 2 to 3. I didn’t want 3 nights, so I changed it back to 2 and I got a rather terse message saying “Minimum Stay: 3”. I’m happy to accept that style of message from a compiler, but not from a public facing website.

I went back and repeated the process wondering if I’d somehow clicked on the wrong rate. I double checked everything this time. Date is correct (but in an American format on a .co.uk website), number of people (1), number of nights (2), number of rooms (1), the room description explicitly gives the rules for the stay conditions for the rate. I meet all the conditions that are presented to me. I press “Book” again…

And it has pre-populated everything again and added an extra night on. I don’t want an extra night! Why even present me with a rate that I can’t have because it doesn’t meet my needs.

By this point I’m more than a wee bit frustrated. So I take off to the website’s contact us page. Instead of providing an email address there is a form to fill in. So, I write a description of the issues I was seeing on their site at which point the site fails again. It failed spectacularly badly. If it had taken me to an error page I would have just shrugged my shoulders and gone off elsewhere. But no, it decided to throw up its internals at me. It vomited details of the SQL Statement that failed, stack traces and so on.

It even had the audacity to tell me that “The following information is meant for the website developer for debugging purposes.” It might have well have said “The following information is meant for an attacker so they can destroy our server.”

So, back to my title, what not to develop. There were many failings on this website that I could see. The user experience was poor to start with and it then descended in to abject failure when it vomited its guts up at me.

1. Don’t use pop-up windows; browsers may block them; they cause confusion for some users. Absolutely do not have a pop-up out of a pop-up; it clutters my screen with needless windows.

2. Don’t have a disconnect between the display locale on the site and the TLD. If you have a geographic TLD then display information in a way that consistent with the culture of that location. e.g. Do not display dates in Month/Day/Year format when you are serving pages on a .co.uk domain. If you have customers from overseas and want to localise content for them then offer that ability, but default to your own locale if you don’t know their preference. Some websites try to be clever and will detect based on the IP of the user but even this isn’t 100% accurate. I’m located in Glasgow, but if you use a IP geo location service it shows me in Greater Manchester.

3. If a user has told you their needs do not present rates that do not meet those needs. If you do want to show near alternatives then make it clear that the details entered do not match the rate displayed, but some minor changes will get the user the rate. Put this information at the bottom or in a different colour. Anything that makes it easily distinguishable.

4. Don’t allow a business rule to mismatch the user friendly description. Make sure that the description of the rate actually matches the business rules that will be used to enforce the rate. If you have a rate that is described to the user as from X to Y don’t have the underlying business rules enforce a stay from X to Z. That will just irritate people.

5. Don’t give users terse error messages; it is unpleasant and unfriendly. If a user has made a mistake then gently point it out.

6. Don’t just send data to the database without validating it first. If a user has typed something that is too long for the column in the database for which it is destined then the software controlling the website should never have attempted to send it to the database in the first place.

7. Don’t display information that could be useful to an attacker. Don’t display stack traces, SQL Statements, system generated error messages, code snippets, etc.

Technorati Tags: fail,website,software development,sql,debug,attack,stack trace,business rules

Rant of the day: Learn to frickin' count!

I was in a shop recently and I bought 6 items at £5 each. A total price of £30, even I can manage that mental arithmetic without resorting to a calculator. However, the till decided that the total price was £30.01. For a penny I really can’t be bothered to argue, but it got me thinking about code quality and wondering about what awfulness must be sitting in that system to create such a simple basic mistake.

My colleagues are probably all aware of my views on code quality. I rant daily whenever I see examples on ineptitude by people that are paid money to write code. I read and respond on forums in order to help others learn their craft, or just get unstuck when they accidentally dig themselves in a hole. However, I see on an almost daily basis these days people posting their homework questions with no apparent attempt to at least try to work it out from themselves.

Take this example I found on Code Project a while ago:

I need to know how to do some simple things with arrays please help with any!
1.Find largest or smallest value
2.Count how many times a given value is in the array
3.Count the number of even or odd integers in the array
4.Add up the sum and compute the mean
5.Create another array of the same size containing the same values in reverse order
Thanks!

This is very obviously an exercise from an introductory course on the language they were studying. They just want someone to give them an answer that they can copy and paste. If this is what they are like now, imagine what they will be like years down the road writing commercial software.

I’ve seen lots of evidence over the years of people writing software by copy and pasting examples from the internet without thought of what is actually going on. This results in slow, bloated, inefficient code that is integrated very badly with the rest of the system, hard to read, hard to debug, and is just generally a complete mess.

If you are tempted to copy and paste some code snippet from the internet for your application then stop and think first. Do you actually understand the code? If not, then don’t copy and paste it. If you don’t understand it, how will you debug it?

I would say that if you are tempted to copy and paste from the internet that you create a very small test application first, paste it in to that and learn how it works. Once you understand how it all fits together and how it works you can then write a version that will integrate in to your application.

While you are at it, write some unit tests to go with it. Make sure you test for edge cases, make sure you test for some normal cases too. If you ever get a bug, then add a test that replicates the bug. So if someone suddenly discovers your software things that 5 times 6 equals 30.01 you can add a test for it, fix the bug and redeploy the system. Hopefully, this would have been caught before the public get a chance to see the glaring error and write blog posts about it.