Finding Bad Controllers

In one mailing list I’m on, someone ran into a problem where they renamed a controller, but ASP.NET MVC could not for the life of it find it. They double checked everything. But ASP.NET MVC simply reported a 404.

This is usually where I tell folks to run the following NuGet command:

Install-Package RouteDebugger

RouteDebugger is a great way to find out why a route isn’t matching a controller.

In this particular case, the culprit was that the person renamed the controller and forgot to append the “Controller” suffix. So from something like HomeController to Default.

In this post, I’ll talk a little bit about what makes a controller a controller and a potential solution so this mistake is caught automatically.

What’s with the convention anyways?

A question that often comes up is why require a name based convention for controllers in the first place? After all, controllers also must implement the IController interface and that should be enough to indicate the class is a controller.

Great question! That decision was made before I joined Microsoft and I never bothered to ask why. What I did do was make up my own retroactive reasoning, which is as follows.

Suppose we simply took the class name to be the controller name. Also suppose you’re building a page to display a category and you want the URL to be something like /category/unicorns. So you write a controller named Category. Chances are, you also have an entity class named Category too that you want to use within the Category controller. So now, a common default situation becomes painful.

If I could get in a time machine and revisit this decision, would I? Hell no! As my former co-worker Eilon always says; if you have a time machine, there’s probably a lot better things you can do than fix bad design decisions in ASP.NET MVC.

But if I were to do this again, I’m not so sure I’d require the “Controller” suffix. Instead, I’d suggest using plural controllers (and URLs) to better avoid conflicts. So the controller would be Categories and the URL would be /categories/unicorns. And perhaps I’d make the “Controller” suffix allowed as a way to resolve conflicts. So CategoryController would still work fine (as a conroller named “category”) if that’s the way you roll.

How do I detect Controller Problems?

Since I didn’t use my time machine to fix this issue (I would rather use it to go back in time and fix line endings text encodings), what can we do about it?

The simplest solution is to do nothing. How often will you make this mistake?

Then again, when you do, it’s kind of maddening because there’s no error message. The controller just isn’t there. Also, there’s other mistakes you could make, though many are unlikley. All the following look like they were intended to be controllers, but aren’t.

public class Category : Controller
{}

public abstract class CategoryContoller : Controller
{}

public class Foo 
{
    public class CategoryController : Controller 
    { }
}

class BarController : Controller
{}

internal class BarController : Controller
{}

public class BuzzController
{}

Controllers in ASP.NET MVC must be public, non-abstract, non-nested, and implement IController, or derive from a class that does.

So the list of possible mistakes you might make are:

• Forget to add the “Controller” suffix (happens more than you think)
• Make the class abstract (probably not likely)
• Nest the class (could happen by accident like you thought you were pasting in a namespace, but very very unlikely)
• Forget to make the class public (not likely if you use the Add Controller dialog. But if you use the Add Class dialog, could happen)
• Forget to derive from Controller or ControllerBase or implement IController. (Again, probably not very likely)

How to detect these cases

As I mentioned before, it might not be that important to do this, but one thing you could consider is writing a unit test that detects these various conditions. Well how would you do that?

You’re in luck. Whether this is super useful or not, I still found this to be an interesting problem to solve. I wrote a ControllerValidator class with some methods for finding all controllers that match one of these conditions.

It builds on the extension method to retrieve all types I blogged about the other day. First, I wrote extension methods for the various controller conditions:

static bool IsPublicClass(this Type type)
{
    return (type != null && type.IsPublic && type.IsClass && !type.IsAbstract);
}

static bool IsControllerType(this Type t)
{
    return typeof (IController).IsAssignableFrom(t);
}

static bool MeetsConvention(this Type t)
{
    return t.Name.EndsWith("Controller", StringComparison.OrdinalIgnoreCase);
}

With these methods, it became a simple case of writing methods that checked for two out of these three conditions.

For example, to get all the controllers that don’t have the “Controller” suffix:

public static IEnumerable<Type> GetUnconventionalControllers
  (this Assembly assembly)
{
  return from t in assembly.GetLoadableTypes()
      where t.IsPublicClass() && t.IsControllerType() && !t.MeetsConvention()
      select t;
}

With these methods, it’s a simple matter to write automated tests that look for these mistakes.

Source code and Demo

I added these methods as part of the ControllerInspector library which is available on NuGet. You can also grab the source code from my CodeHaacks repository on GitHub (click the Clone in Windows button!).

If you get the source code, check out the following projects:

• ControllerInspectorTests.csproj – Unit tests of these new methods show you how you might write your own unit tests.
• MvcHaack.ControllerInspector.csproj – Contains the ControllerValidator class.
• MvcHaack.ControllerInspector.DemoWeb.csproj – Has a website that demonstrates this class too.

The demo website’s homepage uses these methods to show a list of bad controllers.

