Ayende @ Rahien

Refunds available at head office

Where is the bug?

Originally posted at 11/16/2010
var readLine = Console.ReadLine() ?? "";
switch (readLine.ToLowerInvariant())
{
    case "CLS":
        Console.Clear();
        break;
    case "reset":
        Console.Clear();
        return true;
    default:
        return false;
}

Comments

DoubleD
11/17/2010 10:06 AM by
DoubleD

case "CLS": should be lowercase.

Oded
11/17/2010 10:08 AM by
Oded

A couple of issues:

  • "CLS" should be lower case

  • The "CLS" case should also return true

Eleasar
11/17/2010 10:19 AM by
Eleasar

+1:

CLS should be lowercase

"" should be written as String.Empty ;-)

not sure about the "missing" (?) return in the "CLS" case

Szymon Sasin
11/17/2010 10:55 AM by
Szymon Sasin

All above pus you migth add a "one point of entry and one point of exit" rule :)

Regards,

dai
11/17/2010 11:02 AM by
dai

CLS should be "Cls"

not sure about would you should return there though?

dai
11/17/2010 11:08 AM by
dai

I am guessing the readLine.ToLowerInvariant() should be

readLine.ToLower(),

Rob White
11/17/2010 11:39 AM by
Rob White

You could argue the biggest bug is the use of a switch statement as it violates OCP.

ashic
11/17/2010 11:43 AM by
ashic
  1. Console.ReadLine() returns a string. Why the check for null?

  2. The CLS case will not ever be triggered as you're switching on the lowered string.

  3. Two conditions return a bool while one breaks to the following code. Seems like an area where potential problems might occur in the future.

Dominic Pettifer
11/17/2010 11:54 AM by
Dominic Pettifer

Surely this code won't even compile since case "CLS": doesn't return a boolean (just breaks), unless there's a return boolean statement after the switch code.

Matthew Wills
11/17/2010 12:02 PM by
Matthew Wills
  1. Console.ReadLine() returns a string. Why the check for null?

Typing in Control-Z then pressing Enter will return null. Hence the need for ?? .

Ori
11/17/2010 12:02 PM by
Ori

the bug almost always is between the chair and the pc :)

Matthew Wills
11/17/2010 12:09 PM by
Matthew Wills

There appears to be a number of bugs:

a) CLS should be lower case.

b) No return statement in the CLS case (although this might be OK - there may be a return later in the method that isn't shown).

c) Use of Console.Clear() will throw IOException if output is redirected to a file.

Patrik Hägne
11/17/2010 12:59 PM by
Patrik Hägne

The use of a switch statement is a bug in itself.

Mikael Henriksson
11/17/2010 01:43 PM by
Mikael Henriksson

What they said.... It's also worth mentioning that the .NET framework has been optimized for ToUpper() which will perform better so that should be used instead.

Ayende Rahien
11/17/2010 01:44 PM by
Ayende Rahien

Mikael,

Do you have any reference for that?

Jesse Williamson
11/17/2010 01:46 PM by
Jesse Williamson

I'll go with:

1) CLS should be lower case

2) The CLS case doesn't return

3) Use of ?? on a string doesn't account for an empty string from the console.*

*I didn't see anyone else bring that up so I have a feeling I'm missing something, but I thought I'd put it out there anyway.

Mikael Henriksson
11/17/2010 01:49 PM by
Mikael Henriksson

Sorry Ayende, I replied to your email :)

Taken from msdn.microsoft.com/en-us/library/ms973919.aspx

-"DO: Use ToUpperInvariant rather than ToLowerInvariant when normalizing strings for comparison."

there is a bunch of information there but it might be outdated. I ran some tests a few years ago and there was a difference though I can't remember the exact numbers any more.

Brian Mullen
11/17/2010 05:52 PM by
Brian Mullen

@Mikael

The latest regarding this is at:

http://msdn.microsoft.com/en-us/library/dd465121(v=VS.100).aspx

but it says much the same of the original article:

"Use the String.ToUpperInvariant method instead of the String.ToLowerInvariant method when you normalize strings for comparison."

