The focus of this post is to highlight bad programming practises that can occur on any modern programming language or framework. We are going to try and identify bad practises that are unquestionably wrong and should be avoided.
To try and analyse these bad practises, undestand why they are happening and how they can be solved we will need a programming language to show some concrete examples. We will use PHP for this as i am most familiar with it.
So one of the unquestionable bad practises used is...
Uncontrolled usage of global variables 🤪
(in reverse)
Our usual suspects are Global Mutatable States
Global means that it can be accessed from every point of our code. A usual example for this is the $_POST, $_GET or $_SESSION parameter in PHP.
Mutatable means that it can change its value from everywhere. $_SESSION parameter is an example of a parameter that is going to be used throughout our application and can change its state.
State is the current situation. For example a switch of a light has two states. Being on or off. Based on its history it can only go from one state to another. If it is On the only state it can go into is being off.
One clear example of widespread use of Global mutatable states is the $_SESSION variable in PHP.
An example of uncontrolled usage of a global variable🔧
Let's suppose that you are saving in a session the information of a cart in an e-shop. $_SESSION['cart']. Your use of the session variable is justified as the user will need access to the cart information througout his visit. Let's look at an example of a function that takes a decision based on the cart length
function sendNotificationEmailBasedOnCartLength() {
if (sizeof($_SESSION['cart']) > 4 && $_SESSION['cart']['hasSentEmail'] == 0) {
sendSomeEmail();
}
$_SESSION['cart']['hasSentEmail'] = 1;
}
Unessesary global injection ❌
Issues that injection presents:
Maintainabillity:
In this example we can see that there is an unecessary injection of the global variable $_SESSION['cart'] inside function. This function is at this point dependant on the $_SESSION variable to function. Even if we decide later that we have found a better way to store and retrieve cart information we will have to identify every point that $_SESSION is used and change the usage.Readabillity:
One more problem that arises from injecting these global variables is that whoever is reading the function doesn't know that these variables exist in the first place. It is not obvious as they are not stated as parameters. This degrades the readabillity of the code.
This is an error-prone approach. If your project does not have test suites you will have to debug this slowly and a lot of mistakes will eventually land on production.
Uncontrolled change of global state
Here we are changing the hasSentEmail value of cart to 1 to inform the application that we have already sent an email to this cart and to not do it again.
Where else does this value change? We have absolutely no clue The hasSentEmail value from $_SESSION variable could change at any point in our application from any function if we do not have any controlled way of using this session. If we are doing a session_start() and opening the session gate in every file there could be a session mutation in any file. This creates unpredictabillity and destabilizes our program.
Solutions! 🤔
Limit the globallity of the $_SESSION
function sendNotificationEmailBasedOnCartLength(int $cartSize, int $hasEmailBeenSentForCart = 0) {
if ($cartSize > 4 && $hasEmailBeenSentForCart == 0) {
sendSomeEmail();
}
}
First you would need to decouple the function and the session. Pass any information you need about the session inside the parameters of the function. This limits the global variable to a local variable.
Better control of $_SESSION by limiting it's usage
Second, you should not access the $_SESSION['cart'] variable directly in any file. Choose a file and create a class that is responsible for creating the session, retrieving it and changing it. By having a single class that is responsible for these actions you can be assured that the global variable will not have an unexpected state somewhere down the road.
session_start();
class Cart {
private $products;
public function __constructor() {
if (!$_SESSION['cart']) {
$_SESSION['cart'] = ['products' => [], "emailIsSent" => 0];
}
}
public function initializeCart() {
$_SESSION['cart'] = ['products' => [], "emailIsSent" => 0];
}
public function addProduct(Product $product) {
$_SESSION['cart']['products'][] = $product;
}
public function setEmailIsSent() {
$_SESSION['cart']['emailIsSent'] = 1;
}
public function getCart() {
return $_SESSION['cart'];
}
}
Here can see a simple example of creating a class that is in the only mutator and retriever $_SESSION['cart']. If you decide in the future to retrieve information for the cart or creating a new cart you are not dependable on the $_SESSION['cart'] variable as you can swap the functionallity of these functions anyhow you like and the your app would work the same.
Additionally we can decide that only these classes will have the abillity to interact with sessions and remove all session_start() instances from anywhere else making sure that this global variable will only change its state in this controlled enviroment.
The session['cart'] variable can only hold the values specified by this class.
In conclusion
We did show why having a global variable mutate uncontrollably in our application is bad as it creates:
- Unpredictabillity
- Maintainabillity issues
- Unredabillity
and we demonstrated with the use of classes that control the mutations and retrieval of these variables how we can make our application cleaner and easier to adapt to change
Sources:
Top comments (7)
Great article! Just a question, do you think it would be equally useful to have the functions of the
Cartclass defined outside? I mean having the functions without a class, just as subroutines to handle the global variable.First, thanks a lot i appreciate that you liked the article!
I think that using classes specifically in PHP is better than subroutines because you can autoload the classes. If you would use subroutines you would have to require() them. I generally refrain from using require in PHP because it can lead to functions or variables being used that depend on other files indirectly.
For example let's say you got index.php where you are creating a function that you require in index2.php. Let's say now that you create index3.php that requires index2.php which requires index.php and you are using in index3.php the function from index.php. You do not know now from where the function has been imported. While reading index3.php you only know that you are requiring index2.php which does not have the function.
It can generally become a mess really fast and i have seen it becoming a mess. While using autoload instead of require you always know what functions are coming from which class which leads to cleaner code.
I'm not familiar with PHP, so I didn't see that... Now it makes sense!
Keep up the good work!
Your article shows a nicer and more organized way to go about writing session code, but your argument falls flat.
In the example where you check cart size using
$_SESSION['cart']and the later change it to$cartSizeYou would still have the same problem of going and finding all the places where
$cartSizeis modified to make a future change.In the same sense the entire object you created for the Cart has the exact same problem, except now when someone goes "What changed?"
For example:
Instead of saying "on line X we modified
$_SESSION['cart']['emailIsSent']" which would bring us straight to the modified variable.With the new object's usage, when debugging we'd have to go "on line X it's calling Cart->setEmailIsSent()" and we now have to go find that Cart object to then go "This modifies
$_SESSION['cart']['emailIsSent']Now, assuming your team is good at organizing their files and objects, it's a quick hop over to the Cart object, but not all systems are built that nicely which means you end up doing another search to find it.
This is not more efficient, so abstraction often ends up hurting more than helping especially in large systems where abstraction is done multiple levels deep.
Btw, i'm all for using objects, just highlighting that this does not improve debugging time, especially when the argument is "you have to search for it everywhere", since you need to do that anyway, and VSCode has an awesome search that literally finds them in like 0.2 seconds :)
my 2 cents.
The point there is that we are no longer passing a global parameter to a function. This makes the function more reusable and you can easily understand the variables it is using
The point is that by having the session parameter exposed everywhere in our code we force the application to be tied with the $_SESSION. By using a class that handles the interaction with $_SESSION[cart] we can easily change the way we control the cart anyhow we like and we would not have to change all instances of $_SESSION.
For your amusement and edification, the classic How To Write Unmaintainable Code.
Very helpful!