Things you can do, but probably shouldn't

Just hit upon this thought - a listing off of things programming languages allow for that should be avoided since they make debugging harder. Should make for a lively discussion as opinions will differ.

For example, most (or at least I) consider it bad practice to go braceless with an if statement in PHP, Java, JavaScript, C and similar languages…


if ( condition )
  statement;

Braces just make it clearer.


if ( condition ) {
  statement;
}

But even I will make an exception if I’m returning off the condition.


if ( condition ) return value;

NextStatement;

Since here its a little less ambiguous on how the code is moving.

PHP Also allows variable variable names.


$a = 'foo';
$$a = 'bar';
echo $foo; // bar;

I have never figured out what that’s supposed to be useful for though and have avoided it like the plague.

Thoughts?

One of my “pet peeves” is PHP code that has @ error suppression.
Seems any situation where a FAIL might be expected to happen from time to time could be dealt with by using error/exception handlers.

Oh yeah. Forgot about that operator because I never use it. Whenever I see it I take an aspirin - whatever code has it is sure to be bad.

Declaring everything in an object public or static.

It is useful for extracting values out of an associative array into variables having the key names.

while (list ($key, $val) = each ($anArray) {
    $$key = $val;
}

Of course you’d have a lot more code in the loop to actually be useful since moving values between fields without doing something with it first is pointless.

This thread is pretty funny, and similar to this:

http://www.reddit.com/r/PHP/comments/1lpgqk/worst_practices/

Or you could just do:

extract($anArray);

But that doesn’t make provision for all the validation code I omitted from the version I posted - where I indicated that just running that as it is would be pointless but that it does provide a starting platfoem for adding common validation to all the fields.

Perhaps I ought to have posted it as:

while (list ($key, $val) = each ($anArray) {
    $$key = validate($val);
}

Simply copying fields from one name to another is completely pointless as you may as well reference them by theori original names. The normal reason for moving them to a new field name is to indicate that something has changed - for example $_POST variables are tainted but if you validate them and then move them either to separate field names or a separate array then you know that the new field names or array are untainted.

It’s actually useful for template rendering. You have a function which takes an array of data which is extracted to the local scope. When you require the template file it has access to a limited scope containing the data you passed in:


// Taken from http://symfony.com/doc/current/book/from_flat_php_to_symfony2.html
function render_template($path, array $args)
{
    extract($args);
    ob_start();
    require $path;
    $html = ob_get_clean();

    return $html;
}

But that doesn’t need any $$variable names - whereas I was giving an example of where you might use one.

… but you shouldn’t be doing that in the first place. That’s the point of this thread. Your brain shouldn’t work that way. You should say “oh I should use a variable variable name here”. There are few things in programming that are all encompassing, but this is one of them: If you ever think that it’s a good idea to use a variable variable while coding, it’s not and you’re wrong.

It’s such horrible practice and almost instantly leads to unreadable and unmanageable code. It’s almost as if you want to purposely introduce potentially critical and untraceable bugs. It’s just horrific code.

Variable variables don’t exist in any other language that I know of and for good reason. There are other ways to achieve the same results, but in more logical and more manageable ways.

Couldn’t you do that in two steps?

$array = array_map('validate', $anArray);
extract($array);

Granted, I understand that causes two loops on the array, but it is doable without $$ and the performance hit would be negligible…

That’s not true. JavaScript has them as well but implements them a different way.

That would only work if ALL of the entries are using the same validation function. Unless all of your variables are the same type that wouldn’t be the case.

Of course the better solution is to use a $untaintedArray instead of individual variables but if the current code already uses individual variables it may not be something that can be corrected immediately.

I am not suggesting that you should consider using $$variable - just presenting a situation where it may be the simplest way to achieve a particular result without having to rewrite an entire 10000 line script.

What? I looked at extract and array_map again, and I don’t see where the values would all have to be the same type… array_map receives an array, passes the elements to validate() and then returns an array containing the validated values… plus your loop would have this same problem…

Extract doesn’t seem to care if they are all the same type either…

Can you explain your statement? As I’m not seeing it (and it may be due to me being up late…)

So how do you pass fields of type email address to validateEmail, fields of type name to validateName, fields of type phone number to validatePhone using that code - just as three examples of different field types that would need to be validated differently.

The sort of situation I am thinking of where the code I posted might possibly be useful is if you have several dozen fields and just three or four different validation functions where you’d save a significant amount of code by not having a separate validate call for each individual field.

I’m sure I could think of a few ways :wink: The validate function could likely handle most of those scenarios. First would be using the $key name.

$array = array_map('validate', array_keys($anArray), $anArray); // now I have two inputs into validate($key, $val) :)
extract($array);  

So your code example would actually have a switch statement (or an if-elseif) that would call different validate functions (instead of them all calling the same one)…

No it doesn’t. It has objects. Which are probably more like PHP’s associative arrays than Variable Variables.

At least in Java, there is a book called Effective Java that shows all kinds of things we shouldn’t do. I think it applies to all OO programming. For example,

User user = new User(‘firstName’, ‘lastName’);

you can guess that above statement creates new instance of a User with first name and last name.

Unfortunately in the real world, you’ll see

SomeReallyLongNameClassYouDoNotKnow x = new SomeReallyLongNameClassYouDoNotKnow(1, ‘something’, ‘what?’, true, 0, 0,1, ‘Ok’, Status.FAIL);

It’s hard to understand what those arguments are set to which parameters.

So, creating a instance through contructor is now considered “bad practice”. Instead, they recommend

SomeReallyLongNameClassYouDoNotKnow x = new SomeReallyLongNameClassYouDoNotKnow();
x.setStatus(Status.FAIL);
x.setMessage(‘what?’);
x.showMessage(true);
…etc…

This way, you know what each parameters are set to more easily.

This is just one of MANY that’s very controversial.

I can agree with a bit of that. When you have more than X number of constructor parameters, it does make it difficult to read/interpret. There are ways of resolving that, but it definitely is a personal preference.

Even for functions/methods, I try not to make them contain too many parameters. I usually find that if I need to have more than X parameters, there is usually a better approach to the problem, one that is more readable and easier to interpret, and usually fits better into OO.

Using settings objects can help parameter bloat a lot alot.

I view constructor parameters as “this is a required dependency for this class to operate” whereas using property injection is for more optional things and/or things that can have a default implementation but allow users to swap them as needed.

Javascript has prototypes. It is really probably closer to lisp than php or java once you get past the syntax.

Also if anyone at sitepoint is reading this that sage add is horribly annoying – the rollover fires off and pops a modal video on the page. It is right in the middle of the page so doing just about anything fires off the rollover.