From Switch to Function

[gamer-goddess]

So, I'm working on making my code a little prettier.

Previously, if I wanted to do 3 different things, I would create 3 different php pages for those three different things.

 

For Example:

<form action="user_add.php">

<form action="user_edit.php">

<form action="user_delete.php">

This would usually be 6 different pages. (3 "gui" pages and 3 php pages that performed specific actions)

 

So, I've decided to create 1 specific page called "actions.php"

Using a switch.case variable named $request

This will allow me to achieve the following:

<form action="actions.php?request=userAdd">

<form action="actions.php?request=userEdit">

<form action="actions.php?request=userDelete">

 

My actions page looks like this:

<?php
include("connectDB.php");
$request=$_GET['request'];

switch ($request)
{
	case "userDelete":
		echo "delete";
		break;
	case "userEdit":
		echo "edit";
		break;	
	case "userAdd":
		echo "add";
		break;
}
?>

 

Now, assume I access the page that allows me to add a new user. The form action would look like this: <form action=actions.php?request=userAdd>

When I click on the submit button, I would see the words "add" on the page. Perfect, good job, yay me.

 

Now, assume I add the following code block into the "userAdd" case

<?php
...
	case "userAdd":
		// Get values from form
		$username=$_POST['username'];
		$password=$_POST['password'];
		$email=$_POST['email'];	

		// Insert data into mysql
		$sql="INSERT INTO $tbl_users(username, password, email)VALUES('$username', '$password', '$email')";
		$result=mysql_query($sql);

		// if successfully insert data into database, displays message "Successful".
		if($result)
		{ echo "Successful"; }		
		else 
		{ echo "ERROR"; }

		echo $previous_page;
		break;
?>

This works perfectly. Good job, yay me!

 

Now assume I change the case block to look like this:

<?php
...
	case "userAdd":
		adduser();
		break;
}

function adduser()
{
		// Get values from form
		$username=$_POST['username'];
		$password=$_POST['password'];
		$email=$_POST['email'];	

		// Insert data into mysql
		$sql="INSERT INTO $tbl_users(username, password, email)VALUES('$username', '$password', '$email')";
		$result=mysql_query($sql);

		// if successfully insert data into database, displays message "Successful".
		if($result)
		{ echo "Successful"; }		
		else 
		{ echo "ERROR"; }

		echo $previous_page;
}
?>

This doesn't work. WHY? If I change the adduser() function to echo "hello world"; I would see "hello world", but when I attempt to copy and paste the block of code (that works in the switch case statement) into the function, it stops working. (It prints nothing)

 

Now, I don't even know if this is the correct place to put this. Last time I had a mysql database error and my thread was moved here, even though I knew it wasn't a php error. So, if this is the wrong section, I apologize - but I assume it falls under both a mysql and php problem.

[PFMaBiSmAd]

Do you have error_reporting set to E_ALL and display_errors set to ON so that all the php errors your code produces will be reported and displayed?

 

 

[gamer-goddess]

Do you have error_reporting set to E_ALL and display_errors set to ON so that all the php errors your code produces will be reported and displayed?

I don't know how to set that

 

EDIT: I'm looking at my error_log and I have no error reports for today.

Normally errors would be printed to the screen

But I'm not seeing anything, it's blank. There's no error, it's just not printing what I want it to

[rwwd]

well if you had error_reporting set to E_ALL you would see that you would need to pass the db connection handle into the function, just so there is a valid connection there for one thing, secondly, using a function in this way seems to be dodgy, functions are there to RETURN something; this can be a boolean (for ease of use) or a string (so that you can return a message to screen).

 

Oh, and please, sanitise your incoming post data BEFORE it is used within a query, all it needs is for someone to put DROP database; and something serious could happen to undo all your hard work.

 

Your SQL statement could do with being formatted a little better to, just to make it a bit more 'friendly', the use of backtick's on column names is always a good thing to get into, you dont have to; but I just find it a better compatibility thing if you have spaces in the column names or reserved word's, this method actually escapes that.

 

function adduser(){//< pass your connection handle here
//or here
global $conn;

Then your query *might work*

 

Ahh, top of your files, the first line should ALWAYS be this:-

 

<?php

error_reporting(E_ALL);

 

Rw

[PFMaBiSmAd]

I would set them in your master php.ini so that parse errors would also be reported/displayed. Stop and start your server to get any changes made to the master php.ini to take effect and use a phpinfo(); statement to confirm that the values actually changed in case the php.ini that you changed is not the one that php is using.

[PFMaBiSmAd]

@rwwd - using the global keyword to pass anything into a function is bad advice.

 

The reason the function code is failing is because the $tbl_users variable does not exist inside the function.

 

For the record (how many times have we written this) -

 

Just putting function some_name(){} around a block of code does not make that code into a function. You must design a function to perform some specific operation and define the (optional) input parameters, define the processing it will perform, and define the results it will return to the calling code.

 

 

 

[rwwd]

@rwwd - using the global keyword to pass anything into a function is bad advice.

 

Not necessarily bad advice, just an option, though my first option was to pass it into the function, this would take less memory up and would be faster *I assume*.

 

Both options will work - but I agree that bad use of global - but it would work.

 

Rw

[gamer-goddess]

So I added error_reporting(E_ALL);

and found out that $tbl_users was undefined... but it IS defined in connectDB.php

which is included at the top of the page

 

I changed $tbl_users to users within the function, and it worked

 

But I want to use variables for my table names and define all table names in variables in my connectDB.php page

 

So, I added include("connectDB.php") to the top of function adduser() and it worked!

But that would mean I would have to include this line into deleteuser() and edituser() as well

 

Isn't that bad?

Also - why should or shouldn't I go from switch to function

I actually will be returning values using these functions, I just stripped away most of the code in order to present my  problem to the forum

 

Edit: Should I just not worry about putting these code blocks into functions and just keep them in the switch..case statement?

[PFMaBiSmAd]

I edited my post above with this information -

For the record (how many times have we written this) -

 

Just putting function some_name(){} around a block of code does not make that code into a function. You must design a function to perform some specific operation and define the (optional) input parameters, define the processing it will perform, and define the results it will return to the calling code.

 

For functions to be useful and reusable (unless you want to keep rewriting and testing them every time you need to change something in the main program), they must operate as black-boxes with zero interaction with the main program except through the parameters they are passed and through the value they return.

[rwwd]

>>Also - why should or shouldn't I go from switch to function

 

There is no reason at all, in fact I have been doing just that. It's a good thing to to to separate code into functions where possible, rule of thumb is if you need to write the same thing out more than once - make it a function - but if a function contains > 25->30 lines of code, split it up. That may be considered bad practise by people here I would think, but lots of people do advocate this as good practice, all down to personal preference I guess.

 

BTW: Constants would be the way to go with your table names I think, but without seeing the context of this script I don't want to commit to anything. Your wouldn't need to include you files in every function then.

 

Hope that makes more sense now anyway.

 

Rw