Coding Horror

Seth Brundle · 25-08-2018 9:08pm

Just saw this (not horror code but just humourous)...

mooonpie · 28-08-2018 2:26pm

Took me a while to dig out ... but I knew that concept seemed familiar: https://thedailywtf.com/articles/The-Speedup-Loop

BrokenArrows · 28-08-2018 2:35pm

mooonpie wrote: »

Took me a while to dig out ... but I knew that concept seemed familiar: https://thedailywtf.com/articles/The-Speedup-Loop

I thought of that when i read the post but i couldn't find it.
Its hilarious if its true.

Evil Phil · 03-09-2018 3:04pm

Updated an interface, but it's redundant and will be removed in my next commit

*Somebody* did this today *cough* :pac:

14ned · 10-09-2018 10:33am

Seth Brundle wrote: »

Just saw this (not horror code but just humourous)...

Interestingly, in multithreaded C++ code it would not be uncommon to scatter around code of the form:

#ifndef NDEBUG
std::this_thread::sleep_for(std::chrono::milliseconds(random_testing_sleep()));
#endif

The idea is that under Monte-Carlo testing under the thread sanitiser, you're deliberately introducing stochastic variation to increase the combinatorial variations for the sanitiser to test upon.

The fun begins when somebody forgets to #define NDEBUG in the release builds, and even better when customers don't notice for over a year

Niall

victor8600 · 10-09-2018 10:52am

14ned wrote: »
#ifndef NDEBUG
std::this_thread::sleep_for(std::chrono::milliseconds(random_testing_sleep()));
#endif
The fun begins when somebody forgets to #define NDEBUG in the release builds, ...

This is horrific. I nearly choked on my coffee. I would be surprised if somebody remembers to define NDEBUG after a couple of years. The program should warn about the activation of the debugging code and the debug flag must be explicitly set

#define DEBUG 0
#if DEBUG
Log.print("Warning -- debugging is active. Performance will be negatively affected");
#endif

#if DEBUG
std::this_thread::sleep_for(std::chrono::milliseconds(random_testing_sleep()));
#endif

GreeBo · 10-09-2018 11:21am

victor8600 wrote: »
This is horrific. I nearly choked on my coffee. I would be surprised if somebody remembers to define NDEBUG after a couple of years. The program should warn about the activation of the debugging code and the debug flag must be explicitly set
#define DEBUG 0
#if DEBUG
Log.print("Warning -- debugging is active. Performance will be negatively affected");
#endif

#if DEBUG
std::this_thread::sleep_for(std::chrono::milliseconds(random_testing_sleep()));
#endif

+1
Your code should (pretty much) always run in production mode unless configured otherwise. Its annoying when it runs expecting prod infra in a small test env, but its usually much worse if its runs in test mode in prod.

Granted there are some caveats (i.e. you wouldnt configure it to run against a prod DB by default) but performance wise default should be prod settings.

14ned · 10-09-2018 11:39am

victor8600 wrote: »

This is horrific. I nearly choked on my coffee. I would be surprised if somebody remembers to define NDEBUG after a couple of years.

In fairness, if you're using cmake or any other modern build tool, NDEBUG gets defined for you in release builds by default. It's only when somebody writes a cmake flags stripper routine, and forgets to whitelist NDEBUG ...

victor8600 wrote: »

The program should warn about the activation of the debugging code and the debug flag must be explicitly set

Ah, but NDEBUG is not the opposite to DEBUG! Asserts always fire, debug and release builds, unless NDEBUG is defined. Been that way since C89. Similarly, for Monte-Carlo testing under the thread sanitiser, you're almost always compiling a release build but with asserts enabled. So it makes total sense to introduce random waits into a release, non-debug, build.

I would emphasise the fact that no customer noticed for over a year, and even then it was actually a BR by someone who had been stepping through our code, and went "WTF?" and asked if this was correct? Not that anybody had or found an issue with the performance loss.

C++ is often fast enough that if it runs 20% slower, or even 50%, nobody notices. It's "fast enough".

Niall

smacl · 19-09-2018 6:30pm

14ned wrote: »

C++ is often fast enough that if it runs 20% slower, or even 50%, nobody notices. It's "fast enough".

Hmmm,in the process of porting some performance critical C++ code that isn't fast enough (already multi-threaded via OMP) to HLSL to leverage the GPU. To say C++ is fast enough is meaningless. You need to say what exactly C++ is fast enough for, because fast enough for everything it aint. If you're in any doubt about this, ask yourself why there's a dedicated overclocking forum on boards?

Slutmonkey57b · 19-09-2018 8:20pm

It's a permanent problem with coders though. "good enough, don't have the time /energy /knowledge to fix it, the hardware will pick up the slack". We've all done it!

smacl · 20-09-2018 8:16am

