Switch case dynamic generate it

PHP
1

Guys i have the following code.

Can any one suggest a better way to do it as the teams are generated from a mysql table for their names.

switch ($_GET['team']) {

	case 'north':
		
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;

	case 'south':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;

	case 'it':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'accounts':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;	
		
	case 'fr':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
	case 'frall':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'query':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;

	case 'hr':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'reception':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
	case 'Overflow1':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
	case 'Overflow2':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'reports':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'sotl':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;	
		
	case 'bdm':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'frmonth':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'frallmonth':
		echo $head; echo $nav;
		$table = $_REQUEST['team'];
		$users = grab_user ($table);
		display ($table,$users);
		echo $foot;
		break;
		
	case 'updatetoday':
		echo $head; echo $nav;
		require ('functions.php');
		passthru ('sudo /usr/bin/php /var/www/phonestats/today.php');
		echo $foot;
		break;
		
	case 'today':
		echo $head; echo $nav;
		require ('functions.php');
		require ('html2.php');
		echo $foot;
		break;
	case 'admin':
		echo $head; echo $nav;
		echo '<div align="center">';
		echo '<div class="btn-group">
			  <br><br><a href="update.php"><button type="button" class="btn btn-default">Update Stats</button></a><br>
			  <a href="?team=updatetoday"><button type="button" class="btn btn-default">Update Todays Stats</button></a><br>';
		echo "</div>";
		echo $foot;
		break;


	default:
		echo $head; echo $nav;
		require ('functions.php');
		echo " <body><div align='center'><a href='http://www.mysite' target='_blank'><img src='./images/mysite.jpg'></a><br><br><br>";
		

		$teamlist = grab_full_teams ();
foreach ($teamlist as $team_init)
{
	$short = $team_init['short'];
	$full = $team_init['full'];
echo '<a href="?team=' . $short . '">' . $full .'</a>';
 echo "<br>";
}
echo '<div class="btn-group">
			  <br><br><a href="index.php?team=admin"><button type="button" class="btn btn-default">Administration</button></a><br></div>';
echo $foot;
		break;

}
2

I don’t want to sound totally negative or overly harsh, but the case function is used improperly or rather, you shouldn’t be selecting what to display on a page through a case function in this fashion, because you are basically killing the application’s ability to be “dynamic” (unless the system is generating the case code itself, which it really shouldn’t do either). There is a ton of duplicated code. There seems to be no consideration for OOP.

It is nice you gave it a try and you know it isn’t completely right, but there is no proper answer to your request.

Scott

Posted edited by cpradio to make it sound less of a personal attack

1 Like
3

Hi txt3rob,

There are a couple of things you could do that would immediately reduce duplicated code and make your switch statement easier to read:

  • As the first 16 cases are doing the same thing, you can remove the break statement (and the code) from all but the last one and any time one of those cases are matched, it’ll fall-through to the next case until it finds a break.
switch ($_GET['team']) {
    case 'north':
    case 'south':
    case 'it':
    case 'accounts':
    case 'fr':
    case 'frall':
    case 'query':
    case 'hr':
    case 'reception':
    case 'Overflow1':
    case 'Overflow2': 
    case 'reports':
    case 'sotl':
    case 'bdm':
    case 'frmonth':
    case 'frallmonth':
        $table = $_REQUEST['team'];
        $users = grab_user ($table);
        display ($table,$users);
        break;
    // etc..
}
  • As each case is doing echo $head; echo $nav; at the beginning, and echo $foot; at the end, it makes sense to move those before and after the switch respectively to avoid duplicating them in every case.

As @s_molinari implies, there are probably better ways to go about it, depending on what you’re actually trying to do. Perhaps if you could give some more background on what the code does?

1 Like
4

Don’t want to sound negative or definititely don’t want to go off topic but… that’s exactly what he’s trying to do.

I personally think that it is fantastic that someone really tries and, if he’s unsure or suspects that there’s a better option, he’s not ashamed to ask and show his code as wrong as it can be. That’s the only way to learn and go far :smile:

