Coding Horror

Slutmonkey57b

not1but4 wrote: »

At the risk of asking a very stupid question what is the issue with this?

Is it the case if you where to use a 16 bit integer (range being -32768 to + 32767)

You'd get the following;
abs(-32767) = 32767
abs(-32768) = -32768

EDIT:
What would be the correct implementation of this then?

It's a waste of time. If a negative value return is possible, and you don't consider it an error, then you should just always abs() the original value returned and carry on.

But fundamentally one should be suspicious that a negative value is treated the same as its positive counterpart. They're different values for a valid reason so either you're missing an error or the calculation which returned somevar is set up incorrectly.

Aswerty

Slutmonkey57b wrote: »

But fundamentally one should be suspicious that a negative value is treated the same as its positive counterpart. They're different values for a valid reason so either you're missing an error or the calculation which returned somevar is set up incorrectly.

But there is plenty of times where the reason for the differing signs doesn't matter with regards to what you are calculating. That's why the abs() function exists in the first place. So I don't think it is reasonable to consider ignoring the sign as fundamentally suspicious.

croo

I read this a while back and never noticed it mentioned .. anywhere never mind this thread.

This code came from the installation script for Steam for Linux

# figure out the absolute path to the script being run a bit
# non-obvious, the ${0%/*} pulls the path out of $0, cd's into the
# specified directory, then uses $PWD to figure out where that
# directory lives - and all this in a subshell, so we don't affect
# $PWD
STEAMROOT="$(cd "${0%/*}" && echo $PWD)"

# Scary!
rm -rf "$STEAMROOT/"*

Of course the flaw is obvious. Even to the developer given their "Scary!" comment... still it is very easy to install Linux isn't it?

Full story was on the register a few months back.

Slutmonkey57b

Aswerty wrote: »

But there is plenty of times where the reason for the differing signs doesn't matter with regards to what you are calculating. That's why the abs() function exists in the first place. So I don't think it is reasonable to consider ignoring the sign as fundamentally suspicious.

True, but you'd need the context of the original code to be sure. Without it, my first question to the dev would be "why?"

Idleater

croo wrote: »

I read this a while back and never noticed it mentioned .. anywhere never mind this thread.

This code came from the installation script for Steam for Linux

# figure out the absolute path to the script being run a bit
# non-obvious, the ${0%/*} pulls the path out of $0, cd's into the
# specified directory, then uses $PWD to figure out where that
# directory lives - and all this in a subshell, so we don't affect
# $PWD
STEAMROOT="$(cd "${0%/*}" && echo $PWD)"

# Scary!
rm -rf "$STEAMROOT/"*

Of course the flaw is obvious. Even to the developer given their "Scary!" comment... still it is very easy to install Linux isn't it?

Full story was on the register a few months back.

Related rhel bug ?https://bugzilla.redhat.com/show_bug.cgi?id=1202858

Had cause to Google Heisenbug this week, and duly found Hindenbug

croo

Idleater wrote: »

Related rhel bug ?https://bugzilla.redhat.com/show_bug.cgi?id=1202858

Had cause to Google Heisenbug this week, and duly found Hindenbug

Wow... steam I might forgive but RHEL & squid? That must be an extremely common combination. Amazing!

Hindenbug

That's a new one for me too.

Pov06

not1but4 wrote: »

At the risk of asking a very stupid question what is the issue with this?

Is it the case if you where to use a 16 bit integer (range being -32768 to + 32767)

You'd get the following;
abs(-32767) = 32767
abs(-32768) = -32768

EDIT:
What would be the correct implementation of this then?

Just:

another_var -= abs(some_var);

Absolute value will always return a positive number no matter whether you supply a positive or negative.

EDIT: Didn't notice it was answered already.

tobsey

I've just been tasked with fixing bugs in a solution that was previously looked after by one of my colleagues who was let go a couple of months ago. The main page in the asp.net site has the following in the code-behind:

line 896: protected void buttonGo_Click(object sender, EventArgs e)
            {
                  // Loads of validation code

line 994:      if (myDropdown.SelectedValue = MyValues.Value1.ToString())
                  {
                         // set a load of values in a repository
                         // trigger a change in state
                  }
line 1291:     else if (myDropdown.SelectedValue = MyValues.Value2.ToString())
                  {
                         // set a load of the same values in a repository
                         // trigger a different change in state
                  }
line 1306:     else if (myDropdown.SelectedValue = MyValues.Value3.ToString())
                  {
                         // set a load of the same values in a repository
                         // trigger a different change in state
                  }
line 1321:     else if (myDropdown.SelectedValue = MyValues.Value4.ToString())
                  {
                         // set a load of the same values in a repository
                         // trigger a different change in state
                  }
line 1456:     else if (myDropdown.SelectedValue = MyValues.Value5.ToString())
                  {
                         // set a load of the same values in a repository
                         // trigger a different change in state
                  }

// six more of these else if statements

line 2153: }

L.Jenkins

Did he or she ever hear of switch/case? I could understand if they were working with 2/3 cases, but 11! I'm sure that some how you could condense that down even further/tidy it up.

Eugene Norman

Switch statements are syntax sugar. It would still be as long a function, and long functions are my bugbear. Switch statements often -- like this else if -- have huge amounts of code per statement that could be factored out. also I don't know asp.net but surely a method could be set for each menu item.

Edit. Oh, it looks like the same code bar some state setting is set in each if statement. I've seen the same in switch statements.

oscarBravo

Interestingly, Python doesn't have a switch statement; it has an if/elif/else construct similar to the above.

Tar.Aldarion

It's either easy or hard to code switch equivalency in python, it depends on whether you view fall through as a feature or a bug :pac:

tobsey

Eugene Norman wrote: »

Switch statements are syntax sugar. It would still be as long a function, and long functions are my bugbear. Switch statements often -- like this else if -- have huge amounts of code per statement that could be factored out. also I don't know asp.net but surely a method could be set for each menu item.

Edit. Oh, it looks like the same code bar some state setting is set in each if statement. I've seen the same in switch statements.

Bingo. DRY seems to have been completely lost on him.

Another favourite of mine in the codebase:

foreach (KeyValuePair<string, string> myData in SQLDal.GetSomeData())
            {
                if (myData.Key.ToString() == keyDropDown.SelectedValue)
                {
                    valueDropDown.SelectedValue = myData.Key;
                }
                else if (keyDropDown.SelectedValue.Trim() == "")
                {
                    valueDropDown.SelectedValue = " ";
                }
            }

For all records in DB
--> if a field matches record, set other field.
--> Else if field is blank, clear other field

So if 100 records are returned from the DB and none match, we will have checked a field on our form 100 times for an empty string.

Tar.Aldarion

http://thedailywtf.com/articles/The-Speedup-Loop

L.Jenkins

Tar.Aldarion wrote: »

http://thedailywtf.com/articles/The-Speedup-Loop

One innovative way to keep a job for yourself.

Korvanica

Tar.Aldarion wrote: »

http://thedailywtf.com/articles/The-Speedup-Loop

Our software just got slightly slower... until I come save the day.:cool:

SimonTemplar

    Our core system is written in a proprietary language developed by our vendor and used only by the vendor’s applications. I occasionally do some coding in it mostly for data extracts. Was writing a script today and it was failing (it doesn’t provide a line number or even a reason for the error, just a big fat fail). Spent over an hour trying to debug and couldn’t understand what was happening. Got support from the vendor – it turns out that the language is designed so that you can only set the value of a variable once. If you try to change a variable’s value, it will fail. What a pile of absolute crap!!

Calina

SimonTemplar wrote: »

    Our core system is written in a proprietary language developed by our vendor and used only by the vendor’s applications. I occasionally do some coding in it mostly for data extracts. Was writing a script today and it was failing (it doesn’t provide a line number or even a reason for the error, just a big fat fail). Spent over an hour trying to debug and couldn’t understand what was happening. Got support from the vendor – it turns out that the language is designed so that you can only set the value of a variable once. If you try to change a variable’s value, it will fail. What a pile of absolute crap!!

Surely that's not then a variable, but a constant, no?

SimonTemplar

Calina wrote: »

Surely that's not then a variable, but a constant, no?

If that's the case, this language only has constants and no variables.

jonster

Could be immutable variables ala functional programming languages?

The_B_Man

I've spent the last while working on an old app thats still used today. Mostly maintenance.
Any time I open the IDE, I'm met with the most ridiculous of ridiculous coding horror. Its like it was written by a first year.

For example, there were around 50 classes that were copy and pasted from the same class, with a few lines changed. I couldnt, in good conscience, let it go unfixed so spent ages moving the common logic into a base class. That cleared up a few thousand lines of code!

Then there's the stuff like "if (whateverBoolean == true)" instead of just "if (whateverBoolean)", and the random if() statements, with about 20 conditions in them, but no body!!

The more I work on it, the more I keep finding exact copy and paste problems. Like I'll find a problem, and have to go and fix it in about 20 files, minimum. Its atrocious and needs a complete rewrite tbh.

Oh and this isnt even taking into account the database logic. The norm would be to have one database manager class and every request goes through that, so it can properly schedule/queue the requests. The way this was written, any time theres a DB request, a new instance of the DB is created, and the request sent in whenever it feels like it. This throws up some interesting exceptions with the database being locked, and various other errors.

By far the worst code I've ever worked on, and the customer is a household name!! :eek::eek:

(I should point out that we inherited the code, we didnt write it! )

daymobrew

I have just inherited a WordPress/WooCommerce site. It is horrible.

The "offshore" team took the TwentyTwelve theme, copied the directory to "new" instead of making a child theme.
For WooCommerce they copied the entire WooCommerce 2.2.8 plugin into a 'woocommerce' subdirectory. This means that they copied all the override templates too and I have to go through them step by step to see which ones were actually customised and delete those that weren't.

In the functions.php they disabled update notifications.
[php]function remove_core_updates(){
global $wp_version;return(object) array('last_checked'=> time(),'version_checked'=> $wp_version,);
}
add_filter('pre_site_transient_update_core','remove_core_updates');
add_filter('pre_site_transient_update_plugins','remove_core_updates');
add_filter('pre_site_transient_update_themes','remove_core_updates');[/php]
I didn't know about this code until I installed InfiniteWP plugin and saw 9 plugin updates in the InfiniteWP dashboard.

80 errors when I ran the home page through W3C Validator (most were bad closing html comment i.e.

---

> instead of -->).

As I said, I have "just inherited" the site. I expect to find a lot more stuff.

BrokenArrows

A customer reported that their login screen kept closing when they tried to login. They said that the uploaded a new xml language file. This language file allows customers to define the text/language of certain buttons/labels etc.
They sent me the file and i couldnt see anything wrong with it.

Decided to open up the code to have a quick look.

private void buttonHOSTESS_Click(object sender, EventArgs e)
        {
            if( buttonHOSTESS.Text == "OK")
            {
                this.Dispose();
            }
            else
            {// Does the normal work here}
        }

The customer had changed the login button from Ok to OK.

jimmycrackcorm

BrokenArrows wrote:

Decided to open up the code to have a quick look. Code: private void buttonHOSTESS_Click(object sender, EventArgs e) { if( buttonHOSTESS.Text == "OK") { this.Dispose(); } else {// Does the normal work here} } The customer had changed the login button from Ok to OK.

I can't understand this thing about string comparison issues surrounding case sensitivity. It seems crazy from a human perception that we easily acknowledge OK = ok but we don't carry that assumption into our programming.

NeutralHandle

It would increase how much the computer has to do to compare strings significantly, and it would not always be desirable behaviour from a functional point of view.

If you want to do case-neutral string comparison just use something like this: string1.ToUpper().Equals(string2.ToUpper)

oscarBravo

The point that's being missed is that performing an action based on the text on a button - especially in a system that allows for localisation - is insanely, stupidly broken.

NeutralHandle

I didn't miss that; was just responding to the previous comment

BrokenArrows

oscarBravo wrote: »

The point that's being missed is that performing an action based on the text on a button - especially in a system that allows for localisation - is insanely, stupidly broken.

And also I have no idea why that was ever there. There is no reason to dispose the whole thing if the button was ok.

Maximus Alexander

BrokenArrows wrote: »

And also I have no idea why that was ever there. There is no reason to dispose the whole thing if the button was ok.

That's exactly what I was thinking. I'm sitting here wondering why nobody else mentioned that part, and thinking I'm a dummy who has missed something. :pac:

Creamy Goodness

believe it or not, there's programmers out there that have and will never come across localisation :eek: