Coding Horror

Boards.ie: Neil

So I've just come across another horror.

[php]
$editorid = construct_edit_toolbar($pagetext, 0, $foruminfo, iif($foruminfo, 1, 0), 1);
[/php]

the iif is NOT a typo, and $foruminfo will ALWAYS be either a 1 or 0.

below is the code for iif (regulars here will remember Boards.ie: Paddy posting it before).

[php]
/**
* Essentially a wrapper for the ternary operator.
*
* @deprecated Deprecated as of 3.5. Use the ternary operator.
*
* @param string Expression to be evaluated
* @param mixed Return this if the expression evaluates to true
* @param mixed Return this if the expression evaluates to false
*
* @return mixed Either the second or third parameter of this function
*/
function iif($expression, $returntrue, $returnfalse = '')
{
return ($expression ? $returntrue : $returnfalse);
}
[/php]

Colonel Panic

I see that so often, there should be a design (anti)pattern named in it's honour.

ciarank1

i have only been doing java for a year or so now, but my god what i hate is INFINITE LOOPS... they are just terrible, but, one day i learned how to properly code an infinite 'for' loop, so i know how to fix them now, but not other loops

Feathers

Cork24 wrote: »

Some really funny comments

Some of these are brilliant

Cork24 wrote: »

Thats where Commments come into play, every program i do, i have about 3 lines of Comment over every Method/Function telling what its doing and where its pointing to if i was using Pointers or Abstract Coding in Java.

Surely you mean why it's doing something, as opposed to what it's doing? I hope that any developer working on my project, past or present, should be able to look at a method & read what is going on. (just programming in Java mainly at the minute).

If the method is too big to follow, I'd rather break constituent parts into their own private methods (that can then be reused) rather than putting in a lot of comments.

As in, the developer shouldn't get lost in the code & need comments to find their way. I'd use comments mostly for stuff like, if I was deprecating a method — why & what has replaced it; this is something that you can't find out just from reading the code underneath.

Trying to put tests around some legacy code at the moment. Have gotten a few if "true then true"s alright, as well as some comments along the lines of:

//Not really sure why this isn't working properly — it's bizarre

How helpful! :rolleyes:

zynaps

Feathers wrote: »

Trying to put tests around some legacy code at the moment. Have gotten a few if "true then true"s alright, as well as some comments along the lines of:

//Not really sure why this isn't working properly — it's bizarre

How helpful! :rolleyes:

Well, it's at least helpful as a hint that you should write those unit tests to investigate the issue and figure out what the correct behaviour should be and why it's behaving differently now

And my favourite pet peeve, having been bitten a few times (once by my own doing, more recently a colleague - both of us should know better):

try {
  byte[] result = ... several lines of messy network code ...;
  return result;
} catch(Exception e) {
}
return new byte[1];

I was like "okay, it must be doing something because it's returning an actual array rather than null and not throwing any exceptions or printing an error... but why is the array only 1 element long, rather than at least several hundred bytes?" then 10 minutes later "....oh no you didn't leave an empty catch block and return a useless arbitrarily 1-byte-long array? ...KHAAAAAN!!!" :pac:

BrokenArrows

Was making a few small changes to some code today. I was shocked to see a very large amount of if/else conditions in the following format.

If x == y
// Do nothing
Else
// Do stuff

The "// Do nothing" comment was actually in the code
Needless to say I changed them all.

Funnily enough the person who wrote the code is now the director of development.

cython

Just came across this python doozy today that has wasted some of my time already. I found myself extending a script that we use for branch management, and needed to add some special case handling for specific files. All well and good you would think. Except that because someone had used

lines = mrg_output.split("\n")

instead of the platform-independent

lines = mrg_output.splitlines()

and I was running on Windows, the subsequent test I wanted to do using an endswith() failed because there was still a bloody '\r' in the returned string :mad::mad::mad: Surely it's better to do this, than to assume someone (i.e. me) will check

string.endswith('bla') or string.endswith('bla\r')

ChRoMe

zynaps wrote: »

oh no you didn't leave an empty catch block

People have died for less!

COYW

zynaps wrote: »

oh no you didn't leave an empty catch block

Love it! I've seen them before with the comments "I'll fill these in later".

On commenting, I came across a method a few weeks ago that had a substring call in it which removed the last 2 characters in a string parameter with the comment "do not remove this line, it won't work".

ciaranmac

Feathers wrote: »

Surely you mean why it's doing something, as opposed to what it's doing? I hope that any developer working on my project, past or present, should be able to look at a method & read what is going on.

