Ayende @ Rahien

Hi!
My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:

ayende@ayende.com

+972 52-548-6969

, @ Q c

Posts: 18 | Comments: 79

filter by tags archive

Where is the bug?

time to read 1 min | 146 words

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

case "CLS": should be lowercase.

Oded

A couple of issues:

  • "CLS" should be lower case

  • The "CLS" case should also return true

Eleasar

+1:

CLS should be lowercase

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

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

Szymon Sasin

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

Regards,

dai

CLS should be "Cls"

not sure about would you should return there though?

dai
dai

I am guessing the readLine.ToLowerInvariant() should be

readLine.ToLower(),

Rob White

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

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

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
  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
Ori

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

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

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

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

Mikael,

Do you have any reference for that?

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

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

@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

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

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

@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

@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

"" 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

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

@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

@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

@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

@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

@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

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.

Comment preview

Comments have been closed on this topic.

FUTURE POSTS

  1. Production postmortem: The industry at large - 7 hours from now
  2. The insidious cost of allocations - about one day from now
  3. Buffer allocation strategies: A possible solution - 4 days from now
  4. Buffer allocation strategies: Explaining the solution - 5 days from now
  5. Buffer allocation strategies: Bad usage patterns - 6 days from now

And 2 more posts are pending...

There are posts all the way to Sep 11, 2015

RECENT SERIES

  1. Find the bug (5):
    20 Apr 2011 - Why do I get a Null Reference Exception?
  2. Production postmortem (10):
    01 Sep 2015 - The case of the lying configuration file
  3. What is new in RavenDB 3.5 (7):
    12 Aug 2015 - Monitoring support
  4. Career planning (6):
    24 Jul 2015 - The immortal choices aren't
View all series

Syndication

Main feed Feed Stats
Comments feed   Comments Feed Stats