Slutmonkey57b wrote: »

It's a permanent problem with coders though. "good enough, don't have the time /energy /knowledge to fix it, the hardware will pick up the slack". We've all done it!

Agreed, but I often wonder how many have spent significant time looking at orders of complexity of what their coding prior to implementation and spent the same time again in front of a profiler post implementation? Believing that the language, framework, SDK or hardware will do this for you is a fallacy. I've seen a lot of well structured, nice looking C++ with absolutely appalling performance characteristics over the last few years.

Sephiroth_dude · 23-09-2018 4:48pm

Why is it not ok to use try catch as controlling the flow of a program? I hear a lot of people giving out about this.

oscarBravo · 23-09-2018 5:03pm

Sephiroth_dude wrote: »

Why is it not ok to use try catch as controlling the flow of a program? I hear a lot of people giving out about this.

It's a matter of opinion. In some languages exception handling is expensive, and should only be used to catch actual errors. In others, it's a perfectly valid flow control mechanism.

BrokenArrows · 23-09-2018 6:09pm

Sephiroth_dude wrote: »

Why is it not ok to use try catch as controlling the flow of a program? I hear a lot of people giving out about this.

Mainly because if you do things correctly then Exceptions should only happen in exceptional circumstances.

Realistically you always control the flow of a program using exceptions but what you are supposed to do is recover from the exceptional state, log and report the problem to the user etc.

For example if you are connecting to an endpoint using a TCPClient which accepts the hostname and port number from the user. You dont just accept what the user gives you and try and connect, catch the exception when it fails and ask the user to try again. That would be bad usage of exception flow control.

You would check the port number for a valid range and check the hostname to make sure it is a valid hostname and doesnt contain spaces and if they are wrong you will report the error to the user. The exception should only catch the exceptions caused by connection failures which should only happen in exceptional circumstances.

Sephiroth_dude · 23-09-2018 9:49pm

BrokenArrows wrote: »

Mainly because if you do things correctly then Exceptions should only happen in exceptional circumstances.

Realistically you always control the flow of a program using exceptions but what you are supposed to do is recover from the exceptional state, log and report the problem to the user etc.

For example if you are connecting to an endpoint using a TCPClient which accepts the hostname and port number from the user. You dont just accept what the user gives you and try and connect, catch the exception when it fails and ask the user to try again. That would be bad usage of exception flow control.

You would check the port number for a valid range and check the hostname to make sure it is a valid hostname and doesnt contain spaces and if they are wrong you will report the error to the user. The exception should only catch the exceptions caused by connection failures which should only happen in exceptional circumstances.

If you had a program where you take in userInput and lets say it you want them to enter their name, so a string only, rather user try catch, you would be better off using if statement to validate it?

14ned · 23-09-2018 11:09pm

Sephiroth_dude wrote: »

If you had a program where you take in userInput and lets say it you want them to enter their name, so a string only, rather user try catch, you would be better off using if statement to validate it?

Ah, that's the narrow vs wide contract API design debate. See https://blog.regehr.org/archives/1436 for a flavour.

(tldr; "it depends")

Niall

oscarBravo · 24-09-2018 10:41am

Sephiroth_dude wrote: »

If you had a program where you take in userInput and lets say it you want them to enter their name, so a string only, rather user try catch, you would be better off using if statement to validate it?

14ned's "it depends" is the right answer.

Coming at it from a Python perspective, which is where I mostly live: if I need to validate a string input that's supposed to contain an integer value, the easiest way to do it is:

try:
    number = int(value)
except ValueError:
    [handle the exception]

If I need a floating point value:

try:
    number = float(value)
except ValueError:
    [handle the exception]

There simply isn't a simpler way to do it. There may be better approaches in other languages, which is fine: use what makes sense.

14ned · 24-09-2018 11:42am

oscarBravo wrote: »

There simply isn't a simpler way to do it. There may be better approaches in other languages, which is fine: use what makes sense.

So, putting this into the wide vs narrow contract terminology I linked to earlier, in Python exception raises are considered part of a function's wide contract with their caller i.e. if the function doesn't like its inputs, it can say "I refuse to complete due to reason X".

In other languages such as C++, exception throws are considered either wide or narrow i.e. if the function doesn't like its inputs, it panics i.e. aborts the program. In C++, "exception safe" code treats exception throws as wide. In "exception unsafe" code, exception throws are narrow.

In Rust, all functions default to narrow contract. If you call them wrong, the program aborts. However countervailing that, it's much harder to write code which calls a function wrong because the borrow checker will refuse to compile most instances. You then opt-in to wide contracts by returning a Result<T, E> on a function by function basis.

So you can see there is a pattern here. The higher level the language, the more wide contracts you tend to see. The lower level the language, the more narrow contracts you tend to see, at least in new code. Future C++ 26 is expected to have far more narrow contracts again, and become much more like Rust in this regard. We effectively are formalising the low level trueism that "use me wrong, I kill you".

