All of the code in my project is procedural. It isn't using any framework, or any MVC structure. Most of the PHP is form handlers with some logic. So lots of code like this:
<?php
require "headerFile.php";
$user = array(); // filled with user data
if (isset($_POST['submitButton'])) {
if (isset($_POST['money'])) {
if (is_numeric($_POST['money'])) { // I don't actually validate numbers this way
if ($_POST['money'] <= $user['money']) {
$query = mysql_query("SELECT * FROM someTable WHERE id={$user['id']}");
if($result = mysql_fetch_array($query)) {
if ($someOtherCheck == $user['someOtherData']) {
/*
* run some queries, give user some item
*
*/
} else {
echo "This isn't right.";
}
} else {
echo "You don't have a row in some table!";
}
} else {
echo "You don't have that much money!";
}
} else {
echo "Enter a valid number";
}
} else {
echo "Enter how much you want to wager.";
}
}
// some hard coded form goes here
require "footerFile.php";
?>
There are over a hundred of these forms and nested if handlers, some that are similar with more/fewer conditions.
I want to start using OOP and I've been reading up on design patterns but I can't seem to find anything that applicable to my project.
I'm adding loads of new blocks of code similar to the block above, I don't want to copy and paste and just change a couple of bits here and there, what are my options?
I want to start using OOP and I've been reading up on design patterns but I can't seem to find anything that applicable to my project.
You don't have to start with patterns yet .. understand the basics and you can progress form there ..
This is a basic example
if (isset($_POST['submitButton'])) {
$db = new \mysqli("localhost", "user", "pass", "db");
$user = new User("Juddling Stack", 123456);
$user->setMoney(500.01);
$player = new PlayerRegister($db, $user);
if (! isset($_POST['money']) || ! is_numeric($_POST['money']))
$player->addError("Enter Valid Wager Money");
if ($_POST['money'] <= $user['money']) {
$player->addError("You don't have that much money!");
}
try {
$player->doSQL();
} catch ( BaseException $e ) {
foreach ( $e->getError() as $error ) {
echo $error, " <br />";
}
}
}
OOP is not always about application. It is about readability and structure. Honestly, how readable is the code you have posted here? There are a ton of things happening and it would take you 10 minutes to decipher them all. However, if you break it down into classes with class functions you would be able to glance at things and know what is going on.
OOP does not do a ton for php all the time, but its something you want to do for almost all other non-static load languages. With the added benefit that if you have more than one programmer on a project you can both read the code. Comments are you friend.
You're best option is to use functions with returns. Return the value and then the function does whatever it needs to do with it. I personally would do something like
$class->check
if error
$this->error_handler
and the function error handler does whatever you want it to do with the error, if its die or echo then do that, but you build the error handler, so if you ever want to change it, you chance it in one place instead of 20.
Even without bringing in OOP, you could do wonders for structuring your code to be readable. There's no need to nest your conditionals if they're not dependent on each other.
$form_is_valid = true;
if (!isset($_POST['submitButton'])) {
echo "This isn't right.";
$form_is_valid = false;
}
if (!isset($_POST['money'])) {
echo "Enter how much you want to wager.";
$form_is_valid = false;
} elseif (!is_numeric($_POST['money'])) {
echo "Enter a valid number";
$form_is_valid = false;
} elseif (!$_POST['money'] <= $user['money']) {
echo "You don't have that much money!";
$form_is_valid = false;
}
if ($form_is_valid) {
do_stuff_here();
}
It looks like your code is combining certain components of both the model and view layers. You're running queries against a database, and in the same place, you're including hard coded forms. So, a good place for you to start would be to split these tasks up into two separate classes. For example, write a class to connect to your database and run queries against it, and another class to actually serve your content.
My advice on Design patterns is to not get too bogged down in the details. Instead, try to understand why certain patterns are so helpful, and what problems they are trying to solve. A lot of beginners get too bogged down in the how , and they end up wasting so much time learning the nuances of a framework when a simple solution would have sufficed.
Finally, as you read through your code, keep an eye out for things that could potentially be structured into a class. Also, remember that specialization is key, and by building classes that are very specialized in what they do, you are building modular components that could potentially be used in other projects,
First off, if you are ever copying and pasting code that should be a BIG RED FLAG. This should be a warning that instead of copying and pasting, you need to write a generalized function that can be used.
Secondly, it's really time to stop using mysql_query-like functions. Even the PHP page says use of those functions is discouraged. I would start moving your SQL over to PDO which is more OO-like.
When I inherited an application that looked the code you posted, I wrote a blog post about the steps I took to start getting a handle on things. It may also be helpful to you - http://mattmccormick.ca/2011/10/13/how-to-refactor-a-php-application/
A decent framework will help you on your way to organizing code better trough separation of concerns, but does not necessarily enforce best practices. Subjectively, I say it takes hands-on experience and making lots of mistakes before best practices are ingrained in your head.
Try to think of a framework as a delivery mechanism only. Optimally, your code is not tied to any one framework. This generally means using components to handle different aspects of your application such as routing, validation, config, events, dependencies, etc.
Something I feel you should start with would be the SOLID principle. This will help you, although not guarantee, avoid making mistakes that cost you a lot of time down the road.
Foremost, your objects should only have a single responsibility. e.g. a Product object should not be responsible for acting as a data store, persisting itself, handling an order, etc etc.
Also another biggy would be dependency injection. This is huge for unit testing your classes (which you should get in the habit of doing). In a nutshell, do not construct dependency within a class. Construct it beforehand and pass it in through either a constructor argument or a setter method.
The process of architecting an application merits having its own books, so I'm not going to attempt writing it all here. If you follow the SOLID principle though, you will be well on your way to being a better developer.
That kind of nesting is generally a smell, and I can see why you are asking the question...
Step 1 would be to separate the validation in to its own service. Thinking of this in a MVC sense, at the least, your controller would only have [pseudocode] if ($form->isValid()) { do something }. This alone eliminates the bulk of spaghetti you have.
Let's start by stating the problem you are having which is not not using OOP, but called programming-overhead or in programmer terms spaghetti-code.
If you experience a lot of overhead, meaning time wasted writing almost the exact same line of code, where only it's content is different, but the functionality is the same. Then start slicing every peace of code that's the same from it's function, but differentiate its content.
You stated there is more to be copied pasted, and even more complex, I'll just do the form validation part (something I called stage 1), which is just one simple example of how to apply logic that does all the work for you by feeding input it expects. One example might be more elegant than others.
all code below is not tested
An example for locating code of equal functionality.
// functionality on checking values is the same, but it's content is not
if (isset($_POST['submitButton'])) {
if (isset($_POST['money'])) {
if (is_numeric($_POST['money'])) {
and
// though every decision made by its content is only being produced once ..
} else
echo "You don't have that much money!";
} else
echo "Enter a valid number";
} else
echo "Enter how much you want to wager.";
Now the trick is to find a solution to logically solve this. PHP is full of build-in functions, but first grasp the idea on how to solve it. An example would be to have each key, like submitButton, money have a value that would equal to not exists if not set/present, let's say null. Second, you are comparing values with a key supplied by the $_POST, so no matter what.. your $_POST array is the decision maker.
A keen example can be seen on how the jQuery library has been build using the $.extend() method to apply default values to keys, so that a key always has a value and always a decision to make by not checking first if it exists. But so can PHP.
Let's default the values.
$_POST = array_merge(array(
'submitButton' => null,
'money' => 0,
'etc' => '...'
, $_POST);
Building a function to validate this array is a lot easier now, because you can always depend on a value being present.
You stated you have many more forms that need validation, a function that would validate certain fields would be next to figure out.
A formal representation of a valid or invalid form could be an array, e.g.
$valid_form = array(
'submitButton' => array('not_null'),
'money' => array('not_null','int'),
'etc' => '...'
);
A function to validate would be
function validateForm($values, $valid) {
// error array to be returned
$error = array();
// let's iterate over each value, remember we defaulted the $_POST array with
// all the fields it can have, so all fields should be iterated apon.
foreach($values as $key => $value) {
if(array_key_exist($key, $valid)) {
// logic content can be used by different functions, switch
// used here for simplicity
foreach($valid[$key] as $validation) {
switch($validation) {
case 'not_null':
if(is_null($value)) {
$error[] = "error logic";
continue; // skip rest
}
break;
case 'etc':
$error[] = "..";
break;
}
}
}
}
return $error ? $error : true; // true being valid
}
The error handling can be done in a lot of ways, just one simple example (depends on how extensive this project is going to be), you could bind the error content to the validation key, e.g.
$vfe = $valid_form_errors = array( // $vfe for simlicity's sake
'__no_error' => 'no error present for "%key%" validation',
'not_null' => '%key% should not be null',
'int' => '%key% expects to be an integer'
);
$valid_form = array(
'submitButton' => array('not_null'),
'money' => array('not_null','int'),
'etc' => '...'
);
A function to create a formal error message
function error_msg($key, $validation) {
global $vfe;
// error exists?
$eE = array_key_exists($validation,$vfe);
return str_replace('%key%', $eE?$key:$validation, $vfe[$eE?$validation:'__no_error']);
}
And in the simple switch, the error logic is
foreach($valid[$key] as $validation) {
switch($validation) {
case 'not_null':
if(is_null($value))
$error[] = error_msg($key, $validation);
break;
case 'etc':
$error[] = "..";
break;
}
}
So how would your code look like using a different logic?
// first stage ..
$form_valid = validateForm($_POST, $valid_form);
if ($form_valid === true) {
// second stage, same logic be applied as with validateForm, etc.
if($_POST['money'] <= $user['money']) {
$query = mysql_query("SELECT * FROM someTable WHERE id={$user['id']}");
if($result = mysql_fetch_array($query)) {
// third stage, same logic can be applied here..
if ($someOtherCheck == $user['someOtherData']) {
} else {
echo "This isn't right.";
}
} else {
echo "You don't have a row in some table!";
}
}
else {
$errors = $form_valid;
// error handling here
print_r($errors);
}
It all comes to how specific you can define values being expected. You can extend every function to more specific, like binding the errors to a form key so you can specifically target that input at a later stage. They key thing is to erase all possible duplication where a function might do all this for you by simply asking him to compare values you expect, and let him tell which value there actually are.