Terrible coding examples

[D.P.]

That is a complete synonym with what you have quoted, only you are on the side of the management in his post.

No, because we are allowed to refactor the code whenever we want and are constantly making tweaks as we see fit. What we don't do is have an official refactoring policy and set aside time. That is very different to what smyth was saying, their management don't let them and don't want the engineers spending time on refactoring, our managers don't care at all and let us do what we want but since we are always busy we don't have time to do serious refactoring. I could spend all weekend refactoring some of my code, or I could play with my daughter.

 

[peterwalkley]

Looks like someone was paid by the line.

 

[D.P.]

A common pattern I see is reversed conditional checks that needlessly indent code:

void someFunc()
{

  if (someVal == true) {
    // code
    // code
    if (someCondition) {
        //more indented code
        // more indented code
    }
    // code
    // code
  } else {
    // Sometimes this is omitted
    return;
  }
 
}

It is much cleaner to return early

void someFunc()
{

  if (someVal == false) 
       return;

  // code
  // code
  if (someCondition) {
        // indented code
        // indented code
  }
  // code
  // code

}

 

[sniffy]

It depends how open your company is to refactoring code. There are places where once you finish a "feature" / "bug fix" and get it tested and signed off, management are reluctant to go back in and refactor even though you have some nasty code that will most likely cause you loads of trouble down the line.

I need to find a new job ...

I just refrain from saying my work is complete until I've refactored it. I write some bloody awful code sometimes and it's usually the future me who has to deal with it

 

[robotSocks]

A common pattern I see is reversed conditional checks that needlessly indent code:

void someFunc()
{

  if (someVal == true) {
    // code
    // code
    if (someCondition) {
        //more indented code
        // more indented code
    }
    // code
    // code
  } else {
    // Sometimes this is omitted
    return;
  }
 
}

It is much cleaner to return early

void someFunc()
{

  if (someVal == false) 
       return;

  // code
  // code
  if (someCondition) {
        // indented code
        // indented code
  }
  // code
  // code

}

ugh....return statements littered through a function are horrendous, and confusing. Particularly when loops are involved. There should be one single return statement - at the end of the method.

 

[Rroff]

ugh....return statements littered through a function are horrendous, and confusing. Particularly when loops are involved. There should be one single return statement - at the end of the method.

Depends a lot on the nature of your program and the purpose of the routine.

I've some ai functions that have multiple return points and would be extremely performance costly to do any other way (or produce some crazy spaghetti code).

 

[HungryHippos]

I just refrain from saying my work is complete until I've refactored it. I write some bloody awful code sometimes and it's usually the future me who has to deal with it

// future me - if you are reading this then it didn't work.
// time travel is not possible I couldn't go back and fix this code up.

 

[Rosbif]

I never refactor ANYTHING. I don't even sit down and review my code. If it hits the spec and passes everything I throw at it that is it, done and dusted.

It does mean, however, doing things as sensibly as possible in the first place. For me it's all about getting stuff done ASAP.

Users and Clients only see the end result. A Client hearing 'Yeah the guy/firm who wrote this initially was an idiot' just hears 'waffle waffle, excuse, excuse, sound of pound coins falling out of his pocket'

 

[Dj_Jestar]

Then the same client wants something changed, all they here is "OMG how terrible is this?! It's going to take us ages to fix it. In fact we're just going to have to start again, sorry."

 

[azteched]

A common pattern I see is reversed conditional checks that needlessly indent code:

Yes, I hate this contortion of code to fit an artificial "THOU SHALT HAVE A SINGLE RETURN STATEMENT" rule. Makes the code so much more complex to follow.

 

[Haircut]

I never refactor ANYTHING. I don't even sit down and review my code. If it hits the spec and passes everything I throw at it that is it, done and dusted.

It does mean, however, doing things as sensibly as possible in the first place. For me it's all about getting stuff done ASAP.

Users and Clients only see the end result. A Client hearing 'Yeah the guy/firm who wrote this initially was an idiot' just hears 'waffle waffle, excuse, excuse, sound of pound coins falling out of his pocket'

That's great if you write code and then it's never touched again.
What normally happens is the client wants a slight tweak in how something works here, a few extra things added there and your sensibly written code starts to look a bit like Frankenstein's monster with random parts added here there and everywhere.

If you refactor as you're going along (which could involve major surgery to the existing code to fit in with what is needed now) then you can keep things reasonably clean, but a never refactoring approach always leaving you with bad code and a maintenance nightmare in the long run in my experience.

