5

This isn't a rare case and occurs with me often and I spent countless time trying to debug the code.

Latest example, this PHP code where I used $email for the parameter and for object as well.

private function _mail( $email )
{
    if( filter_var( $email, FILTER_VALIDATE_EMAIL ) ):
        $email = new Zend_Mail();
        $email->setSubject( $this->request->post('subject') )
            ->setBodyHtml( $this->request->post('message') )
            ->setFrom( $this->request->post('from') )
            ->addTo( $email )
            ->send();
        unset($email);
    endif;
}

It was throwing an error, strtr() expects parameter 1 to be an string object given and it was really frustrating to browse through zend libraries to see what obscure string statement was throwing this error.

How to avoid such mistakes?

8
  • 6
    someone will suggest hungarian I can feel it Commented Jan 15, 2013 at 14:12
  • 2
    What IDE do you use? Static analysis should be able to pick up on these.
    – StuperUser
    Commented Jan 15, 2013 at 14:13
  • 1
    @StuperUser: I use PHPStorm. It does detect immediate assignment fallacies but sometimes it doesn't.
    – Shubham
    Commented Jan 15, 2013 at 14:22
  • 4
    That's the first time in my life that I ever seen endif used like that!! I mean yeah that was popular when PHP and HTML were mixed together, but endif in a function, that's a first.
    – Songo
    Commented Jan 15, 2013 at 15:13
  • 1
    Honestly, all the 3 answers so far suck. Variable naming doesn't solve this problem in general and puts the blame on the wrong place. Isn't there any linter out there that gives a warning if you assign to a function parameter?
    – hugomg
    Commented Jan 16, 2013 at 2:15

5 Answers 5

13

One good aspect of modern programming languages is that you can have large variable names. In Object oriented programming, which you are utilising, Java conventions are quite popular.

So it would be better to have a descriptive name for the Zend_Mail object, such as

$zendMail = new Zend_Mail();

In the beginning it will consume a bit more time until the brain shifts making it a habit, but that time will have a huge return of investment and stress from having to debug similar cases.

2
  • 8
    In addition, change the parameter name to something like $toAddress. $email could mean so many things.
    – Misko
    Commented Jan 15, 2013 at 17:02
  • 1
    This is a very general solution: if your variable names describe exactly what they contain, the chance of collisions is very low. Commented Jan 17, 2013 at 8:47
5

I think that you have two different concept here. You have the email address of a recipient and an email message that will be sent to the recipient. So why not :

private function _mail( $recipient )
{
    if( filter_var( $recipient, FILTER_VALIDATE_EMAIL ) ):
        $mail = new Zend_Mail();
        $mail->setSubject( $this->request->post('subject') )
            ->setBodyHtml( $this->request->post('message') )
            ->setFrom( $this->request->post('from') )
            ->addTo( $recipient )
            ->send();
    endif;
}
2

This isn't a rare case and occurs with me often and I spent countless time trying to debug the code.

Just that much tells me two things:

  1. If it hurts, stop doing it. You say that this problem "occurs with me often," which means that you need to make a greater effort to avoid this kind of problem. Don't try to use two variables with the same name, ever, even when you're sure that it'll work. There are two possibilities: a) you're right and it works, but your code is a lot more difficult to understand than it should be; or b) you're wrong and, well, you've seen what happens when you're wrong.

  2. Learn from experience. If you keep writing the same kind of bug, you should by now be pretty darn good at recognizing the symptoms and finding the problem. Knowing that you have a propensity for creating a certain class of bug, you should constantly be on the lookout for indications that you've done it yet again. Use assertions to check that a variable contains the type of data that you expect. Question your assumptions. Write some unit tests to test individual components.

0

In PHP I prefix function/method paramaters with 'in' - and don't use any variable starting with 'in' within the body. It's not ideal, but it works.

Dimitris point about more descriptive names is also helpful.

-1

My IDE does that for me. Eclipse has "templates" for code-autocomplete (STRG+Space) I told my Eclipse to always add a prefix to method parameters.

For example your method:

private function _mail( $recipient )

would automatically be auto-completed with $pRecipient.

Short Rule: Always prefix method parameters, to avoid variable clashes inside the method body.

sidenote: think about using interfaces instead ob objects as method parameters.

1
  • I knew someone would suggest hungarian ;) Commented Mar 1, 2013 at 12:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.