The code I wrote is based on looking at how ASP.NET MVC locates and determines controllers. It turns out, because of the performance optimizations, it takes a bit of digging to find the right code.

If you’re interested in looking at the source, check out TypeCacheUtil.cs on CodePlex. It’s really nice that ASP.NET MVC is not only open source, but also accepts contributions. I highly recommend digging through the source as there’s a lot of interesting useful code in there, especially around reflection.

If you don’t find this useful, I hope you at least found it illuminating.

Get All Types in an Assembly

Sometimes, you need to scan all the types in an assembly for a certain reason. For example, ASP.NET MVC does this to look for potential controllers.

One naïve implementation is to simply call Assembly.GetTypes() and hope for the best. But there’s a problem with this. As Suzanne Cook points out,

If a type can't be loaded for some reason during a call to Module.GetTypes(), ReflectionTypeLoadException will be thrown. Assembly.GetTypes() also throws this because it calls Module.GetTypes().

In other words, if any type can’t be loaded, the entire method call blows up and you get zilch.

There’s multiple reason why a type can’t be loaded. Here’s one example:

public class Foo : Bar // Bar defined in another unavailable assembly
{
}

The class Foo derives from a class Bar, but Bar is defined in another assembly. Here’s a non-exhaustive list of reasons why loading Foo might fail:

• The assembly containing Bar does not exist on disk.
• The current user does not have permission to load the assembly containing Bar.
• The assembly containing Bar is corrupted and not a valid assembly.

Once again, for more details check out Suzanne’s blog post on Debugging Assembly Loading Failures.

Solution

As you might expect, being able to get a list of types, even if you don’t plan on instantiating instances of them, is a common and important task. Fortunately, the ReflectionTypeLoadException thrown when a type can’t be loaded contains all the information you need. Here’s an example of ASP.NET MVC taking advantage of this within the internal TypeCacheUtil class (there’s a lot of other great code nuggets if you look around the source code)

Type[] typesInAsm;
try
{
    typesInAsm = assembly.GetTypes();
}
catch (ReflectionTypeLoadException ex)
{
    typesInAsm = ex.Types;
}

This would be more useful as a generic extension method. Well the estimable Jon Skeet has you covered in this StackOverflow answer (slightly edited to add in parameter validation):

public static IEnumerable<Type> GetLoadableTypes(this Assembly assembly)
{
    if (assembly == null) throw new ArgumentNullException("assembly");
    try
    {
        return assembly.GetTypes();
    }
    catch (ReflectionTypeLoadException e)
    {
        return e.Types.Where(t => t != null);
    }
}

I’ve found this code to be extremely useful many times.

Sitting is Making You Fat and Killing You

As a kid, I was an impatient little brat. On any occasion that required waiting, I became Squirmy Wormy until I pushed my dad to make the demand parents so often make of fidgety kids, “Sit still!”

Recent evidence suggests a rejoinder to kids today in response to this command, “What!? Are you trying to kill me?!”

There is compelling evidence that modern workers propensity to sit for prolonged periods every day makes them fat and shortens their lives. Hmmm, you wouldn’t happen to know any professions where sitting limply at a desk for long periods of time is common, would you?

Yeah, me too.

This spurred me to learn more which led me to The Daily Infographic’s great summary of this research. Seriously, click on the image below. I could have stopped there and called it a post. But as always, I don’t know when to stop.

Much has been written about the detrimental health effects of inactivity. According to Marc Hamilton, a biomedical researcher, sitting down shuts off your fat burning.

Physiologists analyzing obesity, heart disease, and diabetes found that the act of sitting shuts down the circulation of a fat-absorbing enzyme called lipase.

The same Hamilton goes into more details in this interesting NY Times article on the potential lethality of prolonged sitting,

This is your body on chairs: Electrical activity in the muscles drops — “the muscles go as silent as those of a dead horse,” Hamilton says — leading to a cascade of harmful metabolic effects. Your calorie-burning rate immediately plunges to about one per minute, a third of what it would be if you got up and walked.

In other words, sitting down is the off button.

This LifeHacker article points out that the the long term health effects of sitting multiple hours a day go way beyond weight gain.

After 10-20 Years of Sitting More Than Six Hours a Day

Sitting for over six hours a day for a decade or two can cut away about seven quality adjusted life years (the kind you want). It increases your risk of dying of heart disease by 64 percent and your overall risk of prostate or breast cancer increases 30 percent.

I want all kinds of life years, but the “quality adjusted” variety sounds extra special.

You might think that you’ll be just fine because you exercise the recommended 30 minutes a day, but a study from the British Journal of Sports Medicine notes that’s not the case.

Even if people meet the current recommendation of 30 minutes of physical activity on most days each week, there may be significant adverse metabolic and health effects from prolonged sitting—the activity that dominates most people’s remaining “non-exercise” waking hours.

