current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

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

Ok I have improved the code, is it now better? Kind regards Bas

class cTitleBreadcrumb{

    //set url params for check
    private $_actions = array('insert','view','update','delete');
    private $_items = array('imagelist','mkdir','sItem');

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

    //display action by checking if action isset
    public function displayAction(){
        if($this->checkUrlAction($this->sAction) === true){     
            return $this->sAction;
        }
    }

    //display item by checking if item isset
    public function displayItem(){
        if($this->checkUrlItem($this->sItem) === true){
            return $this->sItem;
        }
    }       

    //check if action is in array, see private params
    private function checkUrlAction($_actions){
        return in_array($_actions, $this->_actions);    
    }

    //check if item is in array, see private params
    private function checkUrlItem($_items){
        if(in_array($_items, $this->_items)){
            return true;
        }
        elseif($this->checkTables() === true){      
            return true;
        }
    }

    //check in dbase if tablename exist
    private function checkTables(){
            $objShowPDO = new mShowPDO();
            $result = $objShowPDO->allTables();     
            while($array = $result->fetch()){
                if($array[0] == $this->sItem){
                    return true;
                }
            }       

    }
share|improve this question

2 Answers 2

up vote 1 down vote

Just little improvement to write this method shorter

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

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).

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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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