In the end, I got this function. I don't know whether it's normal or not.
function user_registration($user_name, $user_email, $user_pass, $address,
$city, $postalcode, $country, $phone, $mobilephone)
How and why can I improve this?
You could either pass an array with all variables packed nicely together, or just make a "User" class and add all properties via setters and do the validation in the end with a dedicated method:
class User {
public function setName($name) {
$this->name = $name;
}
[...]
public function register() {
//Validate input
if (empty($this->name))
$this->errors[] = "ERROR, Username must not be emtpy";
//Add the user to the database
//Your SQL query
return empty($this->errors);
}
}
$user = new User();
$user->setName("Peter");
$success = $user->register();
if (!$success)
echo "ERRORS OCCURED: ".print_r($user->errors, true);
A solution would be to only have one parameter, that can contain several pieces of data -- like an array.
Your function could be defined this way :
function user_registration(array $data) {
// work with $data['name']
// and $data['email']
// ...
}
And you'd call it like this :
user_registration(array(
'name' => 'blah',
'email' => 'test@example.com',
'pass' => '123456',
// and so on
));
Nice things are :
Not so bad things are :
I personally don't think it has too many parameters. From looking at the functions definition it is clear what you require as input which wouldn't be so obvious if called with an array.
"If it aint broke don't fix it!"
When you look at your argument names, you cannot but notice that they can be grouped into three different groups:
User Data: $user_name, $user_pass
Address Data: $address, $city, $postalcode, $country
Contact Data: $user_email, $phone, $mobilephone
Consequently, you could apply Introduce Parameter Object:
Often you see a particular group of parameters that tend to be passed together. Several methods may use this group, either on one class or in several classes. Such a group of classes is a data clump and can be replaced with an object that carries all of this data. It is worthwhile to turn these parameters into objects just to group the data together. This refactoring is useful because it reduces the size of the parameter lists, and long parameter lists are hard to understand. The defined accessors on the new object also make the code more consistent, which again makes it easier to understand and modify.
If you dont want to do OOP you could group the arguments into arrays as well but you'll lose all the type benefits then. I will just assume you don't mind using objects. So, after applying the Refactoring you'll end up with
function user_registration(User $user, Address $address, Contact $contact)
Looking at that parameter list should make you notice that Address and Contact likely belong to User in the first place, so you could consider changing the function signature to just
function user_registration(User $user)
and then call it like this:
$user = new User('johndoe', 'secretsauce');
$user->setAddress(new Address('Doe Street', 'Doe Town', 12345, 'Neverland'));
$user->setContact('jdoe@example.com', '+123 12345', '+123 54321');
user_registration($user);
We could probably make username and password into a Credentials object as well and then just do
user_registration(new User($credentials, $address, $contact));
By requiring the data in the ctor we make sure newly registered users do have all these information. We could argue whether we need Address and Contact for registering users though, so Setter injection might be good enough here:
$user = new User(new Credentials('johndoe', 'secretsauce'));
$user->setAddress(new Address('Doe Street', 'Doe Town', 12345, 'Neverland'));
$user->setContact(new Contact('jdoe@example.com', '+123 12345', '+123 54321'));
user_registration($user);
However, user_registration as a separate function in global scope is misplaced. By GRASP's Information Expert principle, methods should be on the objects that have the most information to fulfill the responsibility. This improves Cohesion and reduces Coupling. In other words:
$user = new User($credentials);
$user->setAddress($address);
$user->setContact($contact);
$user->register();
One problem with the User class now is that it contains the password. The password is only ever needed to authenticate the user against the authentication service. We could argue about the username, but the password definitely should not be part of the User object at all. So you should do something like
$user = new User;
$user->setAddress($address);
$user->setContact($contact);
$user->register($credentials);
and when register() is called, it will only use the credentials to delegate insertion of a new user into the user storage. But it will not retain them in the actual User instance.
Finally, you might want to add a Simple Factory or Builder pattern to encapsulate the creation of the User to simplify the aggregation of the various instances. Or you might want to introduce a Repository pattern and move the register() method there. This is beyond scope for this question though.
As a general rule of thumb (not as a steadfast rule), anytime you have to ask "Does this function have too many parameters?" -- the answer is yes. Your intuition is telling you something that your brain just hasn't yet been able to work out.
In this particular case, the first thing that comes to mind is that your user cred.s should be checked first (does the username already exist? is the pw complex enough) and your user details should be added separately, probably using an object or array.
One way is to pass an array as parameter to this function and put all info in that array:
function user_registration(array $user_info)
{
// process $user_info;
}
I make array of keys like
$fields = array('field1', 'field2');
function register (array $values, array $keys)
{
$data = array();
foreach ($keys as $one)
{
if (isset($values[$one])) $data[$one] = $values[$one];
}
// or you can use array functions like array_flip and after - array intersect
}
Function (method) without any parameters is best. Function with one paremeter is better than function with 2 parameters. Function with 2 parameters is better than function with 3 parameters and so on.
@Florianh has given a perfect solution how your code can be improved. With this comment, I would like to elaborate on the "why" part from a system design perspective.
From an Object Oriented perspective, other objects should be able to manipulate attributes. That is why attributes should never be defined as "public". They should be "private" e.g.:
private var $name;
The reason for this is when other objects would manipulate this variable, the correct working of the object is at risk. Methods, on the other hand, can be defined publicly:
public function register()
Accordingly, the manipulation of the attributes will happen through the appropriate methods. A method can be used to also evaluate the correctness of operations on the attributes.
There are two operations that can happen: reading the current value of an attribute by using Get methods and saving a new value of an attribute by using Set methods.
A good practice would be to implement a get method for each class attribute. Each attribute that can be modified, should have an appropriate set method as well.
Sometimes it is better to not implement a get/set method (e.g.: showData()) after all. This is because the usage of getters and setters within a particular class may possibly lead to a reduced performance. However, this means that when changing or implementing the class, one must be careful when trying to save false information and as a result putting the integrity of the class at risk.
Now, consider the fact that you decide to use only one phone number instead of both a phone- and mobile number. When the mobile number becomes deprecated, your main program still remains the same. All you have to do is change/remove one method. The advantage of using getters and setters is an increase in adaptability and maintainability.
I'd make it this way
fields=explode(",","name,surname,lastname,street,city,region,zip,country");
user_registration($fields);
Because I am sure these variables coming from $_POST