That’s particularly disheartening. All that other exercise you do might not counteract all the prolonged sitting.

Get up, stand up! Stand up for your code!

With apologies to Bob Marley

So what’s a developer to do? Note that these studies put an emphasis on prolonged. The simple solution is to stop sitting for prolonged periods at a time. Get up at least once an hour and move!

But developers are interesting creatures. We easily get in the zone on a problem and focus so deeply that three hours pass in a blink. Ironically this wasn’t a problem I faced as a Program Manager since I was moving from meeting to meeting nearly every hour.

But in my new job, writing code at home, I knew I needed more than an egg timer to tell me to move every hour. I want to move constantly if I can. For example, the way you do when you stand. So I looked into adjustable desks.

According to Alan Hedge, director of Cornell’s Human Factors and Ergonomics laboratory, workers fare better when using adjustable tables (emphasis mine).

We found that the computer workers who had access to the adjustable work surfaces also reported significantly less musculoskeletal upper-body discomfort, lower afternoon discomfort scores and significantly more productivity," said Alan Hedge, professor of design and environmental analysis in the College of Human Ecology at Cornell and director of Cornell's Human Factors and Ergonomics Laboratory.

So I went on a quest to find the perfect adjustable desk. How did I choose which desk to purchase?

Critical hit! Photo by disoculated from Flickr Ok, not quite. I might have put in a little more research into it than that.

I asked around the interwebs and received a lot of feedback on various options. I found two desk companies that stood out: Ergotron and GeekDesk.

Initially, I really liked the Ergotron approach. Rather than a motorized system for moving the desk up and down, it has a clever quick release lever system that makes it easy to adjust the desk’s height quickly without requiring any tools or electricity.

For this reason, I initially settled on the Workfit-D Sit Stand Desk. Unfortunately, Ergotron is a victim of its own success in this particular case. They were backordered until our sun grows into a red giant and engulfs the planet and I couldn’t wait that long.

So I ended up ordering the GeekDesk Max. This desk uses a motor to adjust to specific heights, but has four presets. This is important because without the presets, you’re sitting there holding the button until it reaches the height you want. While the motor is slower than the Ergotron approach, with the presets, I can just hit the button and go get a coffee. To be fair, it’s not all that slow. Did I mention I’m impatient?

I’m very happy with this desk. Here’s a photo of my workspace that I sent to CoderWall with the desk in a standing configuration.

As far as I can find, there’s only one study that points to a potential negative health impact from standing at work. http://www.ncbi.nlm.nih.gov/pubmed/10901115

Significant relationships were found between the amount of standing at work and atherosclerotic progression.

However, as you might expect, this one study is not conclusive and doesn’t focus solely on the work habits of office workers who stand. From what I can tell so far, the health benefits far outweigh the detriments assuming you don’t over do it.

If you do stand at work, I highly recommend getting some sort of gel or foam padding to stand on. Especially if you’re not wearing shoes. The hard floor might seem fine at first because you’re a tough guy or girl, but over the course of a day, it’ll feel like someone’s taken a bat to your soles.

Also, vary it up throughout the day. Don’t stand all day. Take breaks where you work sitting down and alternate.

Fight malaise!

Not every developer is the same, clearly. Some are fit, but many, well, let’s just say that the health benefits mentioned in this post might not factor into their decision making.

James Levine, a researcher at the Mayo clinic, had a more philosophical point to make about sitting all day that goes beyond just the physical health benefits. He also sees a mental health benefit.

For all of the hard science against sitting, he admits that his campaign against what he calls “the chair-based lifestyle” is not limited to simply a quest for better physical health. His is a war against inertia itself, which he believes sickens more than just our body. “Go into cubeland in a tightly controlled corporate environment and you immediately sense that there is a malaise about being tied behind a computer screen seated all day,” he said. “The soul of the nation is sapped, and now it’s time for the soul of the nation to rise.”

http://www.nytimes.com/2011/04/17/magazine/mag-17sitting-t.html

In other words, stop sitting and write better code! Go forth and conquer.

The Turkish İ Problem and Why You Should Care

Take a look at the following code.

 
const string input = "interesting";
        
bool comparison = input.ToUpper() == "INTERESTING";

Console.WriteLine("These things are equal: " + comparison);
Console.ReadLine();

Let’s imagine that input is actually user input or some value we get from an API. That’s going to print out These things are equal: True right? Right?!

Well not if you live in Turkey. Or more accurately, not if the current culture of your operating system is tr-TR (which is likely if you live in Turkey).

To prove this to ourselves, let’s force this application to run using the Turkish locale. Here’s the full source code for a console application that does this.

