Ayende @ Rahien

It's a girl

The seven deadly sins for the developer (* some restrictions apply)

Originally posted at 3/30/2011
I have seen all of those in production code…
  1. throw new NullReferenceException();
  2. public class AbstractController : Controller
    // or 
    // public class AbstractPag : Page
    {
        public static bool IsAdmin { get;set; }
    }
  3. public static class BlogMgr
    {
        public static void AddPostToBlog(Blog blog, Post post)
        {
                blog.Posts.Add(post);
                post.Blog = blog;
        }
    }

* I could only think of three, but I didn’t want to modify the title. Can you complete the list?

Comments

Bob
04/13/2011 09:28 AM by
Bob

How about:

lock (new object()) {

...stuff

}

Mark Talbot
04/13/2011 09:30 AM by
Mark Talbot

how about my favourite of all time

try

{

//something

}

catch

{

//absolutely nothing

}

marisks
04/13/2011 09:31 AM by
marisks

This too:

try

{

// no code here - catching nothing :)

}

catch(Exception ex)

{

Logger.Log(ex.Message);

}

Ako
04/13/2011 09:31 AM by
Ako

Can you explain more, why they are deadly sins? (I am a begginer)

Thank

Mike Minutillo
04/13/2011 09:32 AM by
Mike Minutillo

How about:

try {

// ...

} catch(Exception ex) {

logger.Log(ex);

throw ex;

}

My favorite prod code was:

if(personId == null) {

/* null personId */

} else {

// do something with personId

}

OR

if(something.GetType() == typeof(TypeA)) {

var typeASomething = (TypeA)something;

// do something with type A thing

} else if(something.GetType() == typeof(TypeB)) {

var typeBSomething = (TypeB)something;

// do something with type B thing

} else if ...

Mel
04/13/2011 09:32 AM by
Mel

public static class Helper

marisks
04/13/2011 09:35 AM by
marisks

I'm refactoring one legacy project and this is quite often:

if(true) { //or other condition which is 100% true

return;

}

// hundreds of code lines here doing complex stuff, which never executes :)

Boris
04/13/2011 09:38 AM by
Boris

T SuperDescriptiveFunctionName() {

// Doing something completely different...

// With matching totally unrelated comments...

}

Mike Minutillo
04/13/2011 09:39 AM by
Mike Minutillo

Hi @Ako - Here's my take on them.

In (1) the developer is throwing a framework-level exception. For a developer encountering this it will be indistinguishable from just accessing the null reference in the first place and be a PITA to find and fix. In fact, the code to check this and throw is entirely unnecessary. The framework will detect the situation and throw the same exception.

(2) is storing something session specific in a static variable (which is site-wide). Very scary. Also, a nitpick, if a class has abstract in the name, I'd make it an abstract class

(3) The logic for how to perform this action belongs in one or the other of the classes. The very fact that you CAN write this method means that people can change the state of the model without using it. i.e. Someone might set the Blog property on the Post object without updating the Blog. This needs to be internal to the Post/Blog so that the operation is atomic.

Ryan Heath
04/13/2011 09:46 AM by
Ryan Heath

Stream stream = new ...

// use stream

stream.Flush();

stream.Close();

stream.Dispose();

stream = null;

Instead of

using(var stream = new ...)

{

// use stream

}

What bothers me most is the null assignment, do people still don't know .net uses a garbage collecter and not reference counters?

// Ryan

nickd
04/13/2011 09:56 AM by
nickd

These look more like WTFs than 'deadly'. Confusing and verbose perhaps, but 'deadly' ?

gedgei
04/13/2011 10:08 AM by
gedgei

if(someString.Length <= 0)

return;
Ashic Mahtab
04/13/2011 10:33 AM by
Ashic Mahtab
  1. public class SomeDomainEntity

{

public string Name {get;set;}

}

[In the domain public getters increase coupling, public setters kill you]

  1. var person = repository.Get <somedomainperson(id);

person.Name= foo;

person.Age = bar;

repository.Save();

