Misc, Software Development

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.

Misc

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.

Misc

What a waste of money by Currys (but win for GAME)

As the end of last year, I was at a Microsoft event where we got to see a number of new Microsoft technologies. At this event I got my first chance to have a look at the XBOX 360 Kinect. Since I’m not a gamer I hadn’t paid much attention to what a Kinect was until I actually saw one and had a play on one. Then I instantly wanted one. If you’ve never seen it, even if you are not into computer games, I would highly recommend you have a look.

Anyway, since arriving back home I decided to have a look at getting my hands on one. I’m not a gamer. so I don’t already have an XBOX 360, but since all the options were explained to me I now know exactly what I want. And what I want is an XBOX 360 250Gb HD with the Kinect sensor bar. I know I should be able to get that bundle for somewhere in the region of £300. But I’m looking for a deal. With that in mind I went looking for options. So I searched on Bing and Google.

They both return advertised links (PPC: Pay Per Click) as well as the regular (“organic”) results. So, I click on all most of them opening them in to new tabs. (Remember, I am looking for a deal, so I want to compare quickly what each of the offerings are).

Currys has a paid link with the tag line “Buy Xbox Kinect. We are in stock Reserve and Collect yours Now.” [sic] It sounds promising, doesn’t it? currys-kinect-page

So, I click the link and go looking for the price. Nope can’t see a price.

Some form of Add to Basket link, surely that’ll get me a price. Nope, can’t see that either.

Anything at all that looks remotely like some form or buy/purchase/reserve link. Anything at all! Nope. Not a thing.

I know what I want. I’m motivated to buy. All I want to know is that you’ve got it in stock and how much you want for it.

Well done Currys, you’ve wasted money on advertising a product that I cannot see how to actually buy. I got so irritated that I went to close down the tab in my browser. But… I didn’t do that. I got to thinking about how the follow up from the advert had not served its purpose. The advert hooked me in, but the website was so ineffectual that I was heading off elsewhere.

So, how do I actually buy it? There is no “buy this” call to action, so I really don’t know where to go from here. Any button I press is going to be a bit random and I have to think about what is likely to give me the best route to accomplishing my goal.

I really feel at this point that the website isn’t doing its job properly. Surely the purpose of this website is to get people to buy stuff? That’s how it makes money. That’s why Currys spend money on building the site and advertising its existence. It is so they can get people to come to them to buy stuff rather than go to a competitor to buy stuff.

Lets consider if this had been a situation where I had actually walked in to a Currys store. It would have been akin to me asking a sales assistant on the shop floor “Can you tell me the price of an XBox 360 with 250Gb drive and the Kinect Sensor bar?" and instead of answering my question they wax lyrical about what a great product it is.

I scroll down the page scanning any text for things that look like links or buttons. There are some pictures with “Find out more” links below each of them. Two of them actually have the sensor bar on it, one of which also has the console on it. I actually had to open both links up to figure that out because at scanning speed they look pretty similar. It is only when I’m analysing my actions do I really consciously take in what the difference is.

currys-kinect-findout-moreOnce I get to the correct page I’m presented with a grid of pretty similar looking pictures. At least this time there is a description below each of them and a price (Finally, I’m getting the information I actually wanted). However, my issues with this website are not over. Since there are several similar bundles which vary only slightly from each other by the type of console and by the packaged games. The graphics are too small to see what the difference is in the games and the consoles all look alike I need the text to tell me which is which. The descriptions pretty much all say “MICROSOFT Xbox 360 Came Console with…”, occasionally it will say something else such as “MICROSOFT Xbox 250Gb Bundle with…”

This is not giving me what I want. In fact, Currys are doing themselves a disservice as well. Some of the titles that just say “Xbox 360” without reference to the type of Xbox are actually the 250Gb version, so at a glance I would skip past them because I’ve also seen other descriptions that say “250Gb” so I am assuming it is a lower spec model that I’m not interested in.

Had I not been piqued with interest about the issues with this website I’d have left a long time ago. Instead, I took some time to understand what was actually going on and highlight them.

I’m guessing there was a meeting at some point to discuss the design of the website. At this meeting various aspects of the site were discussed. In the rush to get the site out of the door short cuts were taken. Certain things weren’t thought about properly.

The “Find out more” button actually takes you to a page where you can browse the products relating to the page you’ve just come from. Why not tell me that? I’d have been much more interested if the link had mentioned that I’d see prices, bundle options or what not. Yes, technically I am finding out more, but it didn’t really inspire me to find out more, which is more my point.

The product names in the page that allows you to browse the products are all clipped. I’m guessing that at some point a graphic designer put together the some visuals to show how the page should look. A web developer converts that into a working site. The visuals show two line product names but the developer sees that some product names are too long to match the visuals, so the product names get clipped and thus rendered (in situations were there are very similar product bundles) next to useless. Again, time is probably very tight. An unforeseen situation early in the project has now come to light. There is no time to redesign the visuals so the next best solution is taken. That’s to force the product names into the space provided.