You'd hope that would be the case. Where comments sometimes come in useful is when the code doesn't do what the original developer expected it to. The comments say what it's meant to do, a tight reading of the code tells you it does something different, and resolving the problem is easier than it would be without the comment.

Feathers

ciaranmac wrote: »

You'd hope that would be the case. Where comments sometimes come in useful is when the code doesn't do what the original developer expected it to. The comments say what it's meant to do, a tight reading of the code tells you it does something different, and resolving the problem is easier than it would be without the comment.

So the person who added the comment knows it does something different to intended - why not refactor the code (e.g. renaming the method to what it actually does) rather than commenting it?

Earthhorse

Or what if they wrote the comment wrong too? What if the comment says "This method returns all clients" whereas the code returns all current clients? Which one is correct?

conor.hogan.2

Earthhorse wrote: »

Or what if they wrote the comment wrong too? What if the comment says "This method returns all clients" whereas the code returns all current clients? Which one is correct?

Whatever is in the spec (whatever or wherever the spec is).

Earthhorse

conor.hogan.2 wrote: »

Whatever is in the spec (whatever or wherever the spec is).

Of course. The point is that the comment can be wrong too; in fact, it might be more likely to be wrong than the code because there won't be any tests run against it.

GreeBo

Earthhorse wrote: »

Of course. The point is that the comment can be wrong too; in fact, it might be more likely to be wrong than the code because there won't be any tests run against it.

If the code is wrong then the test is also wrong by definition, otherwise the test would fail.

Id rather take the risk of having the odd wrong comment than no comments.

Feathers

GreeBo wrote: »

If the code is wrong then the test is also wrong by definition, otherwise the test would fail.

Id rather take the risk of having the odd wrong comment than no comments.

It depends on what we mean by wrong — that it doesn't match the spec (so it's a bug), or that it does match the spec but the spec doesn't cover a case needed by business requirements — the current tests might but right, but incomplete.

Either way, I'd say that as a general rule, comments shouldn't be able to be wrong — if so, you're commenting the type of thing that could instead by held in a variable or method name.

zynaps

GreeBo wrote: »

If the code is wrong then the test is also wrong by definition, otherwise the test would fail.

True, but one common source of "buggy comments" is when the behaviour of other code changes for some valid reason, and a once-accurate comment becomes outdated and misleading. There will be no warning when this happens, because comments are freeform and not validated in any way... but as you point out, the tests should fail (assuming they are properly specified), quickly flagging the need to update the tests to reflect a new reality - which might reveal bugs in the new code, and so forth.

Plus, since you have to specify your tests in a systematic and unambiguous way (code!), they are of course a better descriptor of expected behaviour than comments. That's not to say that I don't use comments to explain parts of the code that may look unnecessary or confusing that I haven't yet or couldn't refactor!

Earthhorse

GreeBo wrote: »

If the code is wrong then the test is also wrong by definition, otherwise the test would fail.

Id rather take the risk of having the odd wrong comment than no comments.

Ah here now, who said anything about "no comments"? Not I.

All I'm saying is that it's fallacious to think that there where the comment and the code disagree the comment is more likely to be correct because it's "easier" to convey your intent in English.

Ludo

Ah lads...this is the coding horror thread..there has been a big discussion about the merits (or lack thereof) of comments here already recently...
http://www.boards.ie/vbulletin/showthread.php?t=2056835525

c_man

Came across one that slipped under the radar in a dusty part of the code base. Essentially this

void obj::send(std::string str)
{
   if(!isStarted())
   {
      send(str);
   }
   else
   {
       mem->passString(str);
   }
}

Trojan

This... works.

