Bad Programming - Falling in Love with your own intelligence

PHP
1

Consider the following block of code from the program I’m paid to maintain (and which is slowly causing me to lose all my hair).


public function saveFromForm(array $values = array(), $id = null, $issue = true) {
	$a = $this->formFieldsToModel($values, $id);
	$a->has_changes = true;
	$issue and $a->issue(false);
}

I snipped some unimportant lines for purposes of this rant. Note the last line in the example, “$issue and $a->issue(false);” :nono:

I call this sort of coding many bad names in my less than charitable moods, but what it boils down to is it smacks of conceit, arrogance, and shows that the author was clearly in love with his own intelligence. This is a problem I see from time to time and I used to catch myself doing it in the past, though it’s less often now.

Code like this technically works - if you can’t see how it works look up lazy evaluation. And there is a time and place for lazy evaluation. This isn’t it. The line is no better than the intuitive


if ($issue) {
  $a->issue(false);
}

All it does is increase the difficulty in parsing the code ever so slightly for no real reason except to ‘show off’ to whoever reads the code. Considering the author of this code consistently makes tyro level mistakes throughout his code and its design I’m not impressed - if anything the showing off like this lowers my opinion of him.

Programming languages have a lot of room for this nonsense. The worst example is nested ternaries.


$a = $b ? $b : $c ? $c : $d ? $d : null;

If $b is 0, $c is 1 and $d is 4 what is $a? Most of us need to stop a moment and think about that. Such code is painfully difficult to debug and mistakes are far too easy


$a = $b ? $c ? $d ? $b : $c : $d : null;

Again, if $b is 0, $c is 1 and $d is 4 what is $a? Everyone really needs to stop a moment and parse the second one out. But given how similar the two are it is easy to mistake one for the other. The chances of one morphing to the other through typo are quite high.

Assignment in conditional is another case of this, though more into the gray area.


if ($a = foo()) {
  // code when foo returns true or anything that evals to true.
} else {
  // code when foo returns false.
}

Personally I will do this when catching function returns, more likely as an escape when the function returns false.


if ( !$a = foo() ) {
 $a = $default;
}

But on the whole, it’s something to avoid because it isn’t clear. There’s something to be said for coding clarity.

Cause clear code will impress the writer’s intelligence to me far more than the tomfoolery of unnecessary lazy evaluations, nested ternaries and other jerk moves which only serve to confuse whoever has to read it.

2

I agree. I think some PHP coders are under the impression that if they stick as much as possible on one line, such as with your nested ternary example, that the code will execute faster. That isn’t necessarily the case.

When writing any type of code, I believe it should be written in as simple of a manner as possible. Not for the benefit of the writer, but for those who are going to read, modify, and maintain the code later. Some coders think that since they understand what the code is doing, you should be able to understand it, too, without any comments explaining what is going on. If code is written simply, it’s pretty easy to understand what is going on. But in cases such as the example you gave, it isn’t easy and a lack of explanation makes it even more difficult. I remember arguing this type of stuff with the coders of SMF years ago.

3

I do admit that I like to condense things onto one line; ie: (totally making this example up, btw)


$jar = end(explode('<p>',current(explode('</p>',$subject))));

Though I will say I only do it when i consider the line an ‘atomic operation’. I could have done each command as a separate line, but it wouldn’t have made the intent any clearer.

4

Michael, your last example is kind of standard programming I believe?
I completely agree with you on the other cases.

5

When I read your original example method saveFromForm() it makes me wonder how many lines of JavaDoc comments preceded it? Were there any?

6

I’m definitely guilty of this time to time. It normal happens after many hours of ongoing programming when trying to save space, variables or the infamous thought of refactoring it later due to time constraints.

7

I’m all for condensing code, but within reason. And there’s the rub. Who decides what’s “within reason”? :slight_smile:

For example, I will typically do this


function testThis(val){                 
                var str = (/^[\\da-z]{7,15}$/.test(val.toLowerCase()))? 'valid' : 'invalid';                 
                alert('username is '+ str);             
            }

rather than


function testThis(val){
                val = val.toLowerCase();
                var regex = /^[\\da-z]{7,15}$/;                 
                var str = (regex.test(val))? 'valid' : 'invalid';                 
                alert('username is '+ str);             
            }

For anyone who understands javascript reasonably well, the first function’s code should be clear. In the second function I see no need at all for taking up a bit more RAM to actually store the regex. In itself, the amount of RAM is trivial, but in a large application all the extra bits of RAM all add up.

My pet hate is people using single letter variables, function names which don’t give even a hint of the function’s purpose and little or no comments throughout the code.

8

Python prohibits it outright, and I don’t remember it being possible in BASIC either, but it’s been forever since I dealt with basic. So when entire languages do not permit the action it’s not safe to say its standard practice. I do it too, but only rarely and usually only in while statements.

No, there were not. Before I arrived there where no comments in most of the code at all. Functions have names like “doSomething”. It’s a ****ing mess.

As you point out there’s reasons for doing that. And personally I can build some very long dot syntax sentences myself. That isn’t the same thing as a programmer who cries out “Hey, I understand lazy eval herp de dur dur de dur” and then proceeds to do stupid crap like create functions with meaningless names – this program had an actual foo function defined - I kid you not. And it was used as an assert statement because the moron who wrote it couldn’t be bothered to read up on what assert does. “Had” because I was so ticked off at the existence of this function I spent three days on a find/replace spree).

My pet hate is people using single letter variables, function names which don’t give even a hint of the function’s purpose and little or no comments throughout the code.

So you wouldn’t like to see function doSomething() defined either I take it?

I do use a few single letter variables, but only in specific recurring cases.

i -> for incrementor
j -> inner loop incrementor
e -> HTML element object in javascript.
ev -> HTML event object.
x, y, z -> coordinate functions will use these vars for horizontal, vertical and depth respectively.

These are all so frequently used that that no one should be surprised by them - I’m certainly not the only one that follows this convention.

9

yeah ok :rolleyes:, I would have thought the above goes without saying. I also use the above single char vars. for those purposes.

What I was referring to is someone who wants to calculate the area of a circle (and this is a very simplistic example) doing something like

$r = 3.45;

$g = 3.1415926 * (2*$r)*(2*$r)/4;
10

I meant in PHP. I’ve seen it a lot. Although, of course, the fact that it’s being done doesn’t mean it’s right. But at least it’s a “fixed” construction (meaning that it always follows the same logic), so once you know it, it’s not hard to understand at all. While all the others require effort to understand what they actually do every time you encounter them (although I didn’t so far, fortunately :wink: ).

11

The above clarification was not meant as an insult to your intelligence, but a clarification for the less experienced readers of the board.

Anyway, outside of convential 1 letter names, programmers who habitually use them can be quite annoying.

Or jerks who exploit the fact that constant and function names in PHP aren’t case sensitive, then screw with your head by playing with that trait. Especially since (and I never figured out why the PHP devs thought this was a good idea) array keys and I think array names ARE case sensitive.

12

So what’s the issue here :confused2? If you know constant and function names aren’t case sensitive I don’t see what difference it can make to anything. Personally I type function names the same way every time but if someone chooses not to, it’s not an issue at all for me because I know it doesn’t make any difference to anything.

It’s certainly not worth a :bouncy3: and :bawling: over in a forum thread :lol:. There are much more important things going on in the world atm for me to be :bouncy3: about :slight_smile:

13

I’ve always coded from the viewpoint of making sure that the person coming in 6 months later (whether that’s me or not) will be able to tell pretty quickly what the codes supposed to do. I don’t know how many times over the years I’ve found situations where a 1/2 hour fix took a day because it took that long to figure out what the heck the code was supposed to do (and that’s if I don’t refactor the code immediately)

some other “gems” I’ve hit…

  • one developer named all their variables after song titles of their favorite music artist at the time
  • an api layer that was a 1:1 match to a commercial api below it. no overrides, just a direct call to the commercial api.
  • a consulting firm whose idea of documentation was a javadoc run because their code was “self-documenting” - tell that to the customer the first time a change had to be made that they couldn’t make heads nor tails of the code base.
  • Same consulting firm who created whole extra widgets to generate standard form elements because they “didn’t like the way the calls were written.”
14

Whenever you come into a project that you didn’t build or participate in building there is always going to be that learning curve. While I have come across certain things I don’t agree with in the end it always take so much time to figure out what the hell is going on because that is just the way it is. Unless you have built it there is going to be a significant learning curve either way given these “little” caveats or not. Surely thousands of layers of abstraction can result in the same feeling for the person who didn’t develop the project. Though I do think that people that name things without programmatic or contextual meaning should be taken out back and shot.