[Violation of Tell, don't ask]

  1. public class Rectangle

{

public void SetHeight(height);

public void SetWidth(width);

}

public class Square : Rectangle

{

}

[violation of Liskov]

rafek
04/13/2011 10:56 AM by
rafek

What amazes me the most is how often I can see the following code in production:

if ( sthThatEvaluatesToBool( args) ) {

return true;

} else {

return false;

}

Fredrik M&#246;rk
04/13/2011 11:20 AM by
Fredrik Mörk

I came across such things:

public static class WhatEver : IDisposable

{

// code

}

Rob Kent
04/13/2011 11:38 AM by
Rob Kent

Er, entitling an open-ended resultset the 'seven deadly sins'?

Simon Bartlett
04/13/2011 11:39 AM by
Simon Bartlett

@Fredrik I doubt you have come across that! It is not possible for static classes to implement interfaces - and it wouldn't compile if you tried.

anon
04/13/2011 12:08 PM by
anon

"public static bool IsAdmin" in a web app means every one is an Admin or could be an admin!

vjd
04/13/2011 01:05 PM by
vjd

Response.QueryString["XXXX"].ToString()

jonnii
04/13/2011 01:10 PM by
jonnii

In a win forms applications, something like this...

public abstract class FormBase : Form {

private void SubmitClicked(object sender, EventArgs e){

if(this is ProductForm){

  ... 100 lines of stuff ...

}

else if (this is CategoryForm){

  .. 100 lines of stuff copy and pasted from above and modified slightly ...

}

}

}

Derek Thornton
04/13/2011 01:11 PM by
Derek Thornton

@Bob:

// quote

lock (new object()) {

...stuff

}

// end-quote

I'm very guilty of this one. It seemed like a really good fix at the time until I learned what it was really doing.

Rob Eisenberg
04/13/2011 01:29 PM by
Rob Eisenberg

Here's a nasty one from the Xaml world:

public class CustomerViewModel : DependencyObject {}

Dominic Pettifer
04/13/2011 01:40 PM by
Dominic Pettifer

One of my favourites:

DateTime startTime = DateTime.Now;

do

{

  try

  {

    dataAdaptor.Fill(dataSet);

    return dataSet.Tables[0];

  }

  catch (Exception err)

  {

    if (DateTime.Now.Subtract(startTime).TotalMinutes > 1)

    {

      throw new Exception(err.Message);

    }

    Thread.Sleep(3000);

  }

} while (true);

Translation: If at first you don't succeed, try try again.....but only for a minute (taking a 3 second rest after every try), then give up!

Frank Quednau
04/13/2011 01:57 PM by
Frank Quednau

Haha, throwing NullReferenceException, sweet chums...

Personally, I totally hate analogs to

if (foo == true)

{

return true;

}

else

{

return false;

}

instead of return foo;

DS
04/13/2011 02:00 PM by
DS

class SomeClass {

static object syncRoot = new Object();

....

public void SomeMethod() {

lock (syncRoot) {

// perform db work in a transaction

}

}

}

Steve
04/13/2011 02:17 PM by
Steve

@Krzysztof,

"ServiceLocator.Resolve()"

There's an app here that insists on doing this in every method, I won't go into the reasoning why because it's stupid and they won't listen to reason. But just for sh!ts and giggles, I just pulled open their solution from Source and did a quick count for a grand total of 3258 Resolves in a single app.

And so I'm contributing, here's one I caught where I used to work:

try

{

return true;

}

catch (Exception ex)

{

throw;

}

Alex Simkin
04/13/2011 03:29 PM by
Alex Simkin

@Rob Eisenberg

What problem with this except that DependencyObjects are not marked as serializable?

Chris Wright
04/13/2011 03:50 PM by
Chris Wright

[Test]

public void TestSomeThings()

{

TestWithParameters("blarg", "", expectedError, null, 4, null);

TestWithParameters("bad value", "exclude", expectedError, null, 2, null);

// and 10 more lines of this

}

The tests SHARE STATE.

I also saw tests that connect to a special unittest database and expect certain values to be present. (There's nothing wrong with connecting to an actual database rather than an in-memory database, precisely; but the tests should expect a blank database to start.)

Andrew
04/13/2011 05:04 PM by
Andrew

Nulling out a reference to a disposed object might be an attempt to provoke an exception that almost certainly won't get caught if someone tries to use it again.

fschwiet
04/13/2011 05:17 PM by
fschwiet

1) Premature optimization

2) Speculative generality

Luke Winikates
04/13/2011 05:18 PM by
Luke Winikates

@Mike

about--

try {

// ...

} catch(Exception ex) {

logger.Log(ex);

throw ex;

}

I agree it smells, and it's definitely a cop-out. The developer is basically saying "I have no idea what could go wrong in this code, if anything. If something goes wrong, hopefully I'll find out later." But It makes me a little optimistic that the person is rethrowing. Usually I feel like I see a less evolved form of this, like:

catch(Exception ex) {

logger.Log(ex);

}

Rethrowing ex makes me feel much better about the situation.

Moti
04/13/2011 07:01 PM by
Moti

@Luke:

  1. If the developer doesn't know what could go with his code, he is facing shitload of problems anyway. No optimism is in order here...

  2. Catch "Exception" only once in your app.

  3. use "throw", not "throw ex;".

Luke Winikates
04/13/2011 07:29 PM by
Luke Winikates

@Moti

  1. Well, limited optimism definitely. At least the exception is not getting swallowed completely. Like I said, the rethrowing makes me feel a little better; this person has learned that "log and ignore" is not a good strategy, and at least they're favoring "log and fail" instead.

  2. That's a really good point-- I hadn't thought known about this, but sure enough, "throw ex" loses the stack trace (perhaps it has other downsides as well). Thanks for pointing that out.

Nadav
04/13/2011 08:09 PM by
Nadav

foreach (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

Pedro J. Molina
04/13/2011 08:14 PM by
Pedro J. Molina

I 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

Tony
04/13/2011 08:36 PM by
Tony

@frank

Why is deadly? It's just unnecessary code which runs fine without possible errors?

Steve Py
04/13/2011 10:57 PM by
Steve Py
  • 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.

    /// 
    /// 


/// <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.)

Russell Wilson
04/13/2011 11:25 PM by
Russell Wilson

Just lazy... only writing 3 out of 7.

Adam Langley
04/14/2011 12:02 AM by
Adam Langley

public class SomeWrapper : IDisposable

{

private AnotherDisposableResource _resource = null;

public SomeWrapper(AnotherDisposableResource resource)

{

this._resource = resource;

}

public void Dispose()

{

// does nothing!! ARGH!

}

}

Adam Langley
04/14/2011 12:04 AM by
Adam Langley

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

fschwiet
04/14/2011 12:11 AM by
fschwiet

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

Me
04/14/2011 02:51 AM by
Me

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

Steve Wagner
04/14/2011 07:36 AM by
Steve Wagner

+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;

}

}

Gian Maria Ricci
04/14/2011 08:15 AM by
Gian Maria Ricci

SomeClass instance = new SomeClass();

instance = FunctionThatReturnSomeClass();

Fredrik M&#246;rk
04/14/2011 08:59 AM by
Fredrik Mörk

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

Dominic Pettifer
04/14/2011 09:13 AM by
Dominic Pettifer

Fun 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

Reza
04/14/2011 10:59 AM by
Reza

I 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

Mark Porter
04/14/2011 11:54 AM by
Mark Porter

Read 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!

Chris Fisher
04/14/2011 01:37 PM by
Chris Fisher

We 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!!


 }

}

Thomas Christensen
04/14/2011 02:33 PM by
Thomas Christensen

Maybe 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;

}

Steve Wagner
04/14/2011 02:44 PM by
Steve Wagner

Another good one:

try{

....

}catch{

MessageBox.Show("An error occured");

}

Jorgen
04/15/2011 12:46 AM by
Jorgen

// Todo: Must fix

Steve G
04/15/2011 10:41 AM by
Steve G

bool isValid = false;

....

if (isValid == null)

{

...

}

Mike Scott
04/15/2011 04:48 PM by
Mike Scott

I'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 ;)

dave
04/15/2011 07:49 PM by
dave

Several 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 :)

Jorge Alves
04/15/2011 10:37 PM by
Jorge Alves

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

Arturo Molina
04/16/2011 04:00 AM by
Arturo Molina

This was actually in production for several years:

if(status == "Continue")

{

if(status == "Cancel")

{

  return;

}

//some logic

}

Chen Kinnrot
04/16/2011 03:44 PM by
Chen Kinnrot

I got a good one

If(UserID == 123)

{

UserID = 321;

}

//... continue login process with a different user. I wonder what will happen on prod...

Matt Freeman
04/17/2011 11:45 AM by
Matt Freeman

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

James_2JS
04/19/2011 11:04 AM by
James_2JS

This 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
Winston
04/19/2011 03:32 PM by
Winston

I 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;

Gavin
04/21/2011 08:37 AM by
Gavin

public void DoStuff()

{

// ...

return; // <- Really?

}

Binoj Antony
05/02/2011 07:07 AM by
Binoj Antony

bool 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

Comments have been closed on this topic.