http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660I 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. http://ayende.com/4693/where-is-the-bug#comment31http://ayende.com/4693/where-is-the-bug#comment31Tue, 30 Nov 2010 17:17:51 GMT@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. http://ayende.com/4693/where-is-the-bug#comment30http://ayende.com/4693/where-is-the-bug#comment30Tue, 30 Nov 2010 17:05:38 GMT@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. http://ayende.com/4693/where-is-the-bug#comment29http://ayende.com/4693/where-is-the-bug#comment29Tue, 23 Nov 2010 10:53:21 GMT@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. http://ayende.com/4693/where-is-the-bug#comment28http://ayende.com/4693/where-is-the-bug#comment28Tue, 23 Nov 2010 10:50:22 GMT@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. http://ayende.com/4693/where-is-the-bug#comment27http://ayende.com/4693/where-is-the-bug#comment27Sun, 21 Nov 2010 22:58:02 GMT@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. http://ayende.com/4693/where-is-the-bug#comment26http://ayende.com/4693/where-is-the-bug#comment26Fri, 19 Nov 2010 23:29:37 GMTSwitch 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. http://ayende.com/4693/where-is-the-bug#comment25http://ayende.com/4693/where-is-the-bug#comment25Fri, 19 Nov 2010 22:54:09 GMT"" 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 http://ayende.com/4693/where-is-the-bug#comment24http://ayende.com/4693/where-is-the-bug#comment24Thu, 18 Nov 2010 21:09:34 GMT@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. http://ayende.com/4693/where-is-the-bug#comment23http://ayende.com/4693/where-is-the-bug#comment23Thu, 18 Nov 2010 13:44:44 GMT@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. http://ayende.com/4693/where-is-the-bug#comment22http://ayende.com/4693/where-is-the-bug#comment22Thu, 18 Nov 2010 07:46:23 GMTReally 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. http://ayende.com/4693/where-is-the-bug#comment21http://ayende.com/4693/where-is-the-bug#comment21Wed, 17 Nov 2010 21:39:56 GMTSame 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. http://ayende.com/4693/where-is-the-bug#comment20http://ayende.com/4693/where-is-the-bug#comment20Wed, 17 Nov 2010 18:03:10 GMT@Mikael The latest regarding this is at: [http://msdn.microsoft.com/en-us/library/dd465121](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." http://ayende.com/4693/where-is-the-bug#comment19http://ayende.com/4693/where-is-the-bug#comment19Wed, 17 Nov 2010 17:52:00 GMTSorry Ayende, I replied to your email :) Taken from [msdn.microsoft.com/en-us/library/ms973919.aspx](http://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. http://ayende.com/4693/where-is-the-bug#comment18http://ayende.com/4693/where-is-the-bug#comment18Wed, 17 Nov 2010 13:49:51 GMTI'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. http://ayende.com/4693/where-is-the-bug#comment17http://ayende.com/4693/where-is-the-bug#comment17Wed, 17 Nov 2010 13:46:03 GMTMikael, Do you have any reference for that? http://ayende.com/4693/where-is-the-bug#comment16http://ayende.com/4693/where-is-the-bug#comment16Wed, 17 Nov 2010 13:44:08 GMTWhat 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. http://ayende.com/4693/where-is-the-bug#comment15http://ayende.com/4693/where-is-the-bug#comment15Wed, 17 Nov 2010 13:43:23 GMTThe use of a switch statement is a bug in itself. http://ayende.com/4693/where-is-the-bug#comment14http://ayende.com/4693/where-is-the-bug#comment14Wed, 17 Nov 2010 12:59:32 GMTThere 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. http://ayende.com/4693/where-is-the-bug#comment13http://ayende.com/4693/where-is-the-bug#comment13Wed, 17 Nov 2010 12:09:27 GMTthe bug almost always is between the chair and the pc :) http://ayende.com/4693/where-is-the-bug#comment12http://ayende.com/4693/where-is-the-bug#comment12Wed, 17 Nov 2010 12:02:28 GMT>> 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 ?? . http://ayende.com/4693/where-is-the-bug#comment11http://ayende.com/4693/where-is-the-bug#comment11Wed, 17 Nov 2010 12:02:12 GMTSurely 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. http://ayende.com/4693/where-is-the-bug#comment10http://ayende.com/4693/where-is-the-bug#comment10Wed, 17 Nov 2010 11:54:36 GMT1. 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. http://ayende.com/4693/where-is-the-bug#comment9http://ayende.com/4693/where-is-the-bug#comment9Wed, 17 Nov 2010 11:43:07 GMTYou could argue the biggest bug is the use of a switch statement as it violates OCP. http://ayende.com/4693/where-is-the-bug#comment8http://ayende.com/4693/where-is-the-bug#comment8Wed, 17 Nov 2010 11:39:37 GMTI am guessing the readLine.ToLowerInvariant() should be readLine.ToLower(), http://ayende.com/4693/where-is-the-bug#comment7http://ayende.com/4693/where-is-the-bug#comment7Wed, 17 Nov 2010 11:08:09 GMTCLS should be "Cls" not sure about would you should return there though? http://ayende.com/4693/where-is-the-bug#comment6http://ayende.com/4693/where-is-the-bug#comment6Wed, 17 Nov 2010 11:02:32 GMTAll above pus you migth add a "one point of entry and one point of exit" rule :) Regards, http://ayende.com/4693/where-is-the-bug#comment5http://ayende.com/4693/where-is-the-bug#comment5Wed, 17 Nov 2010 10:55:24 GMT+1: CLS should be lowercase "" should be written as String.Empty ;-) not sure about the "missing" (?) return in the "CLS" case http://ayende.com/4693/where-is-the-bug#comment4http://ayende.com/4693/where-is-the-bug#comment4Wed, 17 Nov 2010 10:19:51 GMT2 easy :) http://ayende.com/4693/where-is-the-bug#comment3http://ayende.com/4693/where-is-the-bug#comment3Wed, 17 Nov 2010 10:15:02 GMTA couple of issues: - "CLS" should be lower case - The "CLS" case should also return true http://ayende.com/4693/where-is-the-bug#comment2http://ayende.com/4693/where-is-the-bug#comment2Wed, 17 Nov 2010 10:08:04 GMT