case "CLS": should be lowercase.

A couple of issues:

• "CLS" should be lower case

• The "CLS" case should also return true

2 easy :)

+1:

CLS should be lowercase

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

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

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

Regards,

CLS should be "Cls"

not sure about would you should return there though?

I am guessing the readLine.ToLowerInvariant() should be

readLine.ToLower(),

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

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.

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.

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 ?? .

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

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.

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

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.

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.

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.

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

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.

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.

@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.

@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.

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

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.

@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.

@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.

@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.

@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.

@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.

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.