C#: What’s wrong with this code #5

[Update: I messed up some code. Corrected version now]

Greeting from the frigid city of Redmond. Keep in mind, of course, that in the Puget Sound region, “frigid” is any temperature below freezing.

Thanks for all the feedback on previous bad code examples. I’ve created a new category for these posts, so you can avoid my other posts if you wish.

This snippet is a routine that is used for web debugging.

enum ResponseType { ReturnValues, InvalidInput }

string CreateWebText(string userInput, ResponseType operation) {
   switch (operation) {
      case ResponseType.ReturnValues:
         userInput = “<h1>Values</h1>” + FilterOutBadStuff(userInput);
      case ResponseType.InvalidInput:
         userInput = “<h1>Invalid</h1>” + FilterOutBadStuff(userInput);
   return userInput;

What’s wrong with this code?

Hint: There are both obvious and subtle issues lurking…

Comments (36)

  1. Two things I can see off the bat. The first is that you have a parameter and a local variable named "userInput". That won’t compile.

    The second is that you don’t have a default statement in your switch, which goes along with the fact that you don’t check for values passed outside of the values in the ResponseType enumeration.

    Also, you could trim down the code a little, since it seems like you have repetitive code in your switch, but that’s not necessarily wrong, just sloppy.

  2. Obvious:

    No default in ResponseType, if someone adds a new value that switch will not filter out bad stuff.

    There’s a parameter of ReturnValues and a variable within function scope of the same name.

  3. Adam says:

    Here are the obvious ones:

    1 – you’re not initializing userInput, so this shouldn’t compile

    2 – you’re not handling the potential of an invalid ‘operation’ value (that is, either using Enum.IsDefined or having a default case)

  4. David Smith says:

    1 enum ResponseType { ReturnValues, InvalidInput }

    2 string CreateWebText(string userInput, ResponseType operation) {

    3 string userInput;

    4 switch (operation) {

    5 case ResponseType.ReturnValues:

    6 userInput = "<h1>Values</h1>" + FilterOutBadStuff(userInput);

    7 break;

    8 case ResponseType.InvalidInput:

    9 userInput = "<h1>Invalid</h1>" + FilterOutBadStuff(userInput);

    10 break;

    11 }

    12 return userInput;

    13 }

    I’ll wager a couple theories:


    1. On line 3, you can’t define a new variable ‘userInput’, because there’s already a variable defined with that name in this scope.

    Solution: Comment out line 3. Don’t define a new variable. We don’t need it. The variable has already been put on the stack (because of the function call) you can reference that variable without defining a new one.

    2. FilterOutBadStuff() isn’t defined? =D It is a snippit, but I’ll be a dork.

    What other issues are there?

  5. David Smith says:

    In response to Adam:

    Quote: " 1 – you’re not initializing userInput, so this shouldn’t compile "

    I don’t agree. It compiles just fine for me in VS2005.


    Console.WriteLine(p.CreateWebText(null, ResponseType.InvalidInput));

    output: <h1>Invalid</h1>

  6. Chris Brandsma says:

    First problem, rename the local variable userInput. You already have a parameter named userInput.

    I prefer this notation for local variables myself:

    string sUserInput = "";

    –notice I’m initilizing the variable. I make a habit of always initializing variables. It makes like easier in the long run.

    The other problem is error handling…there isn’t any. It looks like you are returning html directly to the browser from this routine. I know from experience that most of the time you don’t check to see if a string == null unless you are doing database code.

    So, I would wrap this routine in a try-catch statement so it returns something…instead of bombing out all of the other code called after this routine.

  7. Alex Yakhnin says:

    1. Since the method is returning the result it’d be a "bad" taste to modify the parameter userInput, So it’s better to declare a local variable with a different name instead.

    2. Both parameters could be null’s, so the check for null’s must be added to avoid exception.

  8. Well, this test case passes, so the code "Works"..


    public void TestEricGusStuff()



    string testInput = INPUT;

    string testWebText = CreateWebText(testInput, ResponseType.InvalidInput);

    Assert.IsTrue(testWebText == "<h1>Invalid</h1>" + INPUT, "Oops, doesn’t work!");

    Assert.IsTrue(testInput == INPUT, "Oops, reference changed…");


    I see 2 problems, no "Undefined" value for the enumeration, and no "Default:" condition for the switch?

  9. Jeremy Wiebe says:

    Another possible problem that I recall from Code Complete is that an input parameter is being modified by the method. Correct me if I’m wrong but that’s also a no-no.

    Maintainers of this code have to know that the parameter may not always be what it was at the beginning of the call. I think the caller of the CreateWebText() method may also be surprised that the first parameter it passed in has changed after the method call.

  10. Frederik says:

    What happens when you pass this:

    ResponseType.ReturnValues & ResponseType.InvalidInput


  11. Matt B. says:

    Initially, I thought the problem might be that your method is changing an input parameter, which – while legal – is, I feel, a bad practice. In fact, several of the comments posted here show why it is a bad practice: programmers don’t expect a method to actually change the data in one of its parameters, so much so that they assumed you had created a local variable of the same name as the parameter (since they wouldn’t think that you would actually manipulate the parameter itself). Since programmers don’t expect that, they can be quite surprised when data that they passed as an argument to a method has been altered after that method call. However, as the NUnit test code demonstrated later on, the data contained in the string variable ("testInput") remained the same after the method call. This is because method parameters are passed by value, and a string, being a value type, was therefore copied into the new "userInput" variable, leaving the original value untouched. This may in fact be one of the problems in the code, in that it may not do what the author of the code intended; if – despite bad programming practices – the author intended to change the value of the input parameter, then he/she would not have been successful.

    There is the issue of not dealing with the possibility that the "operation" parameter contains an invalid value. You didn’t explicitly deal with this situation in your switch statement, though it is "handled" by default, the default being that you leave the input untouched.

  12. Presumably all the comments about having a variable with the same name as the parameter were based on the previous, incorrect version of the code.

    The problem I see (and others have pointed out too) is that if operation is neither of the two allowable values (which is perfectly possible, enums aren’t constrained to their defined values, sadly) then FilterOutBadStuff never gets called and userInput gets returned unmodified. This of course means that (a) there’s no error message and (b) you’re vulnerable to whatever kind of attacks FilterOutBadStuff is designed to protect against.

    I can’t see any other issues though, and I’m not sure which if any of these are supposed to be the "obvious" vs "subtle" ones.

  13. Mark Mullin says:

    Well, here’s a fussy version

    enum ResponseType { UNKNOWN = 0,ReturnValues, InvalidInput }

    string CreateWebText(string userInput, ResponseType operation) {

    string result;

    string strongbad; // πŸ™‚

    switch (operation) {

    case ResponseType.ReturnValues:

    strongbad = FilterOutBadStuff(userInput);

    result = String.Format("<h1>Values</h1>{0}",strongbad != null ? strongbad : "");


    case ResponseType.InvalidInput:

    strongbad = FilterOutBadStuff(userInput);

    result = String.Format("<h1>Invalid</h1>{0}",strongbad != null ? strongbad : "");



    result = "";


    return result;


  14. Matt B, good point that if the programmer intended to change the input value it would not have been successful. It never occurred to me that the programmer might have wanted to do that, it’s an evil thing to want to do.

    Of course, if they *did* want to, the fix is to change the method signature to:

    string CreateWebText(ref string userInput, ResponseType operation) { … }

    And then call it with something like:

    string inp = myTextBox.Text;

    string result = CreateWebText(ref inp, operation);

    But why would you want to modify the input parameter and *also* return the same value?

  15. Sergio Pereira says:

    Like most people noted, you’re not validating the enum input or adding a default branch in the switch(). That could cause an interesting problem if the code is called like this:

    int invalidEnum = 9999;

    someLiteralControl.Text = CreateWebText("<script>alert(‘Gotcha!’);</script>", invalidEnum);

    Because the input string was never processed, it returns directly to the calling code, which apparently dumps the html returned by it.

  16. JD says:

    First off, you’re violating standard ASP.NET design principles by generating raw HTML that (it seems) will just be blasted to the page. This leaves open the problem that if FilterOutBadStuff() only decides to remove certain things but leaves, say, less-than characters in the string then you’ll end up with a very malformed HTML document.

    Don’t generate raw HTML. Use a Label and set its .Text property.

  17. Daniel says:

    In my eyes it there only one case, namely passing a value for ResponseType that is not explicitly defined in the enumeration. Then FilterOutBadStuff will not be called and really bad things may happen since the input is returned unfiltered.

    FilterOutBadStuff should be able to handle null values in my eyes …

    Jeremy: strings are immutable, so changing the value of userInput inside the function will create a new string an not change the parameter.

    Passing ResponseType.ReturnValues & ResponseType.InvalidInput will execute no case statement; but this shouldn’t be called since the enum does not have the [Flags] attribute. So you shouldn’t expect to be able t bitwise combine ResponseType values.

    Where’s the subtle issue? Validating input parameters – expecially from untrusted surces like in web apps – should be obvious in my eyes …

  18. Tim Farley says:

    The very premise that you can even write a function "FilterOutBadStuff()" is incorrect. No matter how carefully that function is written, it is possible that a new "cross site scripting style" vulnerability could come along that could slip through its fingers. Never echo back known bad input into a web page.

  19. Adam says:

    David Smith: the version of code I saw in fact did not compile, then Eric changed it. It used to have a "string userInput;" at the top of the function but it’s since been removed.

    I was wrong about why it wouldn’t compile (I said because it wasn’t initialized, but it’s really because of duplicate scope), but I was right that it wouldn’t compile.

    Other than that, I think everyone else has done a good job pointing things out. πŸ™‚

  20. David Smith says:

    Adam: I was looking at the same code you were. All I was pointing out was that it would compile despite the lack of initialization. (Not because of the redeclaration of a variable) We’re all on the same page. πŸ™‚

    I think we’ve milked this "What’s wrong with this code?" for all it’s worth.

    P.S. To Eric: If you’re going to post "What’s wrong with this code?"s and then actually change the original post, you may want to keep the changed code in, but just commented out. =)

  21. Adam says:

    Interesting, then that’s something new in VS2k5 because it didn’t compile on 2k3 (I get "a variable already in this scope…" error when I compile it).

  22. CN says:

    I’m amazed at the number of posters stating that changing userInput would change the passed in parameter. Maybe it’s related to the lack of pointers in C# and similar languages, where the border between a reference and a real copy of the data is more fuzzy.

    I know what lengths I’ve gone into to explain to programming classes (and my own peers before that) *why*

    void squareme(int x)


    x *= x;


    won’t do a thing except eating cycles to the calling code. That’s the whole point of a stack, right?

    BTW, those of you discussing &ing of enums probably mean |. The missing default statement is all too true, and I also agree that the FilterOutBadStuff function call could be factored out. The questioning of the whole design with returning HTML is of course sensible, as well.

    I guess all has already been mentioned by others and that I am just ranting about reading the post too late.

  23. CN says:

    I’m amazed at the number of posters stating that changing userInput would change the passed in parameter. Maybe it’s related to the lack of pointers in C# and similar languages, where the border between a reference and a real copy of the data is more fuzzy.

    I know what lengths I’ve gone into to explain to programming classes (and my own peers before that) *why*

    void squareme(int x)


    x *= x;


    won’t do a thing except eating cycles to the calling code. That’s the whole point of a stack, right?

    BTW, those of you discussing &ing of enums probably mean |. The missing default statement is all too true, and I also agree that the FilterOutBadStuff function call could be factored out. The questioning of the whole design with returning HTML is of course sensible, as well.

    I guess all has already been mentioned by others and that I am just ranting about reading the post too late.

  24. CN says:

    Damn connections that break while the HTTP request is sent so you don’t know if it got through or not, you update, but obviously it was taken from cache and then you repost and of course it’s already there…

  25. Darren Oakey says:

    What’s wrong?

    1) it’s ugly – brackets don’t belong on the end of lines πŸ™‚

    2) userInput is being changed. That’s BAD with a capital B. Despite CN’s assertion that this we are in a world without pointers – we aren’t – however in this case the pointer itself is being changed, so it won’t actually cause an error, but it’s just dangerous practice – also if we had some cool error checking that showed you the parameters, we wouldn’t be able to tell what happened, because we are hiding them. There are a million reasons not to reuse a variable and zero reasons to re-use it.

    3) just because you’ve defined the enum as two values is no reason why it should only take those two values, as anyone can quite happily cast an int into a responseType. This should have a default case that throws an error.

    4) assuming userinput is null is not a good idea – must check it!

    5) using switch is bad generally – using switch like this, and an enum to control program flow is just hideous – split it into two different functions

    6) even if written like this, it would be crappy, because the two cases are doing the same thing – should be bent around so there’s only one call to FilterOutBadStuff

    7> I’d probably want the user input in some sort of span, so we can css it – but I don’t know what filter out bad stuff does…

    = SO

    = what should it be?

    = ————————-


    public CreateReturnValueText( string userInput)


    Ensure.NotMissing( "userInput", userInput );

    return "<h1>Values</h1>" + Ensure.NotNull( FilterOutBadStuff( userInput ));



    public CreateInvalidInputText( string userInput)


    Ensure.NotMissing( "userInput", userInput );

    return "<h1>Invalid</h1>" + Ensure.NotNull(FilterOutBadStuff( userInput));


  26. Joseph Tosey says:

    Well, I for one don’t think this code is all that bad for hastily written **debugging** code – the point is to be short and sweet, and this code does fulfill that mission.

    I agree that a missing default statement is a smoking gun – if another enumerand or [Flags] is added in the future. If you add Flags, though, you’ve certainly broken non-debug code segments in the application and are compelled to conduct a global review of usage.

    I would not criticize a developer for modifying an input variable, so long as they don’t modify its intent. I consider "cleaning" input parameters as acceptable use – e.g., "userInput = FilterOutBadStuff(userInput);" to simplify subsequent logic within the method (especially if this does nice things like transform null into "(null)"). A similar example would be, for instance, "expiryTime = max(expiryTime, Now);". We’re still talking about expiryTime – no reason to create a new name. Another good use is rounding or clipping to get rid of overlength strings ("if (name.Length > MaxLength) name = name.Substring(0, MaxLength – 1);"). I don’t think this is living dangerously.

    Assuming FilterOutBadStuff does what it says and outputs something like "(null)" when userInput is null, I don’t think you gain anything adding checks here.

  27. Joseph Tosey says:

    Oops, I accidentally cut a line.

    Since in this case userInput is subversively transformed into errorOutput, you *do* need a new variable name. Not because its bad to modify input parameter values, but because it is very bad to modify their meaning.

  28. DrPizza says:

    You should be filtering in the good, not filtering out the bad (assuming the environment you’re using doesn’t have this ability already, which it may do).

  29. AT says:

    a) Always define None = 0 (aka Default, unassigned) default value for enum.

    This will allow you to track out usage of unassigned variables. By default all int are initialised to 0 – in your situation it will be ReturnValues πŸ™

    If this realy matter for your enum to be only legal values (for example if you do reflection to show dropdown boxes) – start with number 1:

    enum ResponseType {

    ReturnValues = 1,



    b) Probably this is not good example – but if "routine that is used for web debugging." – then instead of switch you were able to do

    string CreateWebText(string userInput, ResponseType operation) {

    return String.Format("<h1>{0}</h1>{1}",operation, FilterOutBadStuff(userInput));


    This way you do not need to maintain list of all enum values in multiple separate places and your debug routine will work correctly – not fail (as proposed by others to add "default:").

    As well you will see integer values for all incorrect method calls (very often it’s a valuable for diagnostic information).

    Hope this helps πŸ˜‰

  30. Darren Oakey says:

    There are no good reasons to re-use the same variable – and plenty of reasons not to. Here are some more:

    * a stack trace gives you the wrong information. A lot of people (like me) rely on a stack trace history to work out production bugs. This shows you the input parameters… unless of course, you were stupid enough to change them!

    * its harder to follow. When you are looking at a big procedure, it is not immediately obvious that the variable you are looking at is _not_ in fact what you got passed in. [of course, we don’t write big procs any more, but that’s beside the point :)]

    * a user of a debugger can get confused – again, if you put a break point in a proc you don’t understand, you look at the value and go "that’s not right", and go back to the function who called it… and after a while work out it’s sending the right value – the bug is in the first function after all, but above, where you didn’t notice at first..

    * it’s potentially less efficient.

    if you have the line string output=DoSomethingWith( input) – a dumb optimizer can still look at input and go "it’s not used again – I’ll just leave it in EAX – and see that output is just returned, so leave it in EAX… but if you had the line

    input = DoSomethingWith( input) – it goes "oh – it’s being referenced again – I have to have the value in [ebp-2]"

    * subtle bugs get missed… Imagine you have

    input = ExtractField( input, 1 ) near the top. Later, someone realises that you need something else from that input string. They are likely to just go whatever=ExtractField(input, 2) – and completely not notice that input isn’t what the comment at the top says it would be

    it’s just terrible practice – plain and simple.

    It’s not even easier to type – because at the end of the day – you can refactor out most temporary variables.

    How much cleaner is

    return "x" + Convert(y);

    compared to

    string blah = "x" + Convert(y);

    return blah;

  31. Jason says:

    1) It looks to me like the enumeration is describing a boolean condition, whether the input is valid or not.

    2) The case sections are mostly repeating the same code. The only thing that changes is the string value of the header.

    3) As many other posters noted, changing the meaning of the userInput is a bad idea. But not just because it’s an input variable. Once you’ve changed it to the HTML you want to display, the name "userInput" is no longer true.

    How about this:

    string CreateWebText(string userInput, bool IsValid) {

    string strTitle = "";

    if (IsValid) strTitle = "Values";

    else strTitle = "Invalid";

    string strWebText = String.Format("<h1>{0}</h1>{1}", strTitle, FilterOutBadStuff(userInput));



  32. Joseph Tosey says:

    I was reading a debug log this morning, and it occurred to me the really subtle problem is here.

    The purpose of a debug capability is to see the bad stuff, not to filter it out. You need an API to SafelyFormatBadStuff(), not FilterOutBadStuff().

  33. 友情链ζŽ₯: ε‰εˆ—θ…Ίη‚Žhttp://qlxsos.51.net ε‰εˆ—θ…Ίη‚Žhttp://ppp001.51.net ε‰εˆ—θ…Ίη‚Žhttp://www.qlxsos.com/

  34. andychu says:














































    http://www.zcfounder.com“>http://www.zcfounder.com google

    http://www.zcfounder.com“>http://www.zcfounder.com/promotion.htm google