I often refactor as I'm developing a single feature. As you put things together and start building stuff then often a better approach than what you started with will jump out, and it's time for a refactor.

 

[JimAroo]

nvm delete please.

 

[st00dent]

In the login script of a project I picked up...

$sql = "
SELECT 
* 
FROM users 
WHERE 
username = '".$_POST['username']."' AND
(password = '".$_POST['password']."' OR password = '".md5($_POST['password'])."')

 

[D.P.]

Yes, I hate this contortion of code to fit an artificial "THOU SHALT HAVE A SINGLE RETURN STATEMENT" rule. Makes the code so much more complex to follow.

Agreed, early exits are elegant and simple. You can get some horrifically convoluted code that is horribly complex series of nested if statements.

I think some people take certain recommendations far too literally rather than a suggestion.

 

[Lightnix]

Yeah, I feel like exiting early is preferable to nesting because nesting tends to reduce readability. Both options are preferable to using a goto to reach your single return statement, though.

 

[Russinating]

Resurrection!

Was just checking my account details on the Rugby World Cup website - you'd expect that to be pretty robust - and saw this which made me chuckle geekily.

Date of Birth: 16/07/1986 00:00:00

Stored as a Unix time, fine, I guess, but output with HH:MM:SS as well?!

Actual DOB date changed btw

 

[Scougar]

That's great if you write code and then it's never touched again.
What normally happens is the client wants a slight tweak in how something works here, a few extra things added there and your sensibly written code starts to look a bit like Frankenstein's monster with random parts added here there and everywhere.

If you refactor as you're going along (which could involve major surgery to the existing code to fit in with what is needed now) then you can keep things reasonably clean, but a never refactoring approach always leaving you with bad code and a maintenance nightmare in the long run in my experience.

I often refactor as I'm developing a single feature. As you put things together and start building stuff then often a better approach than what you started with will jump out, and it's time for a refactor.

Yup. I have a feature on the home page of web site that takes up half the page. It didn't start out that way, but over the months after release the requirements got amended so refactoring was not necessary, but sensible and logical. Now each country is wanting that feature on their home page and other pages. In another iteration I had to add a second flow that called an api instead of calling via our normal code route, so had to refactor to take that into account.

I am very surprised that a developer is able to even say they never refactor.

 

[mid_gen]

We recently got some software from a partner to take over nd fix up. they were in charge of writing it but it kept crashing and seemed to have endless problems. It is is full of beautiful gems like this:

 public UInt64 shift(int i)
                {
                        if (i ==0)
                                return (1 << 0);
                        else if (i == 1)
                                return (1 << 1);
                        else if (i == 2)
                                return (1 << 2);
                        else if (i == 3)
                                return (1 << 3);
                        else if (i == 4)
                                return (1 << 4);
                        else if (i == 5)
                                return (1 << 5);
                        else if (i == 6)
                                return (1 << 6);
                        else if (i == 7)
                                return (1 << 7);
                        else if (i == 8)
                                return (1 << 8);
                        else if (i == 9)
                                return (1 << 9);
                        else if (i == 10)
                                return (1 << 10);
                        else if (i == 11)
                                return (1 << 11);
                        else if (i == 12)
                                return (1 << 12);
                        else if (i == 13)
                                return (1 << 13);
                        else if (i == 14)
                                return (1 << 14);
                        else if (i == 15)
                                return (1 << 15);
                        else if (i == 16)
                                return (1 << 16);

                        return 0;
                }

not that its broken, but why, just why!


Any examples of madness you have seen?

I had a contractor curl out something like this for me once. He wasn't retained.

Displays a fundamental lack of basic programming knowledge. I've dropped some nasty code here and there but nothing that bad (I don't think).

 

[JonJ678]

Agreed, early exits are elegant and simple. You can get some horrifically convoluted code that is horribly complex series of nested if statements.

This is language dependent. Great plan iff resources associated with the stack clean up on return. Without some variant of with() or raii, early return implies leak. Always bad in C, often bad in garbage collected languages, great plan for modern C++

 

[uncle_rufus]

My goodness I could find a lot of these if I wanted... but here's one (+extra points for being Fortran!)

I saw a curious block where a developer had what I can assume must have at one point been a controllable switch or something used for debugging... but either way at this point it was being hardcoded. A little messy and lazy that it wasn't simply removed; not the biggest sin in the world, but the variable name was a little curious:

do_not_filter = .TRUE.
IF (do_not_filter) THEN
  ! Inside the statement was the code which 
  ! *did* do the filtering
END IF