DidI buy from Currys now I spent all that time analysing their site? No. Had an interest in usability not caused me to have a think about what was going on I would have left long before. In the end I bought my XBOX 360 Kinect with 250Gb HDD from GAME… in store! And, you know what? When I was looking in store I couldn’t see an XBOX 360 with 250Gb HD and as I  was searching a sales assistant asked me if she could help. I said I was looking for the version with the 250Gb HDD and she said that they didn’t have any left, however
if I bought the HDD as a separate item at the same time as the XBOX they would discount it so that it was the same total price as buying the model with the 250HDD included. Fantastic! Oh… and they knocked roughly 25% off each of the Kinect games we bought to get going with.

Misc

Tesco, Your car wash sucks

Earlier today I was at Tesco to refuel my car and I noticed that it was a bit overdue for a wash, so when I paid for my fuel I also purchased a voucher for the car wash. It was the premium super-duper all singing all dancing wash for £6.

When I drove round to where the car wash was there was a queue of three people in front of me so I had to wait. There was a chap at the jet wash too and I noticed that he was much slower. By the time my turn came around he was still there washing his car. In fact, by the time I wash finished he was still washing his car, he must have put much more money in that machine than I did for the car wash. In hindsight, I think he took the better decision. Why?

Share photos on twitter with TwitpicWhen I got home, I went to open the boot to retrieve my shopping and I noticed that the back of the car was still dirty. Sure, bits of it were clean, and all of it was still wet, but it was obvious that the bushes on the rollers don’t clean very well. Or maybe they don’t clean cars with near vertical rears (like the Toyota Yaris) very well. I could still wipe my finger through the dirt. And here’s a picture just to show you. (You can click the image to see it full size and you can see my finger mark in the remaining dirt)

All I can say is that I’ll not be back to Tesco to use their car wash again. If I do find myself there, I may just use the Jet Wash like the other chap did. That seemed the more sensible solution.

Misc

Technology Trends – October 2010 update

Around this time last year I wrote a blog post showing technology trends for browsers and operating systems as reported by Google Analytics for my blog and some mainstream websites.

Since then the world has moved on and the trends continue to evolve. New technology arrives, old technology slowly withers.

For the moment, it is interesting (to me, at least) to see what the trends are for my own blog.

Operating System

Windows XP is fading, not as fast as I’d hope, but still it is now below 50% of visits. On the other hand Windows 7 is rising fast and sitting at 38% in the space of just over a year. The biggest hit is for people moving away from Windows Vista, that’s where most of the market share is being lost from.

Other operating systems, such as MacOS and Linux don’t feature very heavily, bobbing around the 2% to 5% without much real movement. I’m not completely surprised by that, my blog is mostly aimed at .NET Development which is a Microsoft developer technology.

MyBlog-3yr-OS

Browser

While Operating System technology is being dominated by Microsoft they are losing share on the Browser front. While IE is still the more prevalent browser its share is being eroded, not by FireFox as I would have thought, but by Google’s offering, Chrome.

FireFox has stayed fairly steady over the last three years bobbing around the 30% to 40% mark. IE has gone from just shy of 70% to just over 40%. Meanwhile Chrome has risen from nothing to just under 20%.

Other browsers, such as Opera and Safari, don’t really make that much of an impact.

MyBlog-3yr-Browser

Misc

Why can't customer services actually be helpful?

I’ve noticed that when ever I have to get in touch with customer services for after sales advice then I get a really bad service. I tend to get stock answers that suggest the person who responded didn’t really read my inquiry. Often they will tell me how important I am, but that really is not the impression I get. I don’t want stock answers. I don’t want to be told how important I am when it is so obvious that it really isn’t true.

However, sometime I see examples that really make me think: Didn’t someone in management at least care enough to stop stuff like this happening.

The example I’m talking about is from Halfords. I was looking for a download of the instructions for my bike rack as I’d lost them a while ago. When I searched for them I found a forum on Halfords website attached to the product that where someone had asked that very question: “Would be it possible for you to send me a complete set of instructions for this bike rack. I have lost mine.”. Now rather than actually answer the question Halfords responded with “The Halfords Bike / Cycle carrier range are designed to be universal. The range of products will fit most cars (This carrier is not recommended for cars with spoilers) The Halfords Rear Mount 3 Cycle Carrier should fit your model of car. Halfords colleagues in any one of our stores will be able to assist you in selecting the best bike carrier for your needs and provide fitting if required

Halford's forum

Ummm… okay. That’s just paraphrasing the product description already.

Luckily, someone, another customer I presume, was at least more helpful and showed how to find the instructions on the website. More helpfully, they also included a direct link to the instructions to make it easy.

Why couldn’t Halfords have done this? Why did they think they could get away with fobbing off the customer with stock drivel?