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?

gnat
  • 21,442
  • 29
  • 112
  • 288
Shubham
  • 713
  • 7
  • 17
  • 6
    someone will suggest hungarian I can feel it – ratchet freak Jan 15 '13 at 14:12
  • 2
    What IDE do you use? Static analysis should be able to pick up on these. – StuperUser Jan 15 '13 at 14:13
  • 1
    @StuperUser: I use PHPStorm. It does detect immediate assignment fallacies but sometimes it doesn't. – Shubham Jan 15 '13 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 Jan 15 '13 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 Jan 16 '13 at 02:15
  • @Songo: Well, I feel much more comfortable with `endifs`. Don't know why. – Shubham Jan 16 '13 at 05:12
  • @missingno: Who needs legs when we have wheelchairs? Who needs proven principles of good software engineering when we have linters and debuggers? The names are far too general and tell absolutely nothing about the intention and the functionality -- even to the OP six weeks from having written it. What is `_mail($email)` doing? It's as good as `_do_something($with_this)` So what's wrong with `_send_mail($to_addr)`? – Secure Jan 16 '13 at 06:35
  • @missingno: Variable naming does solve the problem in general. If variable names are specific enough, you only use/assign those variables intentionally. You don't accidentally write `$to_addr = new Zend_Mail()`, and if you do it's easy to recognize the problem. The only reason to assign `$to_addr` is if you actually want to change the recipient's address. – Misko Jan 16 '13 at 16:36

5 Answers5

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.

Dimitrios Mistriotis
  • 2,220
  • 1
  • 16
  • 26
  • 8
    In addition, change the parameter name to something like $toAddress. $email could mean so many things. – Misko Jan 15 '13 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*. – Joachim Sauer Jan 17 '13 at 08: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;
}
Laurent Bourgault-Roy
  • 5,606
  • 4
  • 20
  • 25
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.

Caleb
  • 38,959
  • 8
  • 94
  • 152
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.

Jonno
  • 1,738
  • 10
  • 17
-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.

Wizard Of Tech
  • 332
  • 2
  • 8