HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

PHP isset over use? Good or bad?

Submitted by: @import:stackexchange-codereview··
0
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 $_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

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.