Niall

Capt'n Midnight · 24-09-2018 11:44pm

amen · 25-09-2018 2:07pm

I'm robbing that Safety Pig. I have a home for it.

Sephiroth_dude · 28-09-2018 8:53am

oscarBravo wrote: »
14ned's "it depends" is the right answer.

Coming at it from a Python perspective, which is where I mostly live: if I need to validate a string input that's supposed to contain an integer value, the easiest way to do it is:
try:
    number = int(value)
except ValueError:
    [handle the exception]
If I need a floating point value:
try:
    number = float(value)
except ValueError:
    [handle the exception]
There simply isn't a simpler way to do it. There may be better approaches in other languages, which is fine: use what makes sense.

The reason I asked is because all the C# and Java heads tell me otherwise so was a bit confused about the whole thing

Anima · 28-09-2018 10:30am

In C# and Java, they're probably right. I think it does very much depend. You see them being used in non-exceptional circumstances all the time so hard to form a set of rules. Probably experience will teach you when to use them.

If you're a beginner, I'd just say if you can't return a value from a method that is suitable in an error condition, just throw the exception and deal with it that way. Likewise if you have a method/function that doesn't return something and has errored, then throw an exception.

Capt'n Midnight · 02-10-2018 11:25pm

https://asset-1.soup.io/asset/14391/0449_1622_849.gif

I thought this was a blast from the past. It's not.

Anesthetize · 03-10-2018 7:00am

Capt'n Midnight wrote: »

https://asset-1.soup.io/asset/14391/0449_1622_849.gif

I thought this was a blast from the past. It's not.

Looks like he's still on JDK 1.1

cython · 08-10-2018 10:37pm

Coding horror might be a bit strong, but didn't know where else to put this: https://github.com/auchenberg/volkswagen

Volkswagen detects when your tests are being run in a CI server, and makes them pass.

14ned · 09-10-2018 10:34am

cython wrote: »

Coding horror might be a bit strong, but didn't know where else to put this: https://github.com/auchenberg/volkswagen

Funnily enough, I've written code many times in my career which detects when we are running inside a CI (usually by inspecting environment variables), and early outs some tests with a pass.

As a contractor, you do exactly what your client tells you to do

Niall

tobsey · 07-11-2018 10:49am

Fits here well enough: http://www.drregex.com/2018/11/how-to-match-b-c-where-abc-beast-reborn.html

BrokenArrows · 07-11-2018 12:10pm

tobsey wrote: »

Fits here well enough: http://www.drregex.com/2018/11/how-to-match-b-c-where-abc-beast-reborn.html

horror but also genius

iLikeWaffles · 07-11-2018 12:57pm

BrokenArrows wrote: »

horror but also genius

It's vary hard to follow, is it Python? From the looks its waiting for numbers.

MajesticDonkey · 07-11-2018 1:54pm

iLikeWaffles wrote: »

BrokenArrows wrote: »

horror but also genius

It's vary hard to follow, is it Python? From the looks its waiting for numbers.

It's a regular expression, not a programming language per se...

oscarBravo · 07-11-2018 2:42pm

tobsey wrote: »

Fits here well enough: http://www.drregex.com/2018/11/how-to-match-b-c-where-abc-beast-reborn.html

That's about the scariest thing I've ever seen.

Igotadose · 07-11-2018 6:27pm

oscarBravo wrote: »

That's about the scariest thing I've ever seen.

Great comment:

So this is what programming with Satan is like

BrokenArrows · 15-11-2018 10:45am

Found a beauty today.

So ive setup a Jenkins build server for our team and added the SonarQube plugin https://www.sonarqube.org/. This scans the code base and uploads the results to our sonar server. It reports on issues with the code, potential bugs and vulnerabilities and general ****ty code.

I added some of our older projects to it as a test and this issue got reported.

private void WriteBump()
{
    Height = Height;
    Width = Width;
    BackgroundColor = BackgroundColor;
    StartDay = StartDay;
}

Why would you create a method to set each property to itself?

Then I have a look at each of the properties and see in the property the Get and Set.

public int Width
{
    get { return GetValue("Width"); }
    set { SetValue("Width"); }
}

In the Get and Set value he is maintaining a Dictionary with a bunch of different properties data.

WTF. WHY?

Berserker · 06-12-2018 3:06pm

Drafted onto a new project today, which is running behind schedule and the code is full of Christmas crackers. No unit tests in it and no documentation either.

public string CurrencyMoneyStripper(string Currency)
{
	if (Currency.Contains(".") == false)
		Currency = Currency + ".00";

	if (String.IsNullOrEmpty(Currency))
		return String.Empty;
	else
		return Currency.Substring(0, Currency.IndexOf('.'));
}

