Should I keep/remove the extra precaution?

[3raser]

/*
     * @METHOD  checkExistence
     * @DESC    check if the thread exists
     */
    
    public function checkExistence($id)
    {
        $thread = $database->processQuery("SELECT * FROM `threads` WHERE `id` = ? LIMIT 1", array($id), false);
        return ($database->getRowCount() == 1) ? $x = true : $x = false;
    }
    
    
    /*
     * @METHOD  canView
     * @DESC    checks if the user has permissions to see thread
     */
    
    public function canView($id, $username, $powerLevel)
    {
        //extract thread details
        $thread = $database->processQuery("SELECT `parent`,`hidden` FROM `threads` WHERE `id` = ? LIMIT 1", array($id), true);
        
        if($database->getRowCount() == 1)
        {
            $canSee = true;
            
            //get the parent's type
            $parent = $database->processQuery("SELECT `type` FROM `forums` WHERE `id` = ? LIMIT 1", array($thread[0]['parent']), true);
            
            if($parent[0]['type'] == 3 && ($username != $thread[0]['username'] && $powerLevel < 3))
            {
               $canSee = false; 
            }
            
            if($thread[0]['hidden'] == 1 && $powerLevel < 3)
            {
                $canSee = false;
            }
            
            return false;
        }
        
        return false;
    }

 

And then I preform the checks here:

 

//preform basic checks
if($thread->checkExistence($_GET['id']) == false) $base->redirect('index.php');
if($thread->canView($_GET['id'], $username, $rank) == false) $base->redirect('index.php');

 

But as you can see in the canView() function, it returns false if the thread is not found. So, should I remove the checkExistence() function, leave the code as is, or just remove the if thread exists check from canView()?

[requinix]

checkExistence() could be useful somewhere else, but as you said canView() makes calling it redundant.

[btherl]

I would remove the call to checkExistence().  Removing the check from canView() doesn't make sense because that query is required to get "parent" and "hidden" anyway.