patternphpMinor
PHP isset over use? Good or bad?
Viewed 0 times
phpissetbadgooduseover
Problem
First, I'm in no way an experienced PHP coder. This is my 5th time working with PHP, so if you see anything that can bee improve, please point them out for me.
I have the code checking for the
```
Front Page';
$pgeBreadcumbs .= 'Free website';
if ($pgeName == 'index') {
$query = 'SELECT title, description, keywords FROM rff_pageTitle WHERE pge = \'default\' LIMIT 1';
$pge = mysqli_fetch_assoc(rff_query($query));
$pgeIfo = $pge;
$pgeIfo['breadcrumbs'] = $pgeBreadcumbs;
} else {
if (isset($_GET['sortby'])) {
$sortby = safeVal($_GET['sortby']);
if ($pgeName == 'browsegallery') {
switch ($sortby) {
case 'mostpopular' : $sortby = 'Most Popular'; break;
case 'heighestrated' : $sortby = 'Highest Rated'; break;
case 'mostviewed' : $sortby = 'Most Viewed'; break;
default : $sortby = $sortby;
}
}
}
$pge_num = 1;
if (isset($_GET['page']))
$pge_num = safeVal($_GET['page']);
if (isset($_GET['slug'])) {
$slug = safeVal($_GET['slug']);
$anm_nme = rff_getAnimeName($slug);
$anm_id = getIDbySlug($slug);
$anm_tpe = (rff_getAnimeType($anm_id) == 1) ? 'JAV' : 'H-Anime';
$anm_pdc = '-';
}
if (isset($_GET['s']))
$s = safeVal($_GET['s']);
if (isset($_GET['tag']))
$tag = safeVal($_GET['tag']);
if (isset($_GET['startfrom']))
$startfrom = safeVal($_GET['startfrom']);
if (isset($_GET['param'])) {
$param = safeVal($_GET['param']);
$contriType = str_replace('-', ' ', $param);
}
if (isset($_GET['eps']))
$eps = safeVal($_GET['eps']);
if (isset($_GET['cond'])) {
$cond = safeVal($_GET['cond']);
$edt_cond = str_rep
I have the code checking for the
$_GET variable and use them to create the page title:```
Front Page';
$pgeBreadcumbs .= 'Free website';
if ($pgeName == 'index') {
$query = 'SELECT title, description, keywords FROM rff_pageTitle WHERE pge = \'default\' LIMIT 1';
$pge = mysqli_fetch_assoc(rff_query($query));
$pgeIfo = $pge;
$pgeIfo['breadcrumbs'] = $pgeBreadcumbs;
} else {
if (isset($_GET['sortby'])) {
$sortby = safeVal($_GET['sortby']);
if ($pgeName == 'browsegallery') {
switch ($sortby) {
case 'mostpopular' : $sortby = 'Most Popular'; break;
case 'heighestrated' : $sortby = 'Highest Rated'; break;
case 'mostviewed' : $sortby = 'Most Viewed'; break;
default : $sortby = $sortby;
}
}
}
$pge_num = 1;
if (isset($_GET['page']))
$pge_num = safeVal($_GET['page']);
if (isset($_GET['slug'])) {
$slug = safeVal($_GET['slug']);
$anm_nme = rff_getAnimeName($slug);
$anm_id = getIDbySlug($slug);
$anm_tpe = (rff_getAnimeType($anm_id) == 1) ? 'JAV' : 'H-Anime';
$anm_pdc = '-';
}
if (isset($_GET['s']))
$s = safeVal($_GET['s']);
if (isset($_GET['tag']))
$tag = safeVal($_GET['tag']);
if (isset($_GET['startfrom']))
$startfrom = safeVal($_GET['startfrom']);
if (isset($_GET['param'])) {
$param = safeVal($_GET['param']);
$contriType = str_replace('-', ' ', $param);
}
if (isset($_GET['eps']))
$eps = safeVal($_GET['eps']);
if (isset($_GET['cond'])) {
$cond = safeVal($_GET['cond']);
$edt_cond = str_rep
Solution
Isset should not affect performance.
About code improvements I suggest you to write some kind of wrapper above Request to improve readability
Also connections like this:
should be in config files or in DB. It shouldn't be directly in the code. Common way is to keep page data in DB:
If you have such table you are able to select page by slug or by id and then fill necessary information to display it to user.
If your page is not just a content page (e.g. search form) it's good to have separate controller to handle all request params and a separate view to render this form (including title and other stuff)
About code improvements I suggest you to write some kind of wrapper above Request to improve readability
val = $val;
}
public function raw() {
return $this->val;
}
public function safe() {
// just an example
if (is_string($this->val))
return htmlspecialchars($this->val);
return $this->val;
}
public function int() {
return int($this->val);
}
protected $val;
}
class Request {
public function get($varName, $defaultValue = false) {
if (isset($_GET[$varName]))
return new RequestData($_GET[$varName]);
return new RequestData($defaultValue);
}
public function post($varName, $defaultValue = false) {
if (isset($_POST[$varName]))
return RequestData($_POST[$varName]);
return new RequestData($defaultValue);
}
}
$_GET['test'] = 'alert();';
$request = new Request();
$testVal = $request->get('test')->safe();
if ($testVal) {
echo 'test exists: ' . $testVal;
} else {
echo 'test does not exist';
}Also connections like this:
case 'mysettings' : $newPgeName = 'My Settings'; break;should be in config files or in DB. It shouldn't be directly in the code. Common way is to keep page data in DB:
- id
- title
- excerpt
- content
- slug
- createdAt
- category
- keywords
- description
If you have such table you are able to select page by slug or by id and then fill necessary information to display it to user.
If your page is not just a content page (e.g. search form) it's good to have separate controller to handle all request params and a separate view to render this form (including title and other stuff)
Code Snippets
<?php
class RequestData {
public function __construct($val) {
$this->val = $val;
}
public function raw() {
return $this->val;
}
public function safe() {
// just an example
if (is_string($this->val))
return htmlspecialchars($this->val);
return $this->val;
}
public function int() {
return int($this->val);
}
protected $val;
}
class Request {
public function get($varName, $defaultValue = false) {
if (isset($_GET[$varName]))
return new RequestData($_GET[$varName]);
return new RequestData($defaultValue);
}
public function post($varName, $defaultValue = false) {
if (isset($_POST[$varName]))
return RequestData($_POST[$varName]);
return new RequestData($defaultValue);
}
}
$_GET['test'] = '<script>alert();</script>';
$request = new Request();
$testVal = $request->get('test')->safe();
if ($testVal) {
echo 'test exists: ' . $testVal;
} else {
echo 'test does not exist';
}case 'mysettings' : $newPgeName = 'My Settings'; break;Context
StackExchange Code Review Q#54617, answer score: 8
Revisions (0)
No revisions yet.