Hugo
11/17/2010 06:03 PM by
Hugo

Same comments as above (upper case "CLS", use string.Empty), but since the purpose, and thus the logic is the same for both "cls" and "reset", the switch statement could read:

switch(...)

{

case "cls":

case "reset":

    Console.Clear();

    return true;

default:

...

}

or if all that is being verified in the method is a clear command, the check could be encapsulated, with a switch on true/false (or if in this case), which would allow for easily adding more aliases to the command, such as "clear" for the *nix people.

Nick Aceves
11/17/2010 09:39 PM by
Nick Aceves

Really people? How is the use of "" instead of String.Empty a bug in any sense? (Because, you know, those extra string objects are going to just KILL your performance.)

I'll prefer "" any day of the week.

Moti
11/18/2010 07:46 AM by
Moti

@Nick

You are right it is not a bug, it is just a bad habit.

When using "" instead of string.Empty, you might accidently put space inside and cause yourself a whole lot of misery when trying to find the bug.

marc
11/18/2010 01:44 PM by
marc

@Szymon Sasin "All above pus you migth add a "one point of entry and one point of exit" rule :)"

I prefer "early exit principle" myself. Especially for methods. Easier to read.

Ryan Roberts
11/18/2010 09:09 PM by
Ryan Roberts

"" is sightly is a bad idea if you are building some kind of crazy 0 GC churn application. Personally I think "" is easier on the eye. It's definitely not a bug

developmentalmadness
11/19/2010 10:54 PM by
developmentalmadness

Switch is a bug? Unilaterally? Would you prefer

if(readline.ToLowerInvariant() == "cls"){

} else if(readline.ToLowerInvariant() == "reset"){

} else {

}

Now you're calling ToLowerInvariant() twice. That's if you only have two cases. The more cases you have the worse it is because you have to check each "if" separately. Whereas switch just puts the values on the stack once and then goes to the correct label. The more cases, the more benefit.

Anyway, if you're checking a single variable against a list of possible results how is that a bad thing? If you have a short list that changes rarely then it is hardly worth the extra effort and overhead to implement support for a dictionary when a simple bit of control logic would suffice.

Nick Aceves
11/19/2010 11:29 PM by
Nick Aceves

@Moti

If "you might mistype it" is your rationale for why it's a bad practice, then I suggest you either:

1) learn how to type

2) learn how to write unit tests

3) use a different font or increase the size so you can see spaces more clearly

In all my years of programming, distinguising "" from " " has never been a problem.

Luke Schafer
11/21/2010 10:58 PM by
Luke Schafer

@Nick, thanks for taking up the flag on that one :) +1

@developmentalmadness, I'm sure other people have other reasons for not liking switches, but mine is this: case values may only be constant values, hence for all but trivial decisions you often end up with additional conditional login within one or more cases. This not only increases the cyclomatic complexity, but reduces readability. 'If' statements allow you do combine many logical statements, and even the result of function calls, so are superior in almost every way.

Patrik Hägne
11/23/2010 10:50 AM by
Patrik Hägne

@developmentalmadness No, of course I would not prefer that code, string comparisons should not be made using the "==" operator but string.Equals("foo", "bar", StringComparison.OrdinalIgnoreCase).

I do however prefer multiple if statements to the switch in the case there are no more than say three branches, if there are more you should probably refactor to either use a lookup or some variation of the state pattern.

Patrik Hägne
11/23/2010 10:53 AM by
Patrik Hägne

@developmentalmadness In addition to that, even if you would want to use the equals operator you could easily change the code that sets the readline variable to make the ToLowerInvariant once only.

Moti
11/30/2010 05:05 PM by
Moti

@Nick,

I didn't say it was bad practice, just a bad habit, so you can cut off the "boo hoo, someone told me i am doing something bad routine".

As for the issue itself, you should avoid using "", for the same reason you try (i hope) to avoid using magic strings.

Phil
11/30/2010 05:17 PM by
Phil

I suppose it depends how you define a 'bug', the code will work every time, it just won't work in the way the creater intended it to.

Comments have been closed on this topic.