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\$
1
\$\begingroup\$

Just little improvement to write this method shorter

private function checkUrlAction($sAction)
{
    return in_array($sAction , array('insert','view','update''delete'));
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ A recommend to put the array in a class field. \$\endgroup\$ – Peter Kiss Oct 30 '12 at 18:47
  • \$\begingroup\$ @Peter put an array in a class field, what do you mean by that? \$\endgroup\$ – Bas DL Oct 30 '12 at 18:52
  • \$\begingroup\$ class cTitleBreadcrumb { private $_actions = array(...) } and then in the return statement: return in_array($sAction, $this->_actions)); \$\endgroup\$ – Peter Kiss Oct 30 '12 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\$ – Bas DL Oct 30 '12 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).

| improve this answer | |
\$\endgroup\$
  • \$\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\$ – Bas DL Oct 30 '12 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\$ – nickles80 Oct 30 '12 at 16:56
  • \$\begingroup\$ @Peter, do you mean like mysql_real_escape_string for mysql_*? \$\endgroup\$ – Bas DL Oct 30 '12 at 18:49
  • \$\begingroup\$ Sorry, but i don't understand how the mysql_ stuff comes here. \$\endgroup\$ – Peter Kiss Oct 30 '12 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\$ – Bas DL Oct 31 '12 at 4:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.