BrokenArrows · 10-12-2018 5:01pm

You know you are probably in for some **** coding when you come across this. Yes he called the static class StuffManager and the method doStuff.

He must have been having a bad day.

    class Program
    {
        static void Main(string[] args)
        {
            StuffManager.doStuff(args);
        }
    }

BrokenArrows · 20-12-2018 1:09pm

What the **** are these response enums supposed to represent ffs.

public enum ResponseCodeEnum
    {
        A = 0,
        N = 1,
        E = 2,
        D = 3
    }

seamus · 20-12-2018 1:18pm

BrokenArrows wrote: »
What the **** are these response enums supposed to represent ffs.
public enum ResponseCodeEnum
    {
        A = 0,
        N = 1,
        E = 2,
        D = 3
    }

A = A-OK!
N = No Dice
E = Ehhh...WTF
D = Do it yourself, I'm out

BrokenArrows · 20-12-2018 1:59pm

Im starting to think i work with a bunch of cowboys with the amount i post in this thread. haha.

Berserker · 20-12-2018 4:29pm

BrokenArrows wrote: »

Im starting to think i work with a bunch of cowboys with the amount i post in this thread. haha.

Oh, I'd wager that I could give you a very good run for your money. I'm cautious about posting code in case someone from the company spots it.

I worked my way through thousands of line of utter sh1te last week. One aspx page had 3,000+ lines of javascript in the header tag. This code was written by very senior devs, not juniors. They are all permies with lead level salaries. How the fcuk do these people get these jobs!!!

iLikeWaffles · 20-12-2018 9:46pm

Could they be index positions?

devlinio · 20-12-2018 10:32pm

WTF is that.

I work on a project and one of the controllers is 3500 lines. There's absolutely no need. My job in the new year if refactoring this.

elvis83 · 21-12-2018 12:05am

BrokenArrows wrote: »

Im starting to think i work with a bunch of cowboys with the amount i post in this thread. haha.

You are definitely not alone.

And as to why these jokers are still employed? You have to remember who had the control here. The business side have to deliver X by some date. They don't care about code, at all. That might be hard to hear but it's true. They don't even care about quality. They just want X by date. If X fails after date - well that's where the support contract kicks in...

elvis83 · 21-12-2018 12:05am

BrokenArrows wrote: »

Im starting to think i work with a bunch of cowboys with the amount i post in this thread. haha.

You are definitely not alone.

And as to why these jokers are still employed? You have to remember who have the control here. The business side have to deliver X by some date. They don't care about code, at all. That might be hard to hear but it's true. They don't even care about quality. They just want X by date. If X fails after date - well that's where the support contract kicks in...

off.the.walls · 21-12-2018 10:02pm

Trying to make vuejs modular... that is all.

haskellgeek · 31-03-2019 12:53am

not production code but for C someone recommended *(p+2) rather than p[2].

henryporter · 03-05-2019 6:31pm

This week was a bad week for the world of contributions to Java...

someVariable = checkIfTrueOrFalse(some parameter) ? true : false (result = major facepalm - even Elvis would wish himself dead at that one)

Asked a 'developer' as part of a code review to move the parts of the code that won't throw an exception outside the try-catch block I'm told that:

if(x != null){
blah
}

might cause a null so it should stay.

Also asked to make sure a declared variable was finalized, was asked what?

Aargh!!!!

Said developer is considered (by the company) and considers themselves senior.

devlinio · 03-05-2019 7:01pm

No code to share, but in my last job we had a "Dashboard" controller that was 3500 lines. It controlled nearly everything on the user side. It was insane.

By the time me and my manager finished refactoring it, we reduced it to 300/400 lines. Everything was reconfigured to spit out JSON. It was very nice to read at the end.

Capt'n Midnight · 03-05-2019 11:38pm

^^^^
Reminded me of

Sephiroth_dude · 04-05-2019 4:11pm

Would the following be considered a "Coding Horror"

public void setFirstName(String firstName){
	  
	  if(firstName.length() < 5){
		 
		  throw new IllegalArgumentException("NAME LENGTH IS TOO SHORT! ");
	  }
	  this.firstName = firstName;
  }

Rulmeq · 04-05-2019 4:53pm

Sephiroth_dude wrote: »

Would the following be considered a "Coding Horror"

public void setFirstName(String firstName){
      
      if(firstName.length() < 5){
         
          throw new IllegalArgumentException("NAME LENGTH IS TOO SHORT! ");
      }
      this.firstName = firstName;
  }

I worked with a guy who was tasked to code something similar, his first name was 2 letters (Wu), and I think they had the min set to 3 or 4. He wrote the code, because "it was a requirement" but the stupid really burns sometimes.

Coding Horror

Comments