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

Checking for existence of URL parameters

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
existencecheckingforparametersurl

Problem

I'm busy rewriting some from my spaghetti code into OOP classes. I'm still learning OOP and very new to the concept. From help from this answer from @EliasVanOotegemto on a previous question I've asked I have written the following class which sets up conditionals according to 4 values passed to it by a user and checking these values against URL parameters whether they exist or not

Here is the class with its interface:

ConditionsRefInterface.php

namespace PG\Referrer\Single\Post;

interface ConditionsReferInterface
{
    /**
     * @param (string)$authorReferrer
     * @return $this
    */
    public function setAuthorReferrer($isAuthorReferrer);

    /**
     * @param (string)$dateReferrer
     * @return $this
    */
    public function setDateReferrer($isDateReferrer);

    /**
     * @param (string)$searchReferrer
     * @return $this
    */
    public function setSearchReferrer($isSearchReferrer);

    /**
     * @param (string)$taxReferrer
     * @return $this
    */
    public function setTaxReferrer($isTaxReferrer);

    /**
     * @return (bool)
    */
    public function isAuthorReferrer();

    /**
     * @return (bool)
    */
    public function isDateReferrer();

    /**
     * @return (bool)
    */
    public function isSearchReferrer();

    /**
     * @return (bool)
    */
    public function isTaxReferrer();
}


ConditionsRef.php

```
namespace PG\Referrer\Single\Post;

class ConditionsRefer implements ConditionsReferInterface {

/**
* @var $authorReferrer = null
*/
protected $isAuthorReferrer = null;

/**
* @var $dateReferrer = null
*/
protected $isDateReferrer = null;

/**
* @var $searchReferrer = null
*/
protected $isSearchReferrer = null;

/**
* @var $taxReferrer = null
*/
protected $isTaxReferrer = null;

/**
* @param array $values = null;
*
* Accepted parameters that can be passed to $values.
* @param (string) authorReferrer
* @param (

Solution

GET and setters

One of the points of @EliasVanOotegemto was that your class expects the $_GET variable to be set, which is not ideal because it makes your class very static and hard to reuse. What if you at some point do not store this data in GET, but in POST? Or in a session? You cannot use your class in that case.

That is why @EliasVanOotegemto suggested to pass the value contained in GET instead of passing the index at which the value is stored in GET. But through your new design, GET is actually used in two classes instead of one, which couples your classes together, and makes both hard to reuse.

Your setters should actually look really simple:

public function setDateReferrer($isDateReferrer)
    $this->isDateReferrer = $isDateReferrer;
    return $this;
}


And then used eg like this:

conditionsRefer->setDateReferrer($_GET['dateReferer']);


Naming

  • class names should ideally be the same as the file in which they are stored (possibly with the keyword class added. ConditionsRef.php isn't a very good name (What's a Ref?), ConditionsReferer.php, ConditionsReferer.class.php, conditionsReferer.class.php would all be better (just choose one naming theme, and then stick with it).



  • be consistent: either it's referer or referrer.



  • isVariableName usually implies a boolean value. Your isDateReferrer etc do not seem to be boolean values though.



Comments

  • your auto-generated comments seem wrong (eg @param (string)$authorReferrer vs $isAuthorReferrer, or protected $isAuthorReferrer vs @var $authorReferrer).



  • sometimes, auto-generated comments might be fine, but I would prefer some additional information. For example, what is a taxReferrer? What values are $values?



  • class level comments would be great. What is a ConditionsRefer?



Misc

  • I'm not sure why you use filter_var in your code. If you want to protect against XSS attacks, I would clean the data when echoing it to the enduser, not at any other point (otherwise it becomes a guessing game when echoing data: did I already clean it?).



  • I don't like using switch(true) instead of if-else, it just looks really wrong.



  • do you actually expect a different implementations of ConditionsReferInterface than ConditionsRefer? It's hard for me to imagine one. If there is only one possible implementation of an interface, the interface isn't really needed.

Code Snippets

public function setDateReferrer($isDateReferrer)
    $this->isDateReferrer = $isDateReferrer;
    return $this;
}
conditionsRefer->setDateReferrer($_GET['dateReferer']);

Context

StackExchange Code Review Q#80601, answer score: 5

Revisions (0)

No revisions yet.