http://ayende.comAyende @ RahienCopyright (C) Ayende Rahien 2004 - 2021 (c) 202660bool isJohnGalt = this.WhoIsJohnGalt(); if (isJohnGalt.Equals(true)) //do something //better still if(isJohnGalt = true) //do something more //yet another one Customer item = new Customer(); while (reader.Read()) { item.IsJohnGalt = reader.GetBoolean(0); item.CustomerCode = reader.GetInt32(1); item.Name = reader.GetString(2); } return item; //Most common one - same as vjd pointed out earlier doing a ToString to something that is already a string http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment65http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment65Mon, 02 May 2011 07:07:50 GMTpublic void DoStuff() { // ... return; // <- Really? } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment64http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment64Thu, 21 Apr 2011 08:37:56 GMTI like a lot this boolean redundancy like: if (!confirm('A senha será cancelada, confirma?')) {return false;}; Or bool validUser = Service.ValidateUser(Username, Password); if (!validUser) { return false; } return true; http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment63http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment63Tue, 19 Apr 2011 15:32:45 GMTThis little golden nugget was written by a contract developer earning a fairly decent daily rate. Needless to say he has gone!! Private Function DoesFileNameStartWithATimeStampValue(ByVal FName As String) As Boolean Try 'this function returns true if the first 4 characters of fname are a valid year (from 1900 to 100 years from the current year) 'and if the next 2 characters are between 1 and 12 (month) and if the next 2 characters are between 1 and 31 (days of month) Dim result As Boolean = False If FName.Length > 14 Then 'these checks are to make sure that the leftmost characters are a timestamp value. eg "20110222152115" If Microsoft.VisualBasic.IsNumeric(FName.Substring(0, 14)) Then Dim YearValue As Integer = CInt(FName.Substring(0, 4)) Dim YearNumber As Integer = 1990 'start from 1990 years back Do Until YearNumber > System.DateTime.Now.Year + 100 If YearValue = YearNumber Then Dim MonthsValue As String = FName.Substring(4, 2) Dim Months As Integer = 1 Do Until Months > 12 ' (Months).ToString("00") eg 0 = 01 If (Months).ToString("00") = MonthsValue Then Dim DaysValue As String = FName.Substring(6, 2) Dim Days As Integer = 1 Do Until Days > 31 If (Days).ToString("00") = DaysValue Then result = True Exit Do End If Days += 1 Loop If result Then Exit Do End If Months += 1 Loop If result Then Exit Do End If YearNumber += 1 Loop End If End If Return result Catch ex As Exception Return False End Try End Function http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment62http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment62Tue, 19 Apr 2011 11:04:09 GMTSeen a few good ones. // we need to wait for middleware server for 5 seconds DateTime currentTime = DateTime.Now while((DateTime.Now - currentTime).TotalSeconds < 5) { } (or something to that effect, not tested). This made it to production server on one of most popular price comparison sites a few years ago, debugging 100% cpu kept the team going for while. One of the guys found it so funny he printed and framed it, but he was on the other side of the office so the victim was none the wiser. Also someone decided to reinvent HtmlEncode and UrlEncode, inlined in same class doing the web scraping of course. http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment61http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment61Sun, 17 Apr 2011 11:45:07 GMTI got a good one If(UserID == 123) { UserID = 321; } //... continue login process with a different user. I wonder what will happen on prod... http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment60http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment60Sat, 16 Apr 2011 15:44:12 GMTThis was actually in production for several years: if(status == "Continue") { if(status == "Cancel") { return; } //some logic } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment59http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment59Sat, 16 Apr 2011 04:00:39 GMT@Steve py I've seen your minor wtf a lot. People like to put a breakpoint in the throw; so that it stops there instead of crashing. And yes, no one listens when I say there's an option to do that automatically. http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment58http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment58Fri, 15 Apr 2011 22:37:28 GMTSeveral days ago I stumbled on this in our Silverlight application: public SomeUserControl() { try { InitializeComponent(); } catch(Exception) { InitializeComponent(); } } i found it so funny so I didn't touch this :) http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment57http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment57Fri, 15 Apr 2011 19:49:30 GMTI've seen this a lot in the last couple of years, often in Microsoft sample code and conference sessions! var a = b as C; a.SomeMethod(); It seems that this viral confusion started somehow. Someone in a trusted position probably wrote some code or issued some best practice advice that since "as" is a "safe" cast which won't throw if it fails, it should be used instead of a regular typecast. Then the likes of Scott Hanselman and Rob Conery started doing it. NB: I can't be remember for sure that I saw this in either of these guys code, so I'm saying the *likes* of these guys ;) http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment56http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment56Fri, 15 Apr 2011 16:48:20 GMTbool isValid = false; .... if (isValid == null) { ... } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment55http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment55Fri, 15 Apr 2011 10:41:24 GMT// Todo: Must fix http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment54http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment54Fri, 15 Apr 2011 00:46:45 GMTAnother good one: try{ .... }catch{ MessageBox.Show("An error occured"); } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment53http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment53Thu, 14 Apr 2011 14:44:48 GMTMaybe this is more in the 'clearly the programmer does not understand pass by reference", but anyways: public void ClearToken(Token token) { if (token != null) token = null; } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment52http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment52Thu, 14 Apr 2011 14:33:12 GMTWe had this lovely piece that made it into prod, for a short while until it was asked why it wasn't working. Of course it worked in dev/test where debug was enabled for logging and failed in prod where it was obviously set at error. public void someMethod() { .... .... if (logger.debug.enabled == true { Logger.debug("some log message"); Important business code here!! } } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment51http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment51Thu, 14 Apr 2011 13:37:54 GMTRead this post and comments as I was reviewing some legacy code for a customer - not necessarily worthy of deadly sin status, but certainly worth a giggle. [SuppressMessage("Microsoft.Design", "CA1024:UsePropertiesWhereAppropriate", Justification = "'It can not be converted into a property as it needs an open connection and the one of the constructors" + " does not take connection as a parameter")] public decimal GetSomeValueOrOther() {//literally loads of code...} Excuses, excuses excuses - just fix it already! and of course within this very same method: catch (Exception ex) { Logger.Log("a looked up exception message"); throw new UtterlyMeaninglessException(ex.Message); } names changed to protect the guilty of course. Thanks for a good laugh! http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment50http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment50Thu, 14 Apr 2011 11:54:58 GMTI saw this in a locked down TFS branch (accessible only by the architect) public int GenericAddRecordToTableMethod(string SQL) { // instantiate connection objects etc object result = SqlCommand.ExecuteNonQuery(); return Convert.ToInt32(result.ToString()); } The rest of the site had similar issues with boxing/unboxing and casting down to object http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment49http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment49Thu, 14 Apr 2011 10:59:39 GMTFun with singletons: public class MySingleton {     private static MySingleton _instance = null;     private MySingleton() { }     public static MySingleton GetInstance()     {         if(_instance == null)         {             // expensive initialisation logic that should only get called once here         }         return _instance;     } } And this is in a heavily traffic'd web app :-) PS: Ayende, formatting code in these comments is such a pain http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment48http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment48Thu, 14 Apr 2011 09:13:22 GMT@Simon (4/13/2011 2:39 PM): so true: the class itself was not static, of course, but almost its members and state were static, so it effectively became a "use-once-per-process" type. http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment47http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment47Thu, 14 Apr 2011 08:59:09 GMTSomeClass instance = new SomeClass(); instance = FunctionThatReturnSomeClass(); http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment46http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment46Thu, 14 Apr 2011 08:15:01 GMT+1 for: if ( sthThatEvaluatesToBool( args) ) { return true; } else { return false; } and saw: class FooViewModel { public void Show(ViewModelBase viewModel) { if(viewModel.GetType().Name == "OrderViewModel") do something; if(viewModel.GetType().Name == "InvoiceViewModel") do something; } } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment45http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment45Thu, 14 Apr 2011 07:36:00 GMT@Fredrik How is it possible you've seen this in production c ode? public static class WhatEver : IDisposable { // code } This would be a compile time error... static classes cant implement interfaces... http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment44http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment44Thu, 14 Apr 2011 02:51:06 GMT@Tony Of the original 7 deadly sins, from the Bible, I don't think any of them were actually deadly in an immediate sense. In fact, a few of them are quite enjoyable. Likewise, so are the deadly sins I've listed for programmers. http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment43http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment43Thu, 14 Apr 2011 00:11:05 GMT// C# .net code Application.DoEvents(); NOOOOOOOOOOOO!!!! THE MOST EVIL METHOD EVER WHEN USED ON A PLATFORM THAT FULLY SUPPORTS ADVANCED THREADING!!! Debugging re-entrancy issues due to this old chestnut has consumed much of my life I will never get back. http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment42http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment42Thu, 14 Apr 2011 00:04:43 GMTpublic class SomeWrapper : IDisposable { private AnotherDisposableResource _resource = null; public SomeWrapper(AnotherDisposableResource resource) { this._resource = resource; } public void Dispose() { // does nothing!! ARGH! } } http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment41http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment41Thu, 14 Apr 2011 00:02:07 GMTJust lazy... only writing 3 out of 7. http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment40http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment40Wed, 13 Apr 2011 23:25:22 GMT- Any time I see commented out code checked in. - 95% of times I see a Singleton or static class added. - Most times I see a class "newed" in-line. - Just about any comment generated by GhostDoc. (As being either completely pointless, or completely missing the point.) I.e: /// <summary /// Gets the name of the unique. /// > /// <typeparam> /// <paramThe list. /// <paramThe propname. /// <paramThe name. /// <paramThe prefix. /// <returnsthe unique name > public string GetUniqueName <t(IEnumerable <t list, string propname, string name, string prefix = "") A minor "wtf": try { // Do stuff } catch { throw; } literally around every block of code. (Some kind of weird template.) >http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment39http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment39Wed, 13 Apr 2011 22:57:29 GMT@frank Why is deadly? It's just unnecessary code which runs fine without possible errors? http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment38http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment38Wed, 13 Apr 2011 20:36:44 GMTI swear I saw this in an application having multi-language as one of the main requirements: if (language="en") { return "Hello"; } else if (language="es") { return "Hola"; } //and so on... for each string literal. Now, try to add a new language... and search for this code in all of your application. My eyes are still crying after seeing this. =8-O http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment37http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment37Wed, 13 Apr 2011 20:14:23 GMTforeach (x in collection1) // ~ 3000 instances foreach (y in collection2) ~ 7 instances foreach(z in collection3) ~ 3 instances var results = do query by y and z talk about N^3+1 :) notice that the query doesn't depend on x http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment36http://ayende.com/4810/the-seven-deadly-sins-for-the-developer-some-restrictions-apply#comment36Wed, 13 Apr 2011 20:09:50 GMT