Maybe you can provide a link to a related article, or a brief explanation of what you think is so wrong… Knowing what to search will likely encourage all of us to use Google a bit more. Although in my case, Google must be fed up with me… I may use it a bit too much, if that’s possible :stuck_out_tongue:

@txt3rob With the information you provide, I think that @fretburner gave you a better and cleaner option, much easier to debug. I’d probably have followed the same approach at this point.

1 Like
5

Hi fretburner i’ve done as you’ve suggested.

the code is designed to display some phone stats from the office.

It’s an early project of mine i did a good while back.

idea being the teams are listed in the database and then if i add a new team it will display their name and then provide a link to click to show their stats.

the code basically takes the team name in the get request and then checks the DB for their stats table and then grabs the users and runs a functions file to grab all the stats to be displayed

6

Does the table names differ much from the name of the teams? Maybe you may not need a switch at all.

Edit: I can see the project is really old… <div align0"center">... That’s so… 90’s !! lol
On the other hand, it means that I have a certain age now… I’m getting older! :frowning:

7

Never use $_REQUEST. Worry about why when you have a bit more experience, for now stick to $_GET and $_POST, for this particular page $_GET.

Now we can’t see how grab_user is written, but I have a bad feeling it’s putting the $table argument straight into the SQL. That opens your code up to SQL injection attack. Don’t trust user input.

A start at simplification is this

if (in_array($\_GET['team'], ['north', 'south', 'it', 'accounts', 'fr', 'frall', 'query', 'hr', 'receiption', 'Overflow1', 'Overflow2', 'reports', 'sotl', 'bdm', 'frmonth', 'frallmonth']) {
  echo $head; echo $nav;
  $table = $\_GET['team'];
  $users = grab_user ($table);
  display ($table,$users);
  echo $foot;
} elseif {

… Handle the last three with additional if/else blocks. Test this, then find a way to dynamically generate the array of the first condition.

Edited by DaveMaxwell: Let’s keep the topic on subject and provide the OP with the help he needs to get back into the game.

8

Your passthru invokes a command using sudo privileges, which probably means your webserver is running under a passwordless sudoer account. If this is running on a local server (e.g. not connected to the internet and only accessed on a local network) it’s probably not a huge problem. If it’s online, though, this could represent a huge security hole.

If you need to address this, you’ll want to remove sudo access from your webserver user, then modify any processes that require sudo privileges to not depend on running as the super user (e.g. your /var/www/phonestats/today.php script).

Do you know why that script requires sudo privilege to execute correctly?

9 
function grab_full_teams ()
{
	GLOBAL $db3;
	$staff1 = $db3->prepare("SELECT * FROM `teams`");
	$staff1->execute();
	$teams_full = $staff1->fetchAll();
	return $teams_full;
	}
	

	
$teams = grab_teams_short ();

is how I return the teams ignore the string names for now but for the teams they are stored in two cols
short and full.

so say the team is the north team the cols would equal

short = north
full = North Logistics

i use PDO statements on ALL mysql query’s.

10

The sudo part can be fully ignored as it’s an internal server (raspberry pi!).

nothing on there is either mission critical or private.

I will eventually alter the sudo part.

11

I suppose a good question would be, how far are you interested in refactoring the code? Did you just want to know about tidying up the switch block, or are you interested in going further and seeing what else you could do?

12

I am going to look at a complete recode as it’s old code.

I’ve slowly been looking at re-doing it but even now i know there is some horrible code in there that i will change.

13

Try this then to get your short team names.

class TeamModel {
  private $db;
  public function __construct( PDO $db ) {
    $this->db = $db  
  }

This class is going to keep all our queries that work with that team together logically. It’s constructor takes a database instance in. We do this because we never want to depend on global objects. Never use PHP’s global keyword - find another way, there’s always a better way. Continuing the code of the class.

  public function getTeamShortNames() {
    return $this->db->query("SELECT short FROM teams")
      ->fetchAll(PDO::FETCH_COLUMN);
  }
}

This method grabs the short team names into a simple array which can be used in your conditional. Now your code from the first post can read

$teams = new TeamModel($db3);

if (in_array($_GET['team'], $teams->getTeamShortNames()) {

And so on. You can add additional get methods to the class to do other fetches of data off the table.

This is a rudimentary model - a full one also knows how to update the table it is working with. Why do this separation? Well, for one, it’s up to the model where the data comes from - the outside code shouldn’t know. For example, let’s add the most caching possible, cache just for the session.

class TeamModel {
  private $db;
  protected $short_names = NULL;

  public function __construct( PDO $db ) {
    $this->db = $db  
  }
  
  public function getTeamShortNames() {
    if ($this->short_names === NULL) {
      $this->short_names = $this->db->query("SELECT short FROM teams")
        ->fetchAll(PDO::FETCH_COLUMN);
    }

    return $this->short_names;
  }

}

Now the model only runs the query once no matter how many times the outside world asks for that data - it makes the assumption no one else is taking to the database about that table though, so this falls apart if that assumption isn’t true.

14

I know about the horrible security issue of the table name … could do with some one showing me how best to handle this and how to whitelist table names so it’s more secure.

function grab_user ($table)
{
	GLOBAL $db3;
	$staff1 = $db3->prepare("SELECT DISTINCT `user` FROM `".$table."` WHERE `Answered` = 'Yes'");
	$staff1->execute();
	$staff = $staff1->fetchAll(PDO::FETCH_COLUMN, 0);
	return $staff;
	
}
15

Use in_array to verify the table name exists and is valid before you include it in the query.

16

I will create a function for in_array and handle any further functions with that as the validator.

I have never used or understood class proper before and this is why my code is ugly.

17

in_array is a PHP native function. If you try to create one of your own you’ll get a function_exists fatal error. Validating the input will work the same as my very first example in this thread. Indeed, if grab_user is only called from there you’re safe. However, the function really shouldn’t rely on outside code to do it’s filtering.

18

i’ve done that now but changed it so that the team list is from $teams array :slight_smile:

if (in_array($_GET['team'], $teams)) {
		

		$table = $_GET['team'];
		$users = grab_user ($table);
		display ($table,$users);
		
}

Thank you for this it’s been a big help!

19

I am glad you said this. It is no problem not to know something. It is a problem to expect others to give you that knowledge, without really trying to get it yourself first. If you had of tried to solve your problem at least some, then you would have gotten a better answer from me.

I’ve been in the online community business for a long time and I’ve learned that spoon feeding someone answers, when they actually don’t understand the basics to begin with, is like feeding that person a single fish to eat. The person might be full for the day, but tomorrow she’ll be hungry again.

I am someone who wholeheartedly believes in giving people the means to fish for themselves, so they can eat and never be hungry for a lifetime. And the means for you to fix this problem is to learn how to code first, which is a learning process that unfortunately cannot be done on a forum efficiently!

I am sorry some of you took my response wrong. I always do want to help. And txt3rob needs to learn a whole lot more about programming. I would have suggested places for him to go, but I don’t know his level of experience. Had he asked, and went down that path of, how can I learn more, I would certainly have guided him.

Scott

20

I think that it was a hint that sometimes you come really, really agressive… I was scared that you would have a knife between the teeth! :stuck_out_tongue:

But I’m sure your intetions were good and I appreciate them.

[quote=“s_molinari, post:19, topic:189629”]
I would have suggested places for him to go, but I don’t know his level of experience
[/quote]I think that it is safe to say that his level is low right now but looking forward to improve (forgive me if I’m wrong @txt3rob)

[quote=“s_molinari, post:19, topic:189629”]
I would have suggested places for him to go,
[/quote]If you feel like it… bring them on!

The reason that I say this is because from the very first post he was asking opinions… being aware that his code was both dated and not up to standards.

@txt3rob Don’t worry. We’re on the same boat :smiley:

1 Like