<form method="post" action="<?php the_permalink(); ?>">
	<table><tbody><tr>
		<?php
			$current_user = wp_get_current_user();
			$user_video_pref = get_user_meta($current_user->ID, 'namespace_video_size_pref', true);
			$arr = array("960x540","720x406","320x180", "Auto");
			echo "\n";
			foreach ($arr as $value) {
				echo "\n<td id='td-".$value;
				echo "' onMouseOver=\"this.style.backgroundColor='#f9f9f9'\"; ";
				echo "  onMouseOut=\"this.style.backgroundColor='#DCDCDC'; if (document.getElementById('namespace_video_size_pref".$value."').checked) {document.getElementById('td-960x540').style.backgroundColor='#DCDCDC';document.getElementById('td-720x406').style.backgroundColor='#DCDCDC';document.getElementById('td-320x180').style.backgroundColor='#DCDCDC';document.getElementById('td-Auto').style.backgroundColor='#DCDCDC';this.style.backgroundColor='#f9f9f9'; }\" ";
				echo "  onClick=\"this.style.backgroundColor='#DCDCDC'; if (document.getElementById('namespace_video_size_pref".$value."').checked) {document.getElementById('td-960x540').style.backgroundColor='#DCDCDC';document.getElementById('td-720x406').style.backgroundColor='#DCDCDC';document.getElementById('td-320x180').style.backgroundColor='#DCDCDC';document.getElementById('td-Auto').style.backgroundColor='#DCDCDC';this.style.backgroundColor='#f9f9f9'; }\" ";
				echo " style='padding:0; border:0;";
				if ($value==$user_video_pref) echo " background-color:#f9f9f9; border: 2px solid #444' class='namespace-selected-video-option";
				echo "'>\n<label for='namespace_video_size_pref".$value."'>";

				if ($value=="960x540") 	echo "<h3 style='margin:0 0 0 45px;'>Large</h3>";
				if ($value=="720x406") 	echo "<h3 style='margin:0 0 0 45px;'>Medium</h3>";
				if ($value=="320x180") 	echo "<h3 style='margin:0 0 0 45px;'>Small</h3>";
				if ($value=="Auto") 	echo "<h3 style='margin:0 0 0 45px;'>Auto</h3>";

				echo '<img src="http://images.namespace.com/';
				echo $value.'.gif" alt="Choose image size" style="border:1px solid #585F6B;" /> <br/>';
				echo '<input type="radio" style="padding:10px; margin: 5px 0 0 70px " name="namespace_video_size_pref" id="namespace_video_size_pref'.$value.'" value="'.$value.'"';
				if ($value==$user_video_pref) echo " checked ";
				echo "><br />&nbsp; <br /></label>";
				echo "\n</td>\n";
			}
		?>
	</tr></tbody></table>
	<input name="updateuser" type="submit" id="updateuser" class="submit button" value="Update" />
	<input name="action" type="hidden" id="action" value="update-user" />
</form>

GreeBo

^ ouch my eyes!

Im surprised the whole thing isnt wrapped in a string and eval'd.
Thats like inception or something.

Trojan

GreeBo wrote: »

^ ouch my eyes!

Im surprised the whole thing isnt wrapped in a string and eval'd.
Thats like inception or something.

Worse, I wrote it.

zynaps

c_man wrote: »

void obj::[B]send[/B](std::string str)
{
   if(!isStarted())
   {
      [B]send(str);[/B]
   }
   else
   {
       mem->passString(str);
   }
}

Am I missing something that makes this not an infinite recursion/stackdeath if isStarted() doesn't return true (quickly)?

c_man

zynaps wrote: »

Am I missing something that makes this not an infinite recursion/stackdeath if isStarted() doesn't return true (quickly)?

Nope, you're right it was a potential infinite loop. Luckily for us up til now it always was bloody 'started' :P

Triangle

If X = Y then Y = X
.......

The sad thing is people paid for this logic.

PrzemoF

Triangle wrote: »

If X = Y then Y = X
.......

The sad thing is people paid for this logic.

I can't believe that a sane programmer would do it deliberately. Maybe that's a "pay-by-line-of-code" case? I heard a story about a drafters splitting lines in autocad, because they were paid by the number of entities on the drawing...

Tar.Aldarion

Haha love the comments in the source code
http://www.reddit.com/r/programming/comments/1bnezw/jedi_outcastjedi_academy_source_code_released/

And of course this beauty:

/*
** float q_rsqrt( float number )
*/
float Q_rsqrt( float number )
{
    long i;
    float x2, y;
    const float threehalfs = 1.5F;

    x2 = number * 0.5F;
    y  = number;
    i  = * ( long * ) &y;                       // evil floating point bit level hacking
    i  = 0x5f3759df - ( i >> 1 );               // what the ****?
    y  = * ( float * ) &i;
    y  = y * ( threehalfs - ( x2 * y * y ) );   // 1st iteration
//  y  = y * ( threehalfs - ( x2 * y * y ) );   // 2nd iteration, this can be removed

    return y;
}

Colonel Panic

Do you know the history of that piece of code? It's an incredibly fast square root approximation i the Quake 3 engine. I wouldn't call it a coding horror!!!

http://en.wikipedia.org/wiki/Fast_inverse_square_root

Tar.Aldarion

I do know, I was just saying I liked that I saw it used (the games are on the quake engine)