using System;
using System.Globalization;
using System.Threading;
internal class Program
{
    private static void Main(string[] args)
    {      
        Thread.CurrentThread.CurrentCulture = new CultureInfo("tr-TR");
        const string input = "interesting";
        
        bool comparison = input.ToUpper() == "INTERESTING";

        Console.WriteLine("These things are equal: " + comparison);
        Console.ReadLine();
    }
}

Now we’re seeing this print out These things are equal: False.

To understand why this is the case, I recommend reading much more detailed treatments of this topic:

• Internationalization for Turkish: Dotted and Dotless Letter “I”
• What’s Wrong With Turkey

The tl;dr summary summary is that the uppercase for i in English is I (note the lack of a dot) but in Turkish it’s dotted, İ. So while we have two i’s (upper and lower), they have four.

My app is English only. AMURRICA!

Even if you have no plans to translate your application into other languages, your application can be affected by this. After all, the sample I posted is English only.

Perhaps there aren’t going to be that many Turkish folks using your app, but why subject the ones that do to easily preventable bugs? If you don’t pay attention to this, it’s very easy to end up with a costly security bug as a result.

The solution is simple. In most cases, when you compare strings, you want to compare them using StringComparison.Ordinal or StringComparison.OrdinalIgnoreCase. It just turns out there are so many ways to compare strings. It’s not just String.Equals.

Code Analysis to the rescue

I’ve always been a fan of FxCop. At times it can seem to be a nagging nanny constantly warning you about crap you don’t care about. But hidden among all those warnings are some important rules that can prevent some of these stupid bugs.

If you have the good fortune to start a project from scratch in Visual Studio 2010 or later, I highly recommend enabling Code Analysis (FxCop has been integrated into Visual Studio and is now called Code Analysis). My recommendation is to pick a set of rules you care about and make sure that the build breaks if any of the rules are broken. Don’t turn them on as warnings because warnings are pointless noise. If it’s not important enough to break the build, it’s not important enough to add it.

Of course, many of us are dealing with existing code bases that haven’t enforced these rules from the start. Adding in code analysis after the fact is a daunting task. Here’s an approach I took recently that helped me retain my sanity. At least what’s left of it.

First, I manually created a file with the following contents:

<?xml version="1.0" encoding="utf-8"?>
<RuleSet Name="PickAName" Description="Important Rules" ToolsVersion="10.0">
  <Rules AnalyzerId="Microsoft.Analyzers.ManagedCodeAnalysis"
      RuleNamespace="Microsoft.Rules.Managed">

    <Rule Id="CA1309" Action="Error" />    
  
  </Rules>
</RuleSet>

You could create one per project, but I decided to create one for my solution. It’s just a pain to maintain multiple rule sets. I named this file SolutionName.ruleset and put it in the root of my solution (the name doesn’t matter. Just make the extension .ruleset)

I then configured each project that I cared about in my solution (I ignored the unit test project) to enable code analysis using this ruleset file. Just go to the project properties and select the Code Analysis tab.

I changed the selected Configuration to “All Configurations”. I also checked the “Enable Code Analysis…” checkbox. I then clicked “Open” and selected my ruleset file.

At this point, every time I build, Code Analysis will only run the one rule, CA1309, when I build. This way, adding more rules becomes manageable. Every time I fixed a warning, I’d add that warning to this file one at a time. I went through the following lists looking for important rules.

• Design Warnings
• Globalization Warnings
• Interoperability Warnings
• Maintainability Warnings
• Mobility Warnings
• Naming Warnings
• Performance Warnings
• Portability Warnings
• Reliability Warnings
• Security Warnings
• Usage Warnings

I didn’t add every rule from each of these lists, only the ones I thought were important.

At some point, I reached the point where I was including a large number of rules and it made sense for me to invert the list so rather than listing all the rules I want to include, I only listed the ones I wanted to exclude.

<?xml version="1.0" encoding="utf-8"?>
<RuleSet Name="PickAName" Description="Important Rules" ToolsVersion="10.0">
  <IncludeAll Action="Error" />
  <Rules AnalyzerId="Microsoft.Analyzers.ManagedCodeAnalysis"
      RuleNamespace="Microsoft.Rules.Managed">

    <Rule Id="CA1704" Action="None" />    
  
  </Rules>
</RuleSet>

Notice the IncludeAll element now makes every code analysis warning into an error, but then I turn CA1704 off in the list.

Note that you don’t have to edit this file by hand. If you open the ruleset in Visual Studio it’ll provide a GUI editor. I prefer to simply edit the file.

One other thing I did was for really important rules where there were too many issues to fix in a timely manner, I would simply use Visual Studio to suppress all of them and commit that. At least that ensured that no new violations of the rule would be committed. That allowed me to fix the existing ones at my leisure.

I’ve found this approach makes using code analysis way more useful and less painful than simply turning on every rule and hoping for the best. Hope you find this helpful as well. May you never ship a bug with the Turkish I problem again!