다음을 통해 공유

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);
break;
case ResponseType.InvalidInput:
userInput = "<h1>Invalid</h1>" + FilterOutBadStuff(userInput);
break;
}
return userInput;
}

What's wrong with this code?

Hint: There are both obvious and subtle issues lurking...

Comments

  • Anonymous
    January 04, 2005
    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.
  • Anonymous
    January 04, 2005
    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.
  • Anonymous
    January 04, 2005
    The comment has been removed
  • Anonymous
    January 04, 2005
    The comment has been removed
  • Anonymous
    January 04, 2005
    The comment has been removed
  • Anonymous
    January 04, 2005
    The comment has been removed
  • Anonymous
    January 04, 2005
  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.