3
\$\begingroup\$

I am fresh at OOP and I am curious if the code below is object oriented and can be improved:

class cTitleBreadcrumb {

public function __construct($sAction, $sItem){
    $this->sAction = $sAction;
    $this->sItem = $sItem;
}

public function displayAction(){
    if($this->checkUrlAction($this->sAction) === true){     
        return $this->sAction;
    }
}
public function displayItem(){
    if($this->checkUrlItem($this->sItem) === true){
        return $this->sItem;
    }
}       
private function checkUrlAction($sAction){

    if($sAction == 'insert' || $sAction == 'view' || $sAction == 'update' || $sAction == 'delete'){         
        return true;
    }
}

private function checkUrlItem($sItem){
    if($sItem == 'imagelist' || $sItem == 'mkdir' || $sItem == 'rdir'){
        return true;
    }
    else{       
        $objShowPDO = new mShowPDO();
        $result = $objShowPDO->allTables();     
        while($array = $result->fetch()){
            if($array[0] == $this->sItem){
                return true;
            }
        }       
    }
}



$objTitleBreadcrumb = new cTitleBreadcrumb($sAction,$sItem);

echo ucfirst($objTitleBreadcrumb->displayAction());

echo ucfirst($objTitleBreadcrumb->displayItem());
\$\endgroup\$

2 Answers 2

Reset to default
1
\$\begingroup\$

Just little improvement to write this method shorter

private function checkUrlAction($sAction)
{
    return in_array($sAction , array('insert','view','update''delete'));
}
\$\endgroup\$
4
  • \$\begingroup\$ A recommend to put the array in a class field. \$\endgroup\$ Commented Oct 30, 2012 at 18:47
  • \$\begingroup\$ @Peter put an array in a class field, what do you mean by that? \$\endgroup\$ Commented Oct 30, 2012 at 18:52
  • \$\begingroup\$ class cTitleBreadcrumb { private $_actions = array(...) } and then in the return statement: return in_array($sAction, $this->_actions)); \$\endgroup\$ Commented Oct 30, 2012 at 19:00
  • \$\begingroup\$ ahh ok i understand thanks. But i dont understand the PDO Dependency injection, can you explain it, whats exactly the problem and how can code it correctly? \$\endgroup\$ Commented Oct 30, 2012 at 19:17
1
\$\begingroup\$

You are not always returning anything

private function checkUrlAction($sAction) {
    if(true){         
        return true;
    }

    //but else?
}

Dependency injection

$objShowPDO = new mShowPDO(); is bad a mShowPDO instance should by injected via method or constructor argument see dependency injection topics

private function checkUrlItem(mShowPDO $objShowPDO, $sItem){
    if($sItem == 'imagelist' || $sItem == 'mkdir' || $sItem == 'rdir'){
        return true;
    }

    //redundand else removed

    $result = $objShowPDO->allTables(); //maybe you can inject only the result
    while($array = $result->fetch()){
        if($array[0] == $this->sItem){
            return true;
        }
    }

    return false;
}

Hard coding

The checklists are really hard coded (long if with OR relations), try use some container for them (an array and use in_array() for example).

\$\endgroup\$
6
  • \$\begingroup\$ Hi Peter, thanks for your reply. What do you mean by You are not always returning anything and whats the solution for this dependency injection? \$\endgroup\$ Commented Oct 30, 2012 at 16:03
  • \$\begingroup\$ He means that for the display functions you should always return something. If the sAction doesn't pass your check you don't return anything from displayAction() (and same for displayItem). \$\endgroup\$ Commented Oct 30, 2012 at 16:56
  • \$\begingroup\$ @Peter, do you mean like mysql_real_escape_string for mysql_*? \$\endgroup\$ Commented Oct 30, 2012 at 18:49
  • \$\begingroup\$ Sorry, but i don't understand how the mysql_ stuff comes here. \$\endgroup\$ Commented Oct 30, 2012 at 19:03
  • \$\begingroup\$ ahh ok i understand thanks. But i dont understand the PDO Dependency injection, can you explain it, whats exactly the problem and how can code it correctly? \$\endgroup\$ Commented Oct 31, 2012 at 4:13

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.