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
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?
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
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
@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.
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
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!
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.
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?
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?
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.
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;
}
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.
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.
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!
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.