Reply To: Optimising and consolidating multiple PHPMailer functions

[__]

I have heard quite a few people claiming global variables are bad – what is the problem with them – too open access?

The big thing is that it’s very easy for them to lead to hard-to-discover problems, especially as your program becomes larger or more complex. Your code should be doing everything it does deliberately.

In this case, you code simply assumes that a) there is a global variable named $mail, and b) it is an instance of the PHPmailer class. What if that var wasn’t available for some reason? You’d get an “undefined variable” notice, but many people have error reporting set to ignore notices. So, instead, you’d get a message along the lines of “trying to call method IsSMTP on a non-object”—which is far less clear and takes longer to track down.

Two solutions.

• dependency injection. That’s my first suggestion, where you actually have to pass the var you need into the function as a parameter. Any time you need a var from outside a function, it should be passed in deliberately.
• encapsulation. (This is usually considered in terms of objects, but the idea holds true here.) If your function needs something, and it is never needed outside the function, then the function can create and use it itself.

There are other benefits, having to do with making your code more flexible and adaptable in the future, being easier to debug, and just generally more robust. If you remember that global and $GLOBALS are, in general, “bad words,” you’ll be fine.

After removing the global variable and creating a new object instead, I get a “Fatal error: Class not found”??

You’ll have to make sure that the PHPMailer class definition is loaded before your function tries to create an instance of it. You could use an autoloader (you will learn to love these), or do something like require_once "/path/to/PHPMailer/definition.php" inside the function.

would you recommend having a class just for emails? At the moment it is just in a General class

Yes. I would probably recommend not having a “General” class at all. An object represents one “thing”: an item, a task, etc.. Each “thing” should have its own class